From 81417e1790dee778c998b8591fcb1e28393ab6cf Mon Sep 17 00:00:00 2001
From: Florian Festi <ffesti@redhat.com>
Date: Thu, 10 Oct 2024 16:58:25 +0200
Subject: [PATCH] rpmKeyring and rpmstrPool nrefs to atomic_int

These data structures use the content lock (aka mutex) to also protect the
reference counter. This is wrong and complicates freeing interconnected
data structures. This lock should only protect the content of the
instance.

Turn nrefs members into atomic_int so they are thread-save themselves.

Make sure to only use ++ and -- operators and avoid non atomic
combinations of reading and writing.  As the nref can't change again
after reaching zero this is thread-save even as we look at the result
an instruction later.

Make sure the data lock is not aquired for updating the nrefs members.

Technically the *Free() functions should *Link() the instance after the
ref count has reached 0 and the decision to free it has been made.
Otherwise the cleanup code linking and freeing to will lead to a
recusions. This is not neccessary here so it is omitted.
---
 rpmio/rpmkeyring.cc | 41 +++++++++++++++--------------------------
 rpmio/rpmstrpool.cc | 31 ++++++++++++++-----------------
 2 files changed, 29 insertions(+), 43 deletions(-)

diff --git a/rpmio/rpmkeyring.cc b/rpmio/rpmkeyring.cc
index bb2ac34323..6d8105c0ea 100644
--- a/rpmio/rpmkeyring.cc
+++ b/rpmio/rpmkeyring.cc
@@ -1,6 +1,7 @@
 #include "system.h"
 
 #include <mutex>
+#include <atomic>
 #include <vector>
 #include <shared_mutex>
 #include <string>
@@ -22,7 +23,7 @@ struct rpmPubkey_s {
     std::string keyid;
     rpmPubkey primarykey;
     pgpDigParams pgpkey;
-    int nrefs;
+    std::atomic_int nrefs;
     std::shared_mutex mutex;
 };
 
@@ -31,7 +32,7 @@ using rdlock = std::shared_lock<std::shared_mutex>;
 
 struct rpmKeyring_s {
     std::map<std::string,rpmPubkey> keys; /* pointers to keys */
-    int nrefs;
+    std::atomic_int nrefs;
     std::shared_mutex mutex;
 };
 
@@ -56,15 +57,13 @@ rpmKeyring rpmKeyringNew(void)
 
 rpmKeyring rpmKeyringFree(rpmKeyring keyring)
 {
-    if (keyring == NULL)
+    if (keyring == NULL || --keyring->nrefs > 0)
 	return NULL;
 
-    wrlock lock(keyring->mutex);
-    if (--keyring->nrefs == 0) {
-	for (auto & item : keyring->keys)
-	    rpmPubkeyFree(item.second);
-	delete keyring;
-    }
+    for (auto & item : keyring->keys)
+	rpmPubkeyFree(item.second);
+    delete keyring;
+
     return NULL;
 }
 
@@ -159,7 +158,6 @@ rpmPubkey rpmKeyringLookupKey(rpmKeyring keyring, rpmPubkey key)
 rpmKeyring rpmKeyringLink(rpmKeyring keyring)
 {
     if (keyring) {
-	wrlock lock(keyring->mutex);
 	keyring->nrefs++;
     }
     return keyring;
@@ -219,12 +217,6 @@ static rpmPubkey rpmPubkeyNewSubkey(rpmPubkey primarykey, pgpDigParams pgpkey)
     return key;
 }
 
-static rpmPubkey pubkeyLink(rpmPubkey key)
-{
-    key->nrefs++;
-    return key;
-}
-
 rpmPubkey *rpmGetSubkeys(rpmPubkey primarykey, int *count)
 {
     rpmPubkey *subkeys = NULL;
@@ -243,7 +235,7 @@ rpmPubkey *rpmGetSubkeys(rpmPubkey primarykey, int *count)
 	    subkeys = (rpmPubkey *)xmalloc(pgpsubkeysCount * sizeof(*subkeys));
 	    for (i = 0; i < pgpsubkeysCount; i++) {
 		subkeys[i] = rpmPubkeyNewSubkey(primarykey, pgpsubkeys[i]);
-		primarykey = pubkeyLink(primarykey);
+		primarykey = rpmPubkeyLink(primarykey);
 	    }
 	    free(pgpsubkeys);
 	}
@@ -259,7 +251,7 @@ rpmPubkey pubkeyPrimarykey(rpmPubkey key)
     if (key) {
 	wrlock lock(key->mutex);
 	if (key->primarykey == NULL) {
-	    primarykey = pubkeyLink(key);
+	    primarykey = rpmPubkeyLink(key);
 	} else {
 	    primarykey = rpmPubkeyLink(key->primarykey);
 	}
@@ -301,22 +293,19 @@ char * rpmPubkeyKeyIDAsHex(rpmPubkey key)
 
 rpmPubkey rpmPubkeyFree(rpmPubkey key)
 {
-    if (key == NULL)
+    if (key == NULL || --key->nrefs > 0)
 	return NULL;
 
-    wrlock lock(key->mutex);
-    if (--key->nrefs == 0) {
-	pgpDigParamsFree(key->pgpkey);
-	rpmPubkeyFree(key->primarykey);
-	delete key;
-    }
+    pgpDigParamsFree(key->pgpkey);
+    rpmPubkeyFree(key->primarykey);
+    delete key;
+
     return NULL;
 }
 
 rpmPubkey rpmPubkeyLink(rpmPubkey key)
 {
     if (key) {
-	wrlock lock(key->mutex);
 	key->nrefs++;
     }
     return key;
diff --git a/rpmio/rpmstrpool.cc b/rpmio/rpmstrpool.cc
index 8044f933e3..7fd2430f7c 100644
--- a/rpmio/rpmstrpool.cc
+++ b/rpmio/rpmstrpool.cc
@@ -2,6 +2,7 @@
 
 #include <shared_mutex>
 #include <mutex>
+#include <atomic>
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -43,7 +44,7 @@ struct rpmstrPool_s {
 
     poolHash hash;		/* string -> sid hash table */
     int frozen;			/* are new id additions allowed? */
-    int nrefs;			/* refcount */
+    std::atomic_int nrefs;	/* refcount */
     std::shared_mutex mutex;
 };
 
@@ -256,29 +257,25 @@ rpmstrPool rpmstrPoolCreate(void)
 
 rpmstrPool rpmstrPoolFree(rpmstrPool pool)
 {
-    if (pool) {
-	wrlock lock(pool->mutex);
-	if (pool->nrefs > 1) {
-	    pool->nrefs--;
-	} else {
-	    if (pool_debug)
-		poolHashPrintStats(pool);
-	    poolHashFree(pool->hash);
-	    free(pool->offs);
-	    for (int i=1;i<=pool->chunks_size;i++) {
-		pool->chunks[i] = _free(pool->chunks[i]);
-	    }
-	    free(pool->chunks);
-	    free(pool);
-	}
+    if (pool == NULL || --pool->nrefs > 0)
+	return NULL;
+
+    if (pool_debug)
+	poolHashPrintStats(pool);
+    poolHashFree(pool->hash);
+    free(pool->offs);
+    for (int i=1; i<=pool->chunks_size; i++) {
+	pool->chunks[i] = _free(pool->chunks[i]);
     }
+    free(pool->chunks);
+    free(pool);
+
     return NULL;
 }
 
 rpmstrPool rpmstrPoolLink(rpmstrPool pool)
 {
     if (pool) {
-	wrlock lock(pool->mutex);
 	pool->nrefs++;
     }
     return pool;