From d267539801ce0a32392d3a86d94e6ea37b6dc2ba Mon Sep 17 00:00:00 2001
From: Jaroslav Rohel <jrohel@redhat.com>
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 <jrohel@redhat.com>
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<std::string> 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 <jrohel@redhat.com>
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 <cctype>
#include <map>
+#include <mutex>
#include <set>
#include <string.h>
@@ -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<LrResult> 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 <iostream>
#include <list>
#include <map>
-#include <mutex>
#include <set>
#include <sstream>
#include <type_traits>
@@ -1335,8 +1334,10 @@ LrHandle * Repo::Impl::getCachedHandle()
void Repo::Impl::attachLibsolvRepo(LibsolvRepo * libsolvRepo)
{
+ std::lock_guard<std::mutex> 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<std::mutex> 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 <jrohel@redhat.com>
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<HyRepo>(repo->appdata);
+ if (!repo)
+ return;
+
+ if (auto hrepo = static_cast<HyRepo>(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 <jrohel@redhat.com>
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<std::mutex> 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();