RepositoryConfig: Ensure consistency in reading blobs and trees

...with respect to rejecting invalid entries such as upwards
symlinks. Also ensure that valid trees are only checked once by
remebering known valid tress though marker files in local storage.

8 files changed, 42 insertions, 16 deletions

diff --git a/src/buildtool/common/TARGETS b/src/buildtool/common/TARGETS
index f324d767..718e1f02 100644
--- a/src/buildtool/common/TARGETS
+++ b/src/buildtool/common/TARGETS
@@ -193,7 +193,10 @@
     , ["src/buildtool/storage", "storage"]
     ]
   , "stage": ["src", "buildtool", "common"]
-  , "private-deps": [["src/utils/automata", "dfa_minimizer"]]
+  , "private-deps":
+    [ ["src/buildtool/file_system", "git_tree_utils"]
+    , ["src/utils/automata", "dfa_minimizer"]
+    ]
   }
 , "user_structs":
   { "type": ["@", "rules", "CC", "library"]
diff --git a/src/buildtool/common/repository_config.cpp b/src/buildtool/common/repository_config.cpp
index e47fc8d3..988c65cd 100644
--- a/src/buildtool/common/repository_config.cpp
+++ b/src/buildtool/common/repository_config.cpp
@@ -16,6 +16,7 @@
 
 #include <initializer_list>
 
+#include "src/buildtool/file_system/git_tree_utils.hpp"
 #include "src/utils/automata/dfa_minimizer.hpp"
 
 auto RepositoryConfig::RepositoryInfo::BaseContentDescription() const
@@ -172,3 +173,12 @@ void RepositoryConfig::SetPrecomputedRoot(PrecomputedRoot const& root,
         }
     }
 }
