diff options
Diffstat (limited to 'src/buildtool')
5 files changed, 126 insertions, 78 deletions
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<gsl::not_null<ArtifactInfos const*>, std::string> = 0; [[nodiscard]] virtual auto DirectorySymlinks() noexcept -> expected<gsl::not_null<DirSymlinks const*>, std::string> = 0; + [[nodiscard]] virtual auto HasUpwardsSymlinks() noexcept + -> expected<bool, std::string> = 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<bool, std::string> 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<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()); + // 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 <algorithm> #include <cstddef> #include <exception> #include <filesystem> @@ -40,18 +41,22 @@ namespace { +/// \brief Return auto ProcessDirectoryMessage(HashFunction hash_function, bazel_re::Directory const& dir) noexcept - -> expected<ArtifactBlob, std::string> { - // 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::pair<ArtifactBlob, /*has_upwards_symlinks*/ bool>, + 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<bool, std::string> { + if (auto error_msg = Populate()) { + return unexpected{*std::move(error_msg)}; + } + return has_upwards_symlinks_; +} + auto BazelResponse::Populate() noexcept -> std::optional<std::string> { // Initialized only once lazily if (populated_) { @@ -140,13 +153,10 @@ auto BazelResponse::Populate() noexcept -> std::optional<std::string> { // 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<std::string> { } 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<std::string> { tree_digests.reserve( gsl::narrow<std::size_t>(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<std::string> { // 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<std::string> { return std::nullopt; } -auto BazelResponse::UploadTreeMessageDirectories( - bazel_re::Tree const& tree) const -> expected<ArtifactDigest, std::string> { +auto BazelResponse::UploadTreeMessageDirectories(bazel_re::Tree const& tree) + const -> expected<std::pair<ArtifactDigest, /*has_upwards_symlinks*/ bool>, + std::string> { auto const upload_callback = [&network = *network_](std::unordered_set<ArtifactBlob>&& 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<ArtifactBlob> 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<ArtifactBlob> 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 <memory> #include <optional> #include <string> -#include <utility> // std::move +#include <utility> // std::move, std:pair #include <grpcpp/support/status.h> @@ -91,6 +91,7 @@ class BazelResponse final : public IExecutionResponse { -> expected<gsl::not_null<ArtifactInfos const*>, std::string> final; auto DirectorySymlinks() noexcept -> expected<gsl::not_null<DirSymlinks const*>, std::string> final; + auto HasUpwardsSymlinks() noexcept -> expected<bool, std::string> 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<std::string>; - [[nodiscard]] auto UploadTreeMessageDirectories(bazel_re::Tree const& tree) - const -> expected<ArtifactDigest, std::string>; + /// \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::pair<ArtifactDigest, /*has_upwards_symlinks*/ bool>, + 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()); |