local execution: Check validity of symlinks

Invalid entries, currently all upwards symlinks (pending
implementation of a better way of handling them), are now
identified and handled similarly to remote execution: in compatible
mode on the client side, during handling of local response, and in
native mode during the population of the local action result.

6 files changed, 78 insertions, 14 deletions

diff --git a/src/buildtool/execution_api/bazel_msg/TARGETS b/src/buildtool/execution_api/bazel_msg/TARGETS
index d2a3147a..eb78742d 100644
--- a/src/buildtool/execution_api/bazel_msg/TARGETS
+++ b/src/buildtool/execution_api/bazel_msg/TARGETS
@@ -34,6 +34,7 @@
     ]
   , "private-deps":
     [ ["src/utils/cpp", "hex_string"]
+    , ["src/utils/cpp", "path"]
     , ["src/buildtool/file_system", "file_system_manager"]
     , ["src/buildtool/file_system", "git_repo"]
     , ["src/buildtool/common", "artifact_digest_factory"]
diff --git a/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp b/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp
index a48e7541..1992961d 100644
--- a/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp
+++ b/src/buildtool/execution_api/bazel_msg/bazel_msg_factory.cpp
@@ -29,6 +29,7 @@
 #include "src/buildtool/file_system/file_system_manager.hpp"
 #include "src/buildtool/file_system/git_repo.hpp"
 #include "src/utils/cpp/hex_string.hpp"
+#include "src/utils/cpp/path.hpp"
 
 namespace {
 /// \brief Serialize protobuf message to string.
@@ -427,7 +428,7 @@ auto BazelMsgFactory::CreateGitTreeDigestFromLocalTree(
         try {
             if (IsSymlinkObject(type)) {
                 auto content = FileSystemManager::ReadSymlink(full_name);
-                if (content) {
+                if (content and PathIsNonUpwards(*content)) {
                     if (auto digest = store_symlink(*content)) {
                         if (auto raw_id = FromHexString(digest->hash())) {
                             entries[std::move(*raw_id)].emplace_back(
diff --git a/src/buildtool/execution_api/common/tree_reader_utils.cpp b/src/buildtool/execution_api/common/tree_reader_utils.cpp
index 855c1842..13775201 100644
--- a/src/buildtool/execution_api/common/tree_reader_utils.cpp
+++ b/src/buildtool/execution_api/common/tree_reader_utils.cpp
@@ -106,6 +106,12 @@ auto TreeReaderUtils::ReadObjectInfos(bazel_re::Directory const& dir,
         }
 
         for (auto const& l : dir.symlinks()) {
+            // check validity of symlinks
+            if (not PathIsNonUpwards(l.target())) {
+                Logger::Log(
+                    LogLevel::Error, "found invalid symlink at {}", l.name());
+                return false;
+            }
             if (not store_info(l.name(), CreateObjectInfo(hash_function, l))) {
                 return false;
             }
diff --git a/src/buildtool/execution_api/local/TARGETS b/src/buildtool/execution_api/local/TARGETS
index 9bf9de97..a685d283 100644
--- a/src/buildtool/execution_api/local/TARGETS
+++ b/src/buildtool/execution_api/local/TARGETS
@@ -44,6 +44,7 @@
     , ["src/buildtool/execution_api/execution_service", "cas_utils"]
     , ["src/buildtool/file_system", "git_repo"]
     , ["src/utils/cpp", "expected"]
+    , ["src/utils/cpp", "path"]
     , ["src/utils/cpp", "tmp_dir"]
     , ["src/buildtool/crypto", "hash_function"]
     ]
@@ -54,7 +55,6 @@
     , ["src/buildtool/file_system", "file_system_manager"]
     , ["src/buildtool/execution_api/utils", "outputscheck"]
     , ["src/buildtool/crypto", "hash_function"]
-    , ["src/utils/cpp", "path"]
     ]
   }
 , "context":
diff --git a/src/buildtool/execution_api/local/local_action.cpp b/src/buildtool/execution_api/local/local_action.cpp
index 1f17ed08..10359e92 100644
--- a/src/buildtool/execution_api/local/local_action.cpp
+++ b/src/buildtool/execution_api/local/local_action.cpp
@@ -22,6 +22,7 @@
 #include <utility>
 
 #include "src/buildtool/common/artifact_digest_factory.hpp"
+#include "src/buildtool/common/protocol_traits.hpp"
 #include "src/buildtool/execution_api/common/tree_reader.hpp"
 #include "src/buildtool/execution_api/local/local_cas_reader.hpp"
 #include "src/buildtool/execution_api/local/local_response.hpp"
@@ -30,6 +31,7 @@
 #include "src/buildtool/file_system/object_type.hpp"
 #include "src/buildtool/logging/log_level.hpp"
 #include "src/buildtool/system/system_command.hpp"
+#include "src/utils/cpp/path.hpp"
 
 namespace {
 
@@ -367,12 +369,21 @@ auto LocalAction::CollectOutputFileOrSymlink(
         return std::nullopt;
     }
     if (IsSymlinkObject(*type)) {
-        auto content = FileSystemManager::ReadSymlink(file_path);
-        if (content and local_context_.storage->CAS().StoreBlob(*content)) {
-            auto out_symlink = bazel_re::OutputSymlink{};
-            out_symlink.set_path(local_path);
-            out_symlink.set_target(*content);
-            return out_symlink;
+        if (auto content = FileSystemManager::ReadSymlink(file_path)) {
+            // in native mode: check validity of symlink
+            if (ProtocolTraits::IsNative(
+                    local_context_.storage->GetHashFunction().GetType()) and
+                not PathIsNonUpwards(*content)) {
+                Logger::Log(
+                    LogLevel::Error, "found invalid symlink at {}", local_path);
+                return std::nullopt;
+            }
+            if (local_context_.storage->CAS().StoreBlob(*content)) {
+                auto out_symlink = bazel_re::OutputSymlink{};
+                out_symlink.set_path(local_path);
+                out_symlink.set_target(*content);
+                return out_symlink;
+            }
         }
     }
     else if (IsFileObject(*type)) {
@@ -406,12 +417,21 @@ auto LocalAction::CollectOutputDirOrSymlink(
         return std::nullopt;
     }
     if (IsSymlinkObject(*type)) {
-        auto content = FileSystemManager::ReadSymlink(dir_path);
-        if (content and local_context_.storage->CAS().StoreBlob(*content)) {
-            auto out_symlink = bazel_re::OutputSymlink{};
-            out_symlink.set_path(local_path);
-            out_symlink.set_target(*content);
-            return out_symlink;
+        if (auto content = FileSystemManager::ReadSymlink(dir_path)) {
+            // in native mode: check validity of symlink
+            if (ProtocolTraits::IsNative(
+                    local_context_.storage->GetHashFunction().GetType()) and
+                not PathIsNonUpwards(*content)) {
+                Logger::Log(
+                    LogLevel::Error, "found invalid symlink at {}", local_path);
+                return std::nullopt;
+            }
+            if (local_context_.storage->CAS().StoreBlob(*content)) {
+                auto out_symlink = bazel_re::OutputSymlink{};
+                out_symlink.set_path(local_path);
+                out_symlink.set_target(*content);
+                return out_symlink;
+            }
         }
     }
     else if (IsTreeObject(*type)) {
@@ -423,6 +443,9 @@ auto LocalAction::CollectOutputDirOrSymlink(
                 ArtifactDigestFactory::ToBazel(*digest);
             return out_dir;
         }
+        Logger::Log(LogLevel::Error,
+                    "found invalid entries in directory at {}",
+                    local_path);
     }
     else {
         Logger::Log(
diff --git a/src/buildtool/execution_api/local/local_response.hpp b/src/buildtool/execution_api/local/local_response.hpp
index 141ffbe3..1bb2480b 100644
--- a/src/buildtool/execution_api/local/local_response.hpp
+++ b/src/buildtool/execution_api/local/local_response.hpp
@@ -23,14 +23,18 @@
 #include "fmt/core.h"
 #include "gsl/gsl"
 #include "src/buildtool/common/artifact_digest_factory.hpp"
+#include "src/buildtool/common/protocol_traits.hpp"
 #include "src/buildtool/crypto/hash_function.hpp"
 #include "src/buildtool/execution_api/common/execution_response.hpp"
+#include "src/buildtool/execution_api/common/tree_reader.hpp"
 #include "src/buildtool/execution_api/local/local_action.hpp"
+#include "src/buildtool/execution_api/local/local_cas_reader.hpp"
 #include "src/buildtool/file_system/file_system_manager.hpp"
 #include "src/buildtool/logging/log_level.hpp"
 #include "src/buildtool/logging/logger.hpp"
 #include "src/buildtool/storage/storage.hpp"
 #include "src/utils/cpp/expected.hpp"
+#include "src/utils/cpp/path.hpp"
 
 /// \brief Response of a LocalAction.
 class LocalResponse final : public IExecutionResponse {
@@ -155,6 +159,14 @@ class LocalResponse final : public IExecutionResponse {
         // collect all symlinks and store them
         for (auto const& link : action_result.output_file_symlinks()) {
             try {
+                // in compatible mode: check symlink validity
+                if (not ProtocolTraits::IsNative(
+                        storage_.GetHashFunction().GetType()) and
+                    not PathIsNonUpwards(link.target())) {
+                    return fmt::format(
+                        "LocalResponse: found invalid symlink at {}",
+                        link.path());
+                }
                 artifacts.emplace(
                     link.path(),
                     Artifact::ObjectInfo{
@@ -172,6 +184,14 @@ class LocalResponse final : public IExecutionResponse {
         }
         for (auto const& link : action_result.output_directory_symlinks()) {
             try {
+                // in compatible mode: check symlink validity
+                if (not ProtocolTraits::IsNative(
+                        storage_.GetHashFunction().GetType()) and
+                    not PathIsNonUpwards(link.target())) {
+                    return fmt::format(
+                        "LocalResponse: found invalid symlink at {}",
+                        link.path());
+                }
                 artifacts.emplace(
                     link.path(),
                     Artifact::ObjectInfo{
@@ -199,6 +219,19 @@ class LocalResponse final : public IExecutionResponse {
                     dir.path());
             }
             try {
+                // in compatible mode: check validity of symlinks in dir
+                if (not ProtocolTraits::IsNative(
+                        storage_.GetHashFunction().GetType())) {
+                    auto reader = TreeReader<LocalCasReader>{&storage_.CAS()};
+                    auto result = reader.RecursivelyReadTreeLeafs(
+                        *digest, "", /*include_trees=*/true);
+                    if (not result) {
+                        return fmt::format(
+                            "LocalResponse: found invalid entries in directory "
+                            "{}",
+                            dir.path());
+                    }
+                }
                 artifacts.emplace(
                     dir.path(),
                     Artifact::ObjectInfo{.digest = *std::move(digest),