From dd9fa2841fcb5983b4ea845d5f9dc1b635d8dd18 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Fri, 16 May 2025 17:35:41 +0200 Subject: Executor: Check validity of action outputs in compatible mode This ensures that any entries that the standard remote execution protocol accepts but are invalid in justbuild, i.e., upwards symlinks, are rejected. For this purpose, do not fail in the action response instances, just perform the check there, as all required information is available, and set a flag that the executor can check as needed. --- .../execution_api/common/execution_response.hpp | 2 + .../execution_api/local/local_response.hpp | 57 +++++------ .../execution_api/remote/bazel/bazel_response.cpp | 106 ++++++++++++--------- .../execution_api/remote/bazel/bazel_response.hpp | 14 ++- .../execution_engine/executor/executor.hpp | 25 ++++- 5 files changed, 126 insertions(+), 78 deletions(-) (limited to 'src') diff --git a/src/buildtool/execution_api/common/execution_response.hpp b/src/buildtool/execution_api/common/execution_response.hpp index 882750fe..65fd0ba9 100644 --- a/src/buildtool/execution_api/common/execution_response.hpp +++ b/src/buildtool/execution_api/common/execution_response.hpp @@ -77,6 +77,8 @@ class IExecutionResponse { -> expected, std::string> = 0; [[nodiscard]] virtual auto DirectorySymlinks() noexcept -> expected, std::string> = 0; + [[nodiscard]] virtual auto HasUpwardsSymlinks() noexcept + -> expected = 0; }; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_COMMON_REMOTE_EXECUTION_RESPONSE_HPP diff --git a/src/buildtool/execution_api/local/local_response.hpp b/src/buildtool/execution_api/local/local_response.hpp index 790bb384..f5ed3e94 100644 --- a/src/buildtool/execution_api/local/local_response.hpp +++ b/src/buildtool/execution_api/local/local_response.hpp @@ -33,7 +33,6 @@ #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" @@ -121,12 +120,20 @@ class LocalResponse final : public IExecutionResponse { &dir_symlinks_); // explicit type needed for expected } + auto HasUpwardsSymlinks() noexcept -> expected final { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return has_upwards_symlinks_; + } + private: std::string action_id_; LocalAction::Output output_{}; Storage const& storage_; ArtifactInfos artifacts_; DirSymlinks dir_symlinks_; + bool has_upwards_symlinks_ = false; // only tracked in compatible mode bool populated_ = false; explicit LocalResponse( @@ -188,14 +195,11 @@ 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()); - } + // in compatible mode: track upwards symlinks + has_upwards_symlinks_ = + has_upwards_symlinks_ or + (not ProtocolTraits::IsNative(hash_type) and + not PathIsNonUpwards(link.target())); artifacts.emplace( link.path(), Artifact::ObjectInfo{ @@ -213,14 +217,11 @@ 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()); - } + // in compatible mode: track upwards symlinks + has_upwards_symlinks_ = + has_upwards_symlinks_ or + (not ProtocolTraits::IsNative(hash_type) and + not PathIsNonUpwards(link.target())); artifacts.emplace( link.path(), Artifact::ObjectInfo{ @@ -248,18 +249,18 @@ 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{&storage_.CAS()}; - auto result = reader.RecursivelyReadTreeLeafs( - *digest, "", /*include_trees=*/true); - if (not result) { - return fmt::format( - "LocalResponse: found invalid entries in directory " - "{}", - dir.path()); + // in compatible mode: track upwards symlinks; requires one + // directory traversal; other sources of errors should cause a + // fail too, so it is ok to report all traversal errors as + // if an invalid entry was found + if (not has_upwards_symlinks_ and + not ProtocolTraits::IsNative(hash_type)) { + LocalCasReader reader{&storage_.CAS()}; + auto valid_dir = reader.IsDirectoryValid(*digest); + if (not valid_dir) { + return std::move(valid_dir).error(); } + has_upwards_symlinks_ = not *valid_dir; } artifacts.emplace( dir.path(), diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp index 81392813..e11a3b7f 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.cpp @@ -14,6 +14,7 @@ #include "src/buildtool/execution_api/remote/bazel/bazel_response.hpp" +#include #include #include #include @@ -40,18 +41,22 @@ namespace { +/// \brief Return auto ProcessDirectoryMessage(HashFunction hash_function, bazel_re::Directory const& dir) noexcept - -> expected { - // in compatible mode: check validity of all symlinks - for (auto const& link : dir.symlinks()) { - if (not PathIsNonUpwards(link.target())) { - return unexpected{ - fmt::format("found invalid symlink at {}", link.name())}; - } - } - return ArtifactBlob::FromMemory( + -> expected, + std::string> { + // in compatible mode: track upwards symlinks + bool has_upwards_symlinks = std::any_of( + dir.symlinks().begin(), dir.symlinks().end(), [](auto const& link) { + return not PathIsNonUpwards(link.target()); + }); + auto blob = ArtifactBlob::FromMemory( hash_function, ObjectType::File, dir.SerializeAsString()); + if (not blob) { + return unexpected{std::move(blob).error()}; + } + return std::make_pair(std::move(blob).value(), has_upwards_symlinks); } } // namespace @@ -92,6 +97,14 @@ auto BazelResponse::DirectorySymlinks() noexcept &dir_symlinks_); // explicit type needed for expected } +auto BazelResponse::HasUpwardsSymlinks() noexcept + -> expected { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return has_upwards_symlinks_; +} + auto BazelResponse::Populate() noexcept -> std::optional { // Initialized only once lazily if (populated_) { @@ -140,13 +153,10 @@ auto BazelResponse::Populate() noexcept -> std::optional { // 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( - network_->GetHashFunction().GetType()) and - not PathIsNonUpwards(link.target())) { - return fmt::format("BazelResponse: found invalid symlink at {}", - link.path()); - } + // in compatible mode: track upwards symlinks + has_upwards_symlinks_ = has_upwards_symlinks_ or + (not ProtocolTraits::IsNative(hash_type) and + not PathIsNonUpwards(link.target())); artifacts.emplace( link.path(), Artifact::ObjectInfo{ @@ -164,13 +174,10 @@ auto BazelResponse::Populate() noexcept -> std::optional { } for (auto const& link : action_result.output_directory_symlinks()) { try { - // in compatible mode: check symlink validity - if (not ProtocolTraits::IsNative( - network_->GetHashFunction().GetType()) and - not PathIsNonUpwards(link.target())) { - return fmt::format("BazelResponse: found invalid symlink at {}", - link.path()); - } + // in compatible mode: track upwards symlinks + has_upwards_symlinks_ = has_upwards_symlinks_ or + (not ProtocolTraits::IsNative(hash_type) and + not PathIsNonUpwards(link.target())); artifacts.emplace( link.path(), Artifact::ObjectInfo{ @@ -223,8 +230,8 @@ auto BazelResponse::Populate() noexcept -> std::optional { tree_digests.reserve( gsl::narrow(action_result.output_directories_size())); for (auto const& directory : action_result.output_directories()) { - auto digest = ArtifactDigestFactory::FromBazel( - network_->GetHashFunction().GetType(), directory.tree_digest()); + auto digest = ArtifactDigestFactory::FromBazel(hash_type, + directory.tree_digest()); if (not digest) { return std::move(digest).error(); } @@ -251,16 +258,20 @@ auto BazelResponse::Populate() noexcept -> std::optional { // has sent us as part of the Tree message. If we want to be // able to use the Directories as inputs for actions, we // have to upload them manually. - auto root_digest = UploadTreeMessageDirectories(*tree); - if (not root_digest) { - auto error = - fmt::format("BazelResponse: {}", root_digest.error()); + auto upload_result = UploadTreeMessageDirectories(*tree); + if (not upload_result) { + auto error = fmt::format("BazelResponse: {}", + std::move(upload_result).error()); Logger::Log(LogLevel::Trace, error); return error; } - artifacts.emplace(action_result.output_directories(pos).path(), - Artifact::ObjectInfo{.digest = *root_digest, - .type = ObjectType::Tree}); + has_upwards_symlinks_ = + has_upwards_symlinks_ or upload_result.value().second; + artifacts.emplace( + action_result.output_directories(pos).path(), + Artifact::ObjectInfo{ + .digest = std::move(upload_result).value().first, + .type = ObjectType::Tree}); } catch (std::exception const& ex) { return fmt::format( "BazelResponse: unexpected failure gathering digest for " @@ -276,8 +287,9 @@ auto BazelResponse::Populate() noexcept -> std::optional { return std::nullopt; } -auto BazelResponse::UploadTreeMessageDirectories( - bazel_re::Tree const& tree) const -> expected { +auto BazelResponse::UploadTreeMessageDirectories(bazel_re::Tree const& tree) + const -> expected, + std::string> { auto const upload_callback = [&network = *network_](std::unordered_set&& blobs) -> bool { @@ -285,22 +297,26 @@ auto BazelResponse::UploadTreeMessageDirectories( }; auto const hash_function = network_->GetHashFunction(); - auto rootdir_blob = ProcessDirectoryMessage(hash_function, tree.root()); - if (not rootdir_blob) { - return unexpected{std::move(rootdir_blob).error()}; + auto rootdir_result = ProcessDirectoryMessage(hash_function, tree.root()); + if (not rootdir_result) { + return unexpected{std::move(rootdir_result).error()}; } - auto const root_digest = rootdir_blob->GetDigest(); - std::unordered_set dir_blobs{*std::move(rootdir_blob)}; + auto rootdir_has_upwards_symlinks = rootdir_result.value().second; + auto const root_digest = rootdir_result.value().first.GetDigest(); + std::unordered_set dir_blobs{ + std::move(rootdir_result).value().first}; for (auto const& subdir : tree.children()) { // store or upload blob, taking maximum transfer size into account - auto blob = ProcessDirectoryMessage(hash_function, subdir); - if (not blob) { - return unexpected{std::move(blob).error()}; + auto subdir_result = ProcessDirectoryMessage(hash_function, subdir); + if (not subdir_result) { + return unexpected{std::move(subdir_result).error()}; } - auto const blob_digest = blob->GetDigest(); + rootdir_has_upwards_symlinks = + rootdir_has_upwards_symlinks or subdir_result.value().second; + auto const blob_digest = subdir_result.value().first.GetDigest(); if (not UpdateContainerAndUpload(&dir_blobs, - *std::move(blob), + std::move(subdir_result).value().first, /*exception_is_fatal=*/false, upload_callback)) { return unexpected{ @@ -315,5 +331,5 @@ auto BazelResponse::UploadTreeMessageDirectories( fmt::format("failed to upload blobs for Tree with root digest {}", root_digest.hash())}; } - return root_digest; + return std::make_pair(root_digest, rootdir_has_upwards_symlinks); } diff --git a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp index df652141..55f45236 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_response.hpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_response.hpp @@ -19,7 +19,7 @@ #include #include #include -#include // std::move +#include // std::move, std:pair #include @@ -91,6 +91,7 @@ class BazelResponse final : public IExecutionResponse { -> expected, std::string> final; auto DirectorySymlinks() noexcept -> expected, std::string> final; + auto HasUpwardsSymlinks() noexcept -> expected final; private: std::string action_id_; @@ -98,6 +99,7 @@ class BazelResponse final : public IExecutionResponse { BazelExecutionClient::ExecutionOutput output_{}; ArtifactInfos artifacts_; DirSymlinks dir_symlinks_; + bool has_upwards_symlinks_ = false; // only tracked in compatible mode bool populated_ = false; explicit BazelResponse(std::string action_id, @@ -119,8 +121,14 @@ class BazelResponse final : public IExecutionResponse { /// \returns Error message on failure, nullopt on success. [[nodiscard]] auto Populate() noexcept -> std::optional; - [[nodiscard]] auto UploadTreeMessageDirectories(bazel_re::Tree const& tree) - const -> expected; + /// \brief Tries to upload the tree rot and subdirectories. Performs also a + /// symlinks check. + /// \returns Pair of ArtifactDigest of root tree and flag signaling the + /// presence of any upwards symlinks on success, error message on failure. + [[nodiscard]] auto UploadTreeMessageDirectories( + bazel_re::Tree const& tree) const + -> expected, + std::string>; }; #endif // INCLUDED_SRC_BUILDTOOL_EXECUTION_API_REMOTE_BAZEL_BAZEL_RESPONSE_HPP diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index cfdbf9fc..2d86a405 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -237,9 +237,30 @@ class ExecutorImpl { // set action options remote_action->SetCacheFlag(cache_flag); remote_action->SetTimeout(timeout); + + // execute action auto result = remote_action->Execute(&logger); - if (alternative_api) { - if (result) { + + // process result + if (result) { + // in compatible mode, check that all artifacts are valid + if (not ProtocolTraits::IsNative(api.GetHashType())) { + auto upwards_symlinks_check = result->HasUpwardsSymlinks(); + if (not upwards_symlinks_check) { + logger.Emit(LogLevel::Error, + upwards_symlinks_check.error()); + return nullptr; + } + if (upwards_symlinks_check.value()) { + logger.Emit( + LogLevel::Error, + "Executed action produced invalid outputs -- " + "upwards symlinks"); + return nullptr; + } + } + // if alternative endpoint used, transfer any missing blobs + if (alternative_api) { auto const artifacts = result->Artifacts(); if (not artifacts) { logger.Emit(LogLevel::Error, artifacts.error()); -- cgit v1.2.3