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

@@ -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;

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;