From ed9bfe29a1d54a6dca1fa85853457c56108a6183 Mon Sep 17 00:00:00 2001 From: Paul Cristian Sarbu Date: Mon, 27 Mar 2023 18:15:08 +0200 Subject: GitRepo: Guard fake repository odb wrapping In the current libgit2 implementation, a fake repository wrapped around an existing odb is being registered as owner the same way as a normal repository object. Therefore, one has to guard both the creation and destruction of the fake repository against all other git operations that might access the internal cache during this transfer of ownership. --- src/buildtool/file_system/git_repo.cpp | 149 +++++++++++++++++++++++---------- 1 file changed, 105 insertions(+), 44 deletions(-) (limited to 'src/buildtool/file_system/git_repo.cpp') diff --git a/src/buildtool/file_system/git_repo.cpp b/src/buildtool/file_system/git_repo.cpp index 31bfed38..2e09f84d 100644 --- a/src/buildtool/file_system/git_repo.cpp +++ b/src/buildtool/file_system/git_repo.cpp @@ -243,6 +243,26 @@ auto const kInMemoryODBParent = CreateInMemoryODBParent(); } // namespace #endif // BOOTSTRAP_BUILD_TOOL +GitRepo::GuardedRepo::GuardedRepo(std::shared_mutex* mutex) noexcept + : mutex_{mutex} {} + +auto GitRepo::GuardedRepo::Ptr() -> git_repository* { + return repo_; +} + +auto GitRepo::GuardedRepo::PtrRef() -> git_repository** { + return &repo_; +} + +GitRepo::GuardedRepo::~GuardedRepo() noexcept { +#ifndef BOOTSTRAP_BUILD_TOOL + if (repo_ != nullptr) { + std::unique_lock lock{*mutex_}; + git_repository_free(repo_); + } +#endif +} + auto GitRepo::Open(GitCASPtr git_cas) noexcept -> std::optional { #ifdef BOOTSTRAP_BUILD_TOOL return std::nullopt; @@ -270,21 +290,31 @@ auto GitRepo::Open(std::filesystem::path const& repo_path) noexcept GitRepo::GitRepo(GitCASPtr git_cas) noexcept { #ifndef BOOTSTRAP_BUILD_TOOL - if (git_cas != nullptr) { - git_repository* repo_ptr{nullptr}; - if (git_repository_wrap_odb(&repo_ptr, git_cas->odb_.get()) != 0) { + try { + if (git_cas != nullptr) { + auto repo_ptr = std::make_shared(&git_cas->mutex_); + { + // acquire the odb lock exclusively + std::unique_lock lock{git_cas->mutex_}; + if (git_repository_wrap_odb(repo_ptr->PtrRef(), + git_cas->odb_.get()) != 0) { + Logger::Log(LogLevel::Error, + "could not create wrapper for git repository"); + return; + } + } + repo_ = std::move(repo_ptr); + is_repo_fake_ = true; + git_cas_ = std::move(git_cas); + } + else { Logger::Log(LogLevel::Error, - "could not create wrapper for git repository"); - git_repository_free(repo_ptr); - return; + "git repository creation attempted with null odb!"); } - repo_.reset(repo_ptr); // retain repo - is_repo_fake_ = true; - git_cas_ = std::move(git_cas); - } - else { + } catch (std::exception const& ex) { Logger::Log(LogLevel::Error, - "git repository creation attempted with null odb!"); + "opening git object database failed with:\n{}", + ex.what()); } #endif // BOOTSTRAP_BUILD_TOOL } @@ -296,8 +326,8 @@ GitRepo::GitRepo(std::filesystem::path const& repo_path) noexcept { std::unique_lock lock{repo_mutex}; auto cas = std::make_shared(); // open repo, but retain it - git_repository* repo_ptr{nullptr}; - if (git_repository_open_ext(&repo_ptr, + auto repo_ptr = std::make_shared(&cas->mutex_); + if (git_repository_open_ext(repo_ptr->PtrRef(), repo_path.c_str(), GIT_REPOSITORY_OPEN_NO_SEARCH, nullptr) != 0) { @@ -305,14 +335,12 @@ GitRepo::GitRepo(std::filesystem::path const& repo_path) noexcept { "opening git repository {} failed with:\n{}", repo_path.string(), GitLastError()); - git_repository_free(repo_ptr); - repo_ = nullptr; return; } - repo_.reset(repo_ptr); // retain repo pointer + repo_ = repo_ptr; // retain repo pointer // get odb git_odb* odb_ptr{nullptr}; - git_repository_odb(&odb_ptr, repo_.get()); + git_repository_odb(&odb_ptr, repo_->Ptr()); if (odb_ptr == nullptr) { Logger::Log(LogLevel::Error, "retrieving odb of git repository {} failed with:\n{}", @@ -326,7 +354,7 @@ GitRepo::GitRepo(std::filesystem::path const& repo_path) noexcept { is_repo_fake_ = false; // save root path cas->git_path_ = ToNormalPath(std::filesystem::absolute( - std::filesystem::path(git_repository_path(repo_.get())))); + std::filesystem::path(git_repository_path(repo_->Ptr())))); // retain the pointer git_cas_ = std::static_pointer_cast(cas); } catch (std::exception const& ex) { @@ -345,10 +373,13 @@ GitRepo::GitRepo(GitRepo&& other) noexcept } auto GitRepo::operator=(GitRepo&& other) noexcept -> GitRepo& { - git_cas_ = std::move(other.git_cas_); - repo_ = std::move(other.repo_); - is_repo_fake_ = other.is_repo_fake_; - other.git_cas_ = nullptr; + try { + git_cas_ = std::move(other.git_cas_); + repo_ = std::move(other.repo_); + is_repo_fake_ = other.is_repo_fake_; + other.git_cas_ = nullptr; + } catch (...) { + } return *this; } @@ -416,8 +447,7 @@ auto GitRepo::GetGitCAS() const noexcept -> GitCASPtr { return git_cas_; } -auto GitRepo::GetRepoRef() const noexcept - -> std::unique_ptr const& { +auto GitRepo::GetRepoRef() const noexcept -> GuardedRepoPtr { return repo_; } @@ -443,9 +473,12 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, true /*fatal*/); return std::nullopt; } + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + // add all files to be staged git_index* index_ptr{nullptr}; - git_repository_index(&index_ptr, repo_.get()); + git_repository_index(&index_ptr, repo_->Ptr()); auto index = std::unique_ptr( index_ptr, index_closer); @@ -496,7 +529,7 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, // get tree object git_tree* tree_ptr = nullptr; - if (git_tree_lookup(&tree_ptr, repo_.get(), &tree_oid) != 0) { + if (git_tree_lookup(&tree_ptr, repo_->Ptr(), &tree_oid) != 0) { (*logger)( fmt::format("tree lookup in git repository {} failed with:\n{}", GetGitCAS()->git_path_.string(), @@ -516,7 +549,7 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message, git_oid commit_oid; // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg,hicpp-vararg) if (git_commit_create_v(&commit_oid, - repo_.get(), + repo_->Ptr(), "HEAD", signature.get(), signature.get(), @@ -557,9 +590,12 @@ auto GitRepo::KeepTag(std::string const& commit, true /*fatal*/); return false; } + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + // get commit spec git_object* target_ptr{nullptr}; - if (git_revparse_single(&target_ptr, repo_.get(), commit.c_str()) != + if (git_revparse_single(&target_ptr, repo_->Ptr(), commit.c_str()) != 0) { (*logger)(fmt::format("rev-parse commit {} in repository {} failed " "with:\n{}", @@ -597,7 +633,7 @@ auto GitRepo::KeepTag(std::string const& commit, git_strarray tag_names{}; // check if tag hasn't already been added by another process - if (git_tag_list_match(&tag_names, name.c_str(), repo_.get()) == 0 and + if (git_tag_list_match(&tag_names, name.c_str(), repo_->Ptr()) == 0 and tag_names.count > 0) { git_strarray_dispose(&tag_names); return true; // success! @@ -610,7 +646,7 @@ auto GitRepo::KeepTag(std::string const& commit, while (max_attempts > 0) { --max_attempts; err = git_tag_create(&oid, - repo_.get(), + repo_->Ptr(), name.c_str(), target.get(), tagger.get(), @@ -625,7 +661,7 @@ auto GitRepo::KeepTag(std::string const& commit, break; } // check if tag hasn't already been added by another process - if (git_tag_list_match(&tag_names, name.c_str(), repo_.get()) == + if (git_tag_list_match(&tag_names, name.c_str(), repo_->Ptr()) == 0 and tag_names.count > 0) { git_strarray_dispose(&tag_names); @@ -662,9 +698,12 @@ auto GitRepo::GetHeadCommit(anon_logger_ptr const& logger) noexcept true /*fatal*/); return std::nullopt; } + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + // get root commit id git_oid head_oid; - if (git_reference_name_to_id(&head_oid, repo_.get(), "HEAD") != 0) { + if (git_reference_name_to_id(&head_oid, repo_->Ptr(), "HEAD") != 0) { (*logger)(fmt::format("retrieving head commit in git repository {} " "failed with:\n{}", GetGitCAS()->git_path_.string(), @@ -695,6 +734,9 @@ auto GitRepo::GetSubtreeFromCommit(std::string const& commit, LogLevel::Debug, "Subtree id retrieval from commit called on a real repository"); } + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + // get commit object git_oid commit_oid; if (git_oid_fromstr(&commit_oid, commit.c_str()) != 0) { @@ -708,7 +750,7 @@ auto GitRepo::GetSubtreeFromCommit(std::string const& commit, } git_commit* commit_ptr{nullptr}; - if (git_commit_lookup(&commit_ptr, repo_.get(), &commit_oid) != 0) { + if (git_commit_lookup(&commit_ptr, repo_->Ptr(), &commit_oid) != 0) { (*logger)(fmt::format("retrieving commit {} in git repository {} " "failed with:\n{}", commit, @@ -791,6 +833,9 @@ auto GitRepo::GetSubtreeFromTree(std::string const& tree_id, "Subtree id retrieval from tree called on a real " "repository"); } + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + // get tree object from tree id git_oid tree_oid; if (git_oid_fromstr(&tree_oid, tree_id.c_str()) != 0) { @@ -804,7 +849,7 @@ auto GitRepo::GetSubtreeFromTree(std::string const& tree_id, } git_tree* tree_ptr{nullptr}; - if (git_tree_lookup(&tree_ptr, repo_.get(), &tree_oid) != 0) { + if (git_tree_lookup(&tree_ptr, repo_->Ptr(), &tree_oid) != 0) { (*logger)(fmt::format( "retrieving tree {} in git repository {} failed " "with:\n{}", @@ -926,8 +971,13 @@ auto GitRepo::CheckCommitExists(std::string const& commit, } git_commit* commit_obj = nullptr; - auto lookup_res = - git_commit_lookup(&commit_obj, repo_.get(), &commit_oid); + int lookup_res{}; + { + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + lookup_res = + git_commit_lookup(&commit_obj, repo_->Ptr(), &commit_oid); + } if (lookup_res != 0) { if (lookup_res == GIT_ENOTFOUND) { // cleanup resources @@ -1027,7 +1077,12 @@ auto GitRepo::CheckTreeExists(std::string const& tree_id, } // get tree object git_tree* tree_ptr = nullptr; - auto lookup_res = git_tree_lookup(&tree_ptr, repo_.get(), &tree_oid); + int lookup_res{}; + { + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + lookup_res = git_tree_lookup(&tree_ptr, repo_->Ptr(), &tree_oid); + } git_tree_free(tree_ptr); if (lookup_res != 0) { if (lookup_res == GIT_ENOTFOUND) { @@ -1067,11 +1122,15 @@ auto GitRepo::ReadTree(std::string const& id, bool is_hex_id) const noexcept // lookup tree git_tree* tree_ptr{nullptr}; - if (git_tree_lookup(&tree_ptr, repo_.get(), &(*oid)) != 0) { - Logger::Log(LogLevel::Debug, - "failed to lookup Git tree {}", - is_hex_id ? std::string{id} : ToHexString(id)); - return std::nullopt; + { + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; + if (git_tree_lookup(&tree_ptr, repo_->Ptr(), &(*oid)) != 0) { + Logger::Log(LogLevel::Debug, + "failed to lookup Git tree {}", + is_hex_id ? std::string{id} : ToHexString(id)); + return std::nullopt; + } } auto tree = std::unique_ptr{tree_ptr, tree_closer}; @@ -1103,9 +1162,11 @@ auto GitRepo::CreateTree(tree_entries_t const& entries) const noexcept #ifndef NDEBUG gsl_ExpectsAudit(ValidateEntries(entries)); #endif // NDEBUG + // share the odb lock + std::shared_lock lock{GetGitCAS()->mutex_}; git_treebuilder* builder_ptr{nullptr}; - if (git_treebuilder_new(&builder_ptr, repo_.get(), nullptr) != 0) { + if (git_treebuilder_new(&builder_ptr, repo_->Ptr(), nullptr) != 0) { Logger::Log(LogLevel::Debug, "failed to create Git tree builder"); return std::nullopt; } -- cgit v1.2.3