+
+auto RepositoryConfig::ReadTreeFromGitCAS(
+    std::string const& hex_id) const noexcept -> std::optional<GitTree> {
+    if (git_cas_ == nullptr or storage_config_ == nullptr) {
+        return std::nullopt;
+    }
+    return GitTreeUtils::ReadValidGitCASTree(
+        *storage_config_, hex_id, git_cas_);
+}
diff --git a/src/buildtool/common/repository_config.hpp b/src/buildtool/common/repository_config.hpp
index 1a2f5be1..7fe66fa2 100644
--- a/src/buildtool/common/repository_config.hpp
+++ b/src/buildtool/common/repository_config.hpp
@@ -87,19 +87,18 @@ class RepositoryConfig {
 
     [[nodiscard]] auto ReadBlobFromGitCAS(
         std::string const& hex_id,
+        bool is_symlink,
         LogLevel log_failure = LogLevel::Warning) const noexcept
         -> std::optional<std::string> {
         return git_cas_ ? git_cas_->ReadObject(hex_id,
                                                /*is_hex_id=*/true,
-                                               /*validate=*/false,
+                                               /*as_valid_symlink=*/is_symlink,
                                                log_failure)
                         : std::nullopt;
     }
 
     [[nodiscard]] auto ReadTreeFromGitCAS(
-        std::string const& hex_id) const noexcept -> std::optional<GitTree> {
-        return git_cas_ ? GitTree::Read(git_cas_, hex_id) : std::nullopt;
-    }
+        std::string const& hex_id) const noexcept -> std::optional<GitTree>;
 
     [[nodiscard]] auto WorkspaceRoot(std::string const& repo) const noexcept
         -> FileRoot const* {
diff --git a/src/buildtool/execution_api/git/git_api.cpp b/src/buildtool/execution_api/git/git_api.cpp
index 231e717a..2c083e64 100644
--- a/src/buildtool/execution_api/git/git_api.cpp
+++ b/src/buildtool/execution_api/git/git_api.cpp
@@ -81,7 +81,8 @@ auto GitApi::RetrieveToPaths(
             }
         }
         else {
-            auto blob = repo_config_.ReadBlobFromGitCAS(info.digest.hash());
+            auto blob = repo_config_.ReadBlobFromGitCAS(
+                info.digest.hash(), /*is_symlink=*/IsSymlinkObject(info.type));
             if (not blob) {
                 return false;
             }
@@ -141,7 +142,8 @@ auto GitApi::RetrieveToFds(
             }
         }
         else {
-            auto blob = repo_config_.ReadBlobFromGitCAS(info.digest.hash());
+            auto blob = repo_config_.ReadBlobFromGitCAS(
+                info.digest.hash(), /*is_symlink=*/IsSymlinkObject(info.type));
             if (not blob) {
                 Logger::Log(LogLevel::Debug,
                             "Blob {} not known to git",
@@ -217,7 +219,9 @@ auto GitApi::RetrieveToCas(
             content = tree->RawData();
         }
         else {
-            content = repo_config_.ReadBlobFromGitCAS(info->digest.hash());
+            content = repo_config_.ReadBlobFromGitCAS(
+                info->digest.hash(),
+                /*is_symlink=*/IsSymlinkObject(info->type));
         }
         if (not content) {
             return false;
@@ -248,10 +252,14 @@ auto GitApi::RetrieveToCas(
 
 auto GitApi::RetrieveToMemory(Artifact::ObjectInfo const& artifact_info)
     const noexcept -> std::optional<std::string> {
-    return repo_config_.ReadBlobFromGitCAS(artifact_info.digest.hash());
+    return repo_config_.ReadBlobFromGitCAS(
+        artifact_info.digest.hash(),
+        /*is_symlink=*/IsSymlinkObject(artifact_info.type));
 }
 
 auto GitApi::IsAvailable(ArtifactDigest const& digest) const noexcept -> bool {
-    return repo_config_.ReadBlobFromGitCAS(digest.hash(), LogLevel::Trace)
-        .has_value();
+    return repo_config_
+        .ReadBlobFromGitCAS(
+            digest.hash(), /*is_symlink=*/false, LogLevel::Trace)
+        .has_value();  // allow invalid symlink if just to check existence
 }
diff --git a/src/buildtool/execution_api/utils/rehash_utils.cpp b/src/buildtool/execution_api/utils/rehash_utils.cpp
index 9e64a546..66d3a57f 100644
--- a/src/buildtool/execution_api/utils/rehash_utils.cpp
+++ b/src/buildtool/execution_api/utils/rehash_utils.cpp
@@ -285,10 +285,10 @@ auto RehashGitDigest(std::vector<Artifact::ObjectInfo> const& digests,
                      StorageConfig const& target_config,
                      RepositoryConfig const& repo_config)
     -> expected<std::vector<Artifact::ObjectInfo>, std::string> {
-    auto read = [&repo_config](
-                    ArtifactDigest const& digest,
-                    ObjectType /*type*/) -> std::optional<std::string> {
-        return repo_config.ReadBlobFromGitCAS(digest.hash());
+    auto read = [&repo_config](ArtifactDigest const& digest,
+                               ObjectType type) -> std::optional<std::string> {
+        return repo_config.ReadBlobFromGitCAS(
+            digest.hash(), /*is_symlink=*/IsSymlinkObject(type));
     };
     return RehashDigestImpl(digests,
                             source_config,
diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp
index d47397f0..e38a656f 100644
--- a/src/buildtool/execution_engine/executor/executor.hpp
+++ b/src/buildtool/execution_engine/executor/executor.hpp
@@ -486,7 +486,7 @@ class ExecutorImpl {
         }
         if (not blob) {
             // try to obtain blob from global Git CAS, if any
-            blob = repo_config->ReadBlobFromGitCAS(hash);
+            blob = repo_config->ReadBlobFromGitCAS(hash, /*is_symlink=*/false);
         }
         return blob;
     }
diff --git a/src/buildtool/file_system/TARGETS b/src/buildtool/file_system/TARGETS
index 5320482d..13efeba3 100644
--- a/src/buildtool/file_system/TARGETS
+++ b/src/buildtool/file_system/TARGETS
@@ -226,6 +226,7 @@
   , "private-deps":
     [ "file_system_manager"
     , "object_type"
+    , ["@", "gsl", "", "gsl"]
     , ["src/buildtool/storage", "fs_utils"]
     ]
   , "stage": ["src", "buildtool", "file_system"]
diff --git a/src/buildtool/file_system/git_tree_utils.cpp b/src/buildtool/file_system/git_tree_utils.cpp
index e3e290d1..98b2b38d 100644
--- a/src/buildtool/file_system/git_tree_utils.cpp
+++ b/src/buildtool/file_system/git_tree_utils.cpp
@@ -15,7 +15,12 @@
 #include "src/buildtool/file_system/git_tree_utils.hpp"
 
 #include <cstddef>
+#include <filesystem>
+#include <memory>
+#include <optional>
+#include <unordered_map>
 
+#include "gsl/gsl"
 #include "src/buildtool/file_system/file_system_manager.hpp"
 #include "src/buildtool/file_system/object_type.hpp"
 #include "src/buildtool/storage/fs_utils.hpp"