Turn nrefs into atomic_int

Throughout the code base reference counting is done in a thread-unsave
way. A few places 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 all 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. Drop all the static *Unlink
functions that are involved in this.

Make sure the data lock is not aquired while 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. Right now only rpmts needs that. SO it is omitted everywhere
else.

FD_t is special in that fdFree return the instance after free iff it
still has not reached a ref count of 0. Unfortunately code in
rpmShowProgress() relies on that.

This does not make the data structures thread save on its own. But it
gives a foundation on which data locking can be implemented.

lib/backend/dbi.hh

@@ -4,6 +4,7 @@
 #include <unordered_map>
 
 #include <stdio.h>
+#include <atomic>
 #include "dbiset.hh"
 #include <rpm/rpmtag.h>
 
@@ -68,7 +69,7 @@ struct rpmdb_s {
     struct rpmop_s db_putops;
     struct rpmop_s db_delops;
 
-    int nrefs;			/*!< Reference count. */
+    std::atomic_int nrefs;	/*!< Reference count. */
 };
 
 /* Type of the dbi, also serves as the join key size */

lib/header.cc

@@ -12,6 +12,7 @@
 #include <netdb.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <atomic>
 #include <rpm/rpmtypes.h>
 #include <rpm/rpmstring.h>
 #include "header_internal.hh"
@@ -109,7 +110,7 @@ struct headerToken_s {
     unsigned int instance;	/*!< Rpmdb instance */
     headerFlags flags;
     int sorted;			/*!< Current sort method */
-    int nrefs;			/*!< Reference count. */
+    std::atomic_int nrefs;			/*!< Reference count. */
 };
 
 /** \ingroup header
@@ -239,18 +240,9 @@ Header headerLink(Header h)
     return h;
 }
 
-static Header headerUnlink(Header h)
-{
-    if (h != NULL)
-	h->nrefs--;
-    return NULL;
-}
-
 Header headerFree(Header h)
 {
-    (void) headerUnlink(h);
-
-    if (h == NULL || h->nrefs > 0)
+    if (h == NULL || --h->nrefs > 0)
 	return NULL;
 
     if (h->index) {

lib/psm.cc

@@ -6,6 +6,7 @@
 #include "system.h"
 
 #include <errno.h>
+#include <atomic>
 
 #include <rpm/rpmlib.h>		/* rpmvercmp and others */
 #include <rpm/rpmmacro.h>
@@ -43,7 +44,7 @@ struct rpmpsm_s {
     rpm_loff_t amount;		/*!< Callback amount. */
     rpm_loff_t total;		/*!< Callback total. */
 
-    int nrefs;			/*!< Reference count. */
+    std::atomic_int nrefs;	/*!< Reference count. */
 };
 
 static rpmpsm rpmpsmNew(rpmts ts, rpmte te, pkgGoal goal);

lib/rpmdb.cc

@@ -43,8 +43,6 @@
 using std::unordered_map;
 using std::vector;
 
