From c70ce16da685a4972145a42b3aa6d37a67dc56b1 Mon Sep 17 00:00:00 2001
From: Paul Cristian Sarbu <paul.cristian.sarbu@huawei.com>
Date: Tue, 27 Aug 2024 15:38:12 +0200
Subject: SourceTree: Check for missing value after using resolve_symlinks_map

Fixes a false assumption that the result of resolving the tree will
always be set if the map doesn't log fatal, when in fact the map
might fail to set a value if, e.g., a thread is killed by the
system or there is a symlinks cycle.
---
 .../serve_api/serve_service/source_tree.cpp        | 34 +++++++++++++---------
 1 file changed, 20 insertions(+), 14 deletions(-)

(limited to 'src/buildtool/serve_api/serve_service/source_tree.cpp')

diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp
index aaa0256e..8b049691 100644
--- a/src/buildtool/serve_api/serve_service/source_tree.cpp
+++ b/src/buildtool/serve_api/serve_service/source_tree.cpp
@@ -360,7 +360,7 @@ auto SourceTreeService::ResolveContentTree(
                 return ::grpc::Status::OK;
             }
         }
-        ResolvedGitObject resolved_tree{};
+        std::optional<ResolvedGitObject> resolved_tree = std::nullopt;
         bool failed{false};
         {
             TaskSystem ts{serve_config_.jobs};
@@ -388,16 +388,21 @@ auto SourceTreeService::ResolveContentTree(
             response->set_status(ServeArchiveTreeResponse::RESOLVE_ERROR);
             return ::grpc::Status::OK;
         }
-        // check for cycles
-        if (auto error = DetectAndReportCycle(
-                fmt::format("resolving Git tree {}", tree_id),
-                resolve_symlinks_map_,
-                kGitObjectToResolvePrinter)) {
+        // check if we have a value
+        if (not resolved_tree) {
+            // check for cycles
+            if (auto error = DetectAndReportCycle(
+                    fmt::format("resolving symlinks in tree {}", tree_id),
+                    resolve_symlinks_map_,
+                    kGitObjectToResolvePrinter)) {
+                logger_->Emit(LogLevel::Error, *error);
+                response->set_status(ServeArchiveTreeResponse::RESOLVE_ERROR);
+                return ::grpc::Status::OK;
+            }
             logger_->Emit(LogLevel::Error,
-                          "Failed to resolve symlinks in tree {}:\n{}",
-                          tree_id,
-                          *error);
-            response->set_status(ServeArchiveTreeResponse::RESOLVE_ERROR);
+                          "Unknown error while resolving tree id {}",
+                          tree_id);
+            response->set_status(ServeArchiveTreeResponse::INTERNAL_ERROR);
             return ::grpc::Status::OK;
         }
         // keep tree alive in the Git cache via a tagged commit
@@ -408,7 +413,7 @@ auto SourceTreeService::ResolveContentTree(
                 if (fatal) {
                     logger->Emit(LogLevel::Error,
                                  "While keeping tree {} in repository {}:\n{}",
-                                 resolved_tree.id,
+                                 resolved_tree->id,
                                  storage_config->GitRoot().string(),
                                  msg);
                 }
@@ -426,7 +431,7 @@ auto SourceTreeService::ResolveContentTree(
                 return ::grpc::Status::OK;
             }
             // Important: message must be consistent with just-mr!
-            if (not git_repo->KeepTree(resolved_tree.id,
+            if (not git_repo->KeepTree(resolved_tree->id,
                                        "Keep referenced tree alive",  // message
                                        wrapped_logger)) {
                 response->set_status(ServeArchiveTreeResponse::RESOLVE_ERROR);
@@ -434,14 +439,15 @@ auto SourceTreeService::ResolveContentTree(
             }
         }
         // cache the resolved tree association
-        if (not StorageUtils::WriteTreeIDFile(tree_id_file, resolved_tree.id)) {
+        if (not StorageUtils::WriteTreeIDFile(tree_id_file,
+                                              resolved_tree->id)) {
             logger_->Emit(LogLevel::Error,
                           "Failed to write resolved tree id to file {}",
                           tree_id_file.string());
             response->set_status(ServeArchiveTreeResponse::RESOLVE_ERROR);
             return ::grpc::Status::OK;
         }
-        return SyncArchive(resolved_tree.id, repo_path, sync_tree, response);
+        return SyncArchive(resolved_tree->id, repo_path, sync_tree, response);
     }
     // if no special handling of symlinks, use given tree as-is
     return SyncArchive(tree_id, repo_path, sync_tree, response);
-- 
cgit v1.2.3