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/remote/bazel/bazel_response.cpp | 106 ++++++++++++--------- 1 file changed, 61 insertions(+), 45 deletions(-) (limited to 'src/buildtool/execution_api/remote/bazel/bazel_response.cpp') 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); } -- cgit v1.2.3