-static rpmdb rpmdbUnlink(rpmdb db);
-
 static int buildIndexes(rpmdb db)
 {
     int rc = 0;
@@ -346,12 +344,7 @@ int rpmdbClose(rpmdb db)
 {
     int rc = 0;
 
-    if (db == NULL)
-	goto exit;
-
-    (void) rpmdbUnlink(db);
-
-    if (db->nrefs > 0)
+    if (db == NULL || --db->nrefs > 0)
 	goto exit;
 
     /* Always re-enable fsync on close of rw-database */
@@ -462,13 +455,6 @@ static int openDatabase(const char * prefix,
     return rc;
 }
 
-static rpmdb rpmdbUnlink(rpmdb db)
-{
-    if (db)
-	db->nrefs--;
-    return NULL;
-}
-
 rpmdb rpmdbLink(rpmdb db)
 {
     if (db)

lib/rpmds.cc

@@ -2,6 +2,7 @@
  * \file lib/rpmds.c
  */
 #include "system.h"
+#include <atomic>
 
 #include <rpm/rpmtypes.h>
 #include <rpm/rpmlib.h>		/* rpmvercmp */
@@ -29,7 +30,7 @@ struct rpmds_s {
     int32_t Count;		/*!< No. of elements */
     unsigned int instance;	/*!< From rpmdb instance? */
     int i;			/*!< Element index. */
-    int nrefs;			/*!< Reference count. */
+    std::atomic_int nrefs;	/*!< Reference count. */
     int *ti;			/*!< Trigger index. */
 };
 
@@ -202,12 +203,6 @@ rpm_color_t rpmdsColorIndex(rpmds ds, int i)
 	Color = ds->Color[i];
     return Color;
 }
-static rpmds rpmdsUnlink(rpmds ds)
-{
-    if (ds)
-	ds->nrefs--;
-    return NULL;
-}
 
 rpmds rpmdsLink(rpmds ds)
 {
@@ -220,12 +215,9 @@ rpmds rpmdsFree(rpmds ds)
 {
     rpmTagVal tagEVR, tagF, tagTi;
 
-    if (ds == NULL)
+    if (ds == NULL || --ds->nrefs > 0)
 	return NULL;
 
-    if (ds->nrefs > 1)
-	return rpmdsUnlink(ds);
-
     if (dsType(ds->tagN, NULL, &tagEVR, &tagF, &tagTi))
 	return NULL;
 
@@ -240,7 +232,6 @@ rpmds rpmdsFree(rpmds ds)
     ds->DNEVR = _free(ds->DNEVR);
     ds->Color = _free(ds->Color);
 
-    (void) rpmdsUnlink(ds);
     delete ds;
     return NULL;
 }

lib/rpmfi.cc

@@ -8,6 +8,7 @@
 #include <unordered_map>
 #include <vector>
 #include <memory>
+#include <atomic>
 
 #include <rpm/rpmlog.h>
 #include <rpm/rpmts.h>
@@ -50,7 +51,7 @@ struct rpmfi_s {
     rpmfiles files;		/*!< File info set */
     rpmcpio_t archive;		/*!< Archive with payload */
     uint8_t * found;	/*!< Bit field of files found in the archive */
-    int nrefs;			/*!< Reference count */
+    std::atomic_int nrefs;	/*!< Reference count */
 };
 
 struct rpmfn_s {
@@ -122,33 +123,19 @@ struct rpmfiles_s {
     nlinkHash *nlinks;		/*!< Files connected by hardlinks */
     rpm_loff_t * replacedSizes;	/*!< (TR_ADDED) */
     int magic;
-    int nrefs;		/*!< Reference count. */
+    std::atomic_int nrefs;	/*!< Reference count. */
 };
 
 static int indexSane(rpmtd xd, rpmtd yd, rpmtd zd);
 static int cmpPoolFn(rpmstrPool pool, rpmfn files, int ix, const char * fn);
 
-static rpmfiles rpmfilesUnlink(rpmfiles fi)
-{
-    if (fi)
-	fi->nrefs--;
-    return NULL;
-}
-
 rpmfiles rpmfilesLink(rpmfiles fi)
 {
     if (fi)
 	fi->nrefs++;
     return fi;
 }
 
-static rpmfi rpmfiUnlink(rpmfi fi)
-{
-    if (fi)
-	fi->nrefs--;
-    return NULL;
-}
-
 rpmfi rpmfiLink(rpmfi fi)
 {
     if (fi)
@@ -1240,10 +1227,8 @@ int rpmfilesConfigConflict(rpmfiles fi, int ix)
 
 rpmfiles rpmfilesFree(rpmfiles fi)
 {
-    if (fi == NULL) return NULL;
-
-    if (fi->nrefs > 1)
-	return rpmfilesUnlink(fi);
+    if (fi == NULL || --fi->nrefs > 0)
+	return NULL;
 
     if (rpmfilesFC(fi) > 0) {
 	if (fi->ofndata != &fi->fndata) {
@@ -1297,18 +1282,15 @@ rpmfiles rpmfilesFree(rpmfiles fi)
 
     delete fi->nlinks;
 
-    (void) rpmfilesUnlink(fi);
     delete fi;
 
     return NULL;
 }
 
 rpmfi rpmfiFree(rpmfi fi)
 {
-    if (fi == NULL) return NULL;
-
-    if (fi->nrefs > 1)
-	return rpmfiUnlink(fi);
+    if (fi == NULL || --fi->nrefs > 0)
+	return NULL;
 
     fi->files = rpmfilesFree(fi->files);
     fi->fn = _free(fi->fn);

lib/rpmprob.cc

@@ -6,6 +6,7 @@
 
 #include <inttypes.h>
 #include <stdlib.h>
+#include <atomic>
 
 #include <rpm/rpmstring.h>
 #include <rpm/rpmprob.h>
@@ -19,11 +20,9 @@ struct rpmProblem_s {
     rpmProblemType type;
     char * str1;
     uint64_t num1;
-    int nrefs;
+    std::atomic_int nrefs;
 };
 
-static rpmProblem rpmProblemUnlink(rpmProblem prob);
-
 rpmProblem rpmProblemCreate(rpmProblemType type,
                             const char * pkgNEVR, fnpyKey key,
                             const char * altNEVR,
@@ -44,11 +43,9 @@ rpmProblem rpmProblemCreate(rpmProblemType type,
 
 rpmProblem rpmProblemFree(rpmProblem prob)
 {
-    if (prob == NULL) return NULL;
+    if (prob == NULL || --prob->nrefs > 0)
+	return NULL;
 
-    if (prob->nrefs > 1) {
-	return rpmProblemUnlink(prob);
-    }
     prob->pkgNEVR = _free(prob->pkgNEVR);
     prob->altNEVR = _free(prob->altNEVR);
     prob->str1 = _free(prob->str1);
@@ -64,14 +61,6 @@ rpmProblem rpmProblemLink(rpmProblem prob)
     return prob;
 }
 
-static rpmProblem rpmProblemUnlink(rpmProblem prob)
-{
-    if (prob) {
-	prob->nrefs--;
-    }
-    return NULL;
-}
-
 const char * rpmProblemGetPkgNEVR(rpmProblem p)
 {
     return (p->pkgNEVR);

lib/rpmps.cc

@@ -5,7 +5,7 @@
 #include "system.h"
 
 #include <vector>
-
+#include <atomic>
 #include <inttypes.h>
 #include <stdlib.h>
 
@@ -18,7 +18,7 @@ using std::vector;
 
 struct rpmps_s {
     vector<rpmProblem> probs;	/*!< Array of pointers to specific problems. */
-    int nrefs;			/*!< Reference count. */
+    std::atomic_int nrefs;	/*!< Reference count. */
 };
 
 struct rpmpsi_s {
@@ -27,14 +27,6 @@ struct rpmpsi_s {
 };
 
 
-static rpmps rpmpsUnlink(rpmps ps)
-{
-    if (ps) {
-	ps->nrefs--;
-    }
-    return NULL;
-}
-
 rpmps rpmpsLink(rpmps ps)
 {
     if (ps) {
@@ -65,7 +57,7 @@ rpmpsi rpmpsInitIterator(rpmps ps)
 rpmpsi rpmpsFreeIterator(rpmpsi psi)
 {
     if (psi != NULL) {
-	rpmpsUnlink(psi->ps);
+	rpmpsFree(psi->ps);
 	delete psi;
     }
     return NULL;
@@ -107,10 +99,8 @@ rpmps rpmpsCreate(void)
 
 rpmps rpmpsFree(rpmps ps)
 {
-    if (ps == NULL) return NULL;
-    if (ps->nrefs > 1) {
-	return rpmpsUnlink(ps);
-    }
+    if (ps == NULL || --ps->nrefs > 0)
+	return NULL;
 
     for (auto & prob : ps->probs)
 	rpmProblemFree(prob);

lib/rpmts.cc

@@ -60,13 +60,6 @@ static void loadKeyring(rpmts ts);
 
 int _rpmts_stats = 0;
 
-static rpmts rpmtsUnlink(rpmts ts)
-{
-    if (ts)
-	ts->nrefs--;
-    return NULL;
-}
-
 rpmts rpmtsLink(rpmts ts)
 {
     if (ts)
@@ -951,11 +944,11 @@ static void rpmtsPrintStats(rpmts ts)
 
 rpmts rpmtsFree(rpmts ts)
 {
-    if (ts == NULL)
+    if (ts == NULL || (--(ts->nrefs)) > 0)
 	return NULL;
 
-    if (ts->nrefs > 1)
-	return rpmtsUnlink(ts);
+    /* Cleanup still needs to rpmtsLink() and rpmtsFree() */
+    ts  = rpmtsLink(ts);
 
     /* Don't issue element change callbacks when freeing */
     rpmtsSetChangeCallback(ts, NULL, NULL);
@@ -984,7 +977,6 @@ rpmts rpmtsFree(rpmts ts)
     if (_rpmts_stats)
 	rpmtsPrintStats(ts);
 
-    (void) rpmtsUnlink(ts);
     delete ts;
 
     return NULL;
@@ -1403,7 +1395,6 @@ rpm_time_t rpmtsGetTime(rpmts ts, time_t step)
 
 rpmtsi rpmtsiFree(rpmtsi tsi)
 {
-    /* XXX watchout: a funky recursion segfaults here iff nrefs is wrong. */
     if (tsi) {
 	tsi->ts = rpmtsFree(tsi->ts);
 	delete tsi;