From d267539801ce0a32392d3a86d94e6ea37b6dc2ba Mon Sep 17 00:00:00 2001 From: Jaroslav Rohel Date: Fri, 12 Jul 2019 06:51:25 +0200 Subject: [PATCH 1/5] [context] Fix: Correctly detach libsolv repo (RhBug:1727343) Seting repoImpl->libsolvRepo = nullptr and repoImpl->nrefs = 1 is not sufficient. The libsolvRepo inernally points back to us. And during destroying it destroy us too because nrefs was set to 1. Solution is to do full detach using detachLibsolvRepo(). It fixes https://bugzilla.redhat.com/show_bug.cgi?id=1727343 and probably https://bugzilla.redhat.com/show_bug.cgi?id=1727424 --- libdnf/dnf-repo.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libdnf/dnf-repo.cpp b/libdnf/dnf-repo.cpp index a358356a9..45c6c7e5b 100644 --- a/libdnf/dnf-repo.cpp +++ b/libdnf/dnf-repo.cpp @@ -1403,9 +1403,9 @@ dnf_repo_check_internal(DnfRepo *repo, } /* init */ - repoImpl->libsolvRepo = nullptr; + if (repoImpl->libsolvRepo) + repoImpl->detachLibsolvRepo(); repoImpl->needs_internalizing = false; - repoImpl->nrefs = 1; repoImpl->state_main = _HY_NEW; repoImpl->state_filelists = _HY_NEW; repoImpl->state_presto = _HY_NEW; From f1e2e534d7d375d051c4eae51431c5bb3649f9f1 Mon Sep 17 00:00:00 2001 From: Jaroslav Rohel Date: Fri, 12 Jul 2019 07:59:15 +0200 Subject: [PATCH 2/5] [Repo] Remove unused delReference --- libdnf/repo/Repo.cpp | 7 ------- libdnf/repo/Repo.hpp | 2 -- 2 files changed, 9 deletions(-) diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp index 23638839a..fd2415fa5 100644 --- a/libdnf/repo/Repo.cpp +++ b/libdnf/repo/Repo.cpp @@ -1441,13 +1441,6 @@ std::vector Repo::getMirrors() const return mirrors; } -void Repo::delReference() -{ - if (--pImpl->nrefs > 0) - return; - delete this; -} - int PackageTargetCB::end(TransferStatus status, const char * msg) { return 0; } int PackageTargetCB::progress(double totalToDownload, double downloaded) { return 0; } int PackageTargetCB::mirrorFailure(const char *msg, const char *url) { return 0; } diff --git a/libdnf/repo/Repo.hpp b/libdnf/repo/Repo.hpp index cbf4ed147..785c6e9d5 100644 --- a/libdnf/repo/Repo.hpp +++ b/libdnf/repo/Repo.hpp @@ -275,8 +275,6 @@ struct Repo { ~Repo(); - void delReference(); - class Impl; private: friend struct PackageTarget; From 4c35d135bc79939d58844e99e0d5ed924ba86fd5 Mon Sep 17 00:00:00 2001 From: Jaroslav Rohel Date: Fri, 12 Jul 2019 08:50:54 +0200 Subject: [PATCH 3/5] [Repo] Add locking and asserts Add locking and asserts into attachLibsolvRepo(), detachLibsolvRepo() and hy_repo_free() --- libdnf/dnf-repo.cpp | 3 +-- libdnf/repo/Repo-private.hpp | 5 +++++ libdnf/repo/Repo.cpp | 34 ++++++++++++++++++++++++++-------- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/libdnf/dnf-repo.cpp b/libdnf/dnf-repo.cpp index 45c6c7e5b..c30d99d17 100644 --- a/libdnf/dnf-repo.cpp +++ b/libdnf/dnf-repo.cpp @@ -1403,8 +1403,7 @@ dnf_repo_check_internal(DnfRepo *repo, } /* init */ - if (repoImpl->libsolvRepo) - repoImpl->detachLibsolvRepo(); + repoImpl->detachLibsolvRepo(); repoImpl->needs_internalizing = false; repoImpl->state_main = _HY_NEW; repoImpl->state_filelists = _HY_NEW; diff --git a/libdnf/repo/Repo-private.hpp b/libdnf/repo/Repo-private.hpp index 23acf3622..333eb7bfd 100644 --- a/libdnf/repo/Repo-private.hpp +++ b/libdnf/repo/Repo-private.hpp @@ -43,6 +43,7 @@ #include #include +#include #include #include @@ -177,6 +178,10 @@ class Repo::Impl { int main_nrepodata{0}; int main_end{0}; + // Lock attachLibsolvRepo(), detachLibsolvRepo() and hy_repo_free() to ensure atomic behavior + // in threaded environment such as PackageKit. + std::mutex attachLibsolvMutex; + private: Repo * owner; std::unique_ptr lrHandlePerform(LrHandle * handle, const std::string & destDirectory, diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp index fd2415fa5..86531fe9b 100644 --- a/libdnf/repo/Repo.cpp +++ b/libdnf/repo/Repo.cpp @@ -60,7 +60,6 @@ #include #include #include -#include #include #include #include @@ -1335,8 +1334,10 @@ LrHandle * Repo::Impl::getCachedHandle() void Repo::Impl::attachLibsolvRepo(LibsolvRepo * libsolvRepo) { + std::lock_guard guard(attachLibsolvMutex); + assert(!this->libsolvRepo); ++nrefs; - libsolvRepo->appdata = owner; + libsolvRepo->appdata = owner; // The libsolvRepo references back to us. libsolvRepo->subpriority = -owner->getCost(); libsolvRepo->priority = -owner->getPriority(); this->libsolvRepo = libsolvRepo; @@ -1344,11 +1345,23 @@ void Repo::Impl::attachLibsolvRepo(LibsolvRepo * libsolvRepo) void Repo::Impl::detachLibsolvRepo() { - libsolvRepo->appdata = nullptr; - if (--nrefs > 0) - this->libsolvRepo = nullptr; - else + attachLibsolvMutex.lock(); + if (!libsolvRepo) { + // Nothing to do, libsolvRepo is not attached. + attachLibsolvMutex.unlock(); + return; + } + + libsolvRepo->appdata = nullptr; // Removes reference to this object from libsolvRepo. + this->libsolvRepo = nullptr; + + if (--nrefs <= 0) { + // There is no reference to this object, we are going to destroy it. + // Mutex is part of this object, we must unlock it before destroying. + attachLibsolvMutex.unlock(); delete owner; + } else + attachLibsolvMutex.unlock(); } void Repo::setMaxMirrorTries(int maxMirrorTries) @@ -2057,7 +2070,12 @@ hy_repo_get_string(HyRepo repo, int which) void hy_repo_free(HyRepo repo) { - if (--libdnf::repoGetImpl(repo)->nrefs > 0) - return; + auto repoImpl = libdnf::repoGetImpl(repo); + { + std::lock_guard guard(repoImpl->attachLibsolvMutex); + if (--repoImpl->nrefs > 0) + return; // There is still a reference to this object. Don't destroy it. + } + assert(!repoImpl->libsolvRepo); delete repo; } From 3a61d4c590612427bfeb7302236e4429acae90b0 Mon Sep 17 00:00:00 2001 From: Jaroslav Rohel Date: Fri, 12 Jul 2019 11:21:48 +0200 Subject: [PATCH 4/5] Fix: crash in repo_internalize_trigger() without HyRepo attached The repo_internalize_trigger() uses needs_internalizing from HyRepo. If HyRepo is not attached we will assume needs_internalizing==true. --- libdnf/repo/Repo.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp index 86531fe9b..210ecde8f 100644 --- a/libdnf/repo/Repo.cpp +++ b/libdnf/repo/Repo.cpp @@ -1826,15 +1826,19 @@ repo_internalize_all_trigger(Pool *pool) void repo_internalize_trigger(Repo * repo) { - if (repo) { - auto hrepo = static_cast(repo->appdata); + if (!repo) + return; + + if (auto hrepo = static_cast(repo->appdata)) { + // HyRepo is attached. The hint needs_internalizing will be used. auto repoImpl = libdnf::repoGetImpl(hrepo); assert(repoImpl->libsolvRepo == repo); if (!repoImpl->needs_internalizing) return; repoImpl->needs_internalizing = false; - repo_internalize(repo); } + + repo_internalize(repo); } void From e4cabf803e1dbb6283330636d06ccb4a26e89ad4 Mon Sep 17 00:00:00 2001 From: Jaroslav Rohel Date: Sun, 14 Jul 2019 10:45:27 +0200 Subject: [PATCH 5/5] [Repo] attachLibsolvRepo() can reattach repo to another libsolvRepo --- libdnf/repo/Repo.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/libdnf/repo/Repo.cpp b/libdnf/repo/Repo.cpp index 210ecde8f..70c6a7411 100644 --- a/libdnf/repo/Repo.cpp +++ b/libdnf/repo/Repo.cpp @@ -1335,8 +1335,14 @@ LrHandle * Repo::Impl::getCachedHandle() void Repo::Impl::attachLibsolvRepo(LibsolvRepo * libsolvRepo) { std::lock_guard guard(attachLibsolvMutex); - assert(!this->libsolvRepo); - ++nrefs; + + if (this->libsolvRepo) + // A libsolvRepo was attached to this object before. Remove it's reference to this object. + this->libsolvRepo->appdata = nullptr; + else + // The libsolvRepo will reference this object. Increase reference counter. + ++nrefs; + libsolvRepo->appdata = owner; // The libsolvRepo references back to us. libsolvRepo->subpriority = -owner->getCost(); libsolvRepo->priority = -owner->getPriority();