From 0ff7d92c61e40f5f6c33fbff27b7011fc2833e8f Mon Sep 17 00:00:00 2001 From: Pavla Kratochvilova Date: Jul 22 2019 08:30:16 +0000 Subject: Fix attaching/detaching of libsolvRepo (RhBug:1727343,1727424) --- diff --git a/0003-Fix-attaching-and-detaching-of-libsolvRepo.patch b/0003-Fix-attaching-and-detaching-of-libsolvRepo.patch new file mode 100644 index 0000000..29e542a --- /dev/null +++ b/0003-Fix-attaching-and-detaching-of-libsolvRepo.patch @@ -0,0 +1,265 @@ +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(); diff --git a/libdnf.spec b/libdnf.spec index 23d2fce..5d718a2 100644 --- a/libdnf.spec +++ b/libdnf.spec @@ -38,7 +38,7 @@ Name: libdnf Version: 0.35.1 -Release: 1%{?dist} +Release: 2%{?dist} Summary: Library providing simplified C and Python API to libsolv License: LGPLv2+ URL: https://github.com/rpm-software-management/libdnf @@ -47,6 +47,9 @@ Patch0001: 0001-Revert-9309e92332241ff1113433057c969cebf127734e.patch # Temporary patch to not fail on modular RPMs without modular metadata # until the infrastructure is ready Patch0002: 0002-Revert-consequences-of-Fail-Safe-mechanism.patch +# https://bugzilla.redhat.com/show_bug.cgi?id=1727343 +# https://bugzilla.redhat.com/show_bug.cgi?id=1727424 +Patch0003: 0003-Fix-attaching-and-detaching-of-libsolvRepo.patch BuildRequires: cmake BuildRequires: gcc @@ -257,6 +260,10 @@ popd %endif %changelog +* Mon Jul 22 2019 Pavla Kratochvilova - 0.35.1-2 +- Backport patch to fix attaching and detaching of libsolvRepo and + repo_internalize_trigger() (RhBug:1727343,1727424) + * Thu Jul 04 2019 Pavla Kratochvilova - 0.35.1-1 - Update to 0.35.1 - Enhance logging handling