Skip to content

Commit

Permalink
libpriv/rpm-util: Query package repo checksum under lock
Browse files Browse the repository at this point in the history
In general, libsolv isn't thread-safe.[[1]] There are multiple
functions which return pointers that can be invalidated by other
threads. It's hard to follow the libsolv side of this, but I think
`solvable_lookup_bin_checksum()` (called by `dnf_package_get_chksum()`)
is such a case. Because we do importing in bulk using multi-threading,
rpm-ostree triggers this bug.

This is the root cause of #4565, where during the `cosa fetch` we embed
the wrong repodata chksum_repr in the cached RPM's OSTree commit. So
then `cosa build` considers it a cache miss when it sees the mismatch
and wants to redownload it.

A trivial way to reproduce #4565 more easily is with:

```diff
diff --git a/src/libpriv/rpmostree-rpm-util.cxx b/src/libpriv/rpmostree-rpm-util.cxx
index 658a5fd8..ad8dc104 100644
--- a/src/libpriv/rpmostree-rpm-util.cxx
+++ b/src/libpriv/rpmostree-rpm-util.cxx
@@ -1142,6 +1142,7 @@ get_repodata_chksum_repr (DnfPackage &pkg)
     chksum_raw = dnf_package_get_hdr_chksum (&pkg, &chksum_type);
   else
     chksum_raw = dnf_package_get_chksum (&pkg, &chksum_type);
+  g_usleep (5000);
   if (chksum_raw)
     chksum = hy_chksum_str (chksum_raw, chksum_type);
```

Fix this by locking down this critical section. As mentioned in the
comment, a more correct fix would be to not call unsafe libsolv APIs in
the importer, but that'd require a larger rework. The locking added here
doesn't appear to affect performance.

I've checked that we don't do any other unsafe libsolv calls in the
importer path that could be subject to this.

Fixes: #4565

[1]: openSUSE/libsolv#471
  • Loading branch information
jlebon authored and cgwalters committed Oct 26, 2023
1 parent 8c83311 commit 7e85d58
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/libpriv/rpmostree-rpm-util.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -1133,17 +1133,24 @@ namespace rpmostreecxx
rust::String
get_repodata_chksum_repr (DnfPackage &pkg)
{
static GMutex mutex;
int chksum_type;
g_autofree char *chksum = NULL;
const unsigned char *chksum_raw = NULL;

/* Have to do this under lock since chksum_raw can be invalidated by another
* thread; the right fix for this is to not use libsolv in a multi-threaded
* context, but that's a larger refactor.
* See https://github.com/coreos/rpm-ostree/issues/4565. */
g_mutex_lock (&mutex);
/* for rpmdb packages, use the hdr checksum */
if (dnf_package_installed (&pkg))
chksum_raw = dnf_package_get_hdr_chksum (&pkg, &chksum_type);
else
chksum_raw = dnf_package_get_chksum (&pkg, &chksum_type);
if (chksum_raw)
chksum = hy_chksum_str (chksum_raw, chksum_type);
g_mutex_unlock (&mutex);

if (chksum == NULL)
{
Expand Down

0 comments on commit 7e85d58

Please sign in to comment.