Stop leaking PeerDigests on reconfiguration (#1371)

peerDigestCreate() cbdata-locked the digest twice. The second lock was a
"self lock" -- a lock without storing the locked pointer somewhere. That
self lock was introduced (with an XXX) in 2002 commit fa80a8e. It did
not have the corresponding unlock, and Squid leaked the digest object.
That leak became conditional in 2018 commit b56b37c: Since then, Squid
was missing one digest unlock only when the CachePeer object was gone
before a peerDigestDestroy() call, as happens during reconfiguration.

Also removed a pair of excessive cbdata locks/unlocks (that triggered
this leak): Long-term users should lock when they start storing a
pointer to the cbdata-protected object and unlock when they stop.

Also converted PeerDigest::peer to CbcPointer, addressing a TODO.

Also fixed subtle problems with detecting gone CachePeer and gone
PeerDigest objects in peerDigestFetchedEnough().

Also removed stale peerNoteDigestGone() declaration.

diff --git a/src/PeerDigest.h b/src/PeerDigest.h
index 2864e35fdf82c472743a961ddffb715b2f27f0cf..f76923db3972a331b0af2bff491c9fbb6d1eef87 100644 (file)
--- a/src/PeerDigest.h
+++ b/src/PeerDigest.h
@@ -79,9 +79,9 @@ public:
     PeerDigest(CachePeer *);
     ~PeerDigest();
 
-    CachePeer *peer = nullptr;          /**< pointer back to peer structure, argh */
+    CbcPointer<CachePeer> peer; ///< pointer back to peer structure, argh
     CacheDigest *cd = nullptr;            /**< actual digest structure */
-    SBuf host;                        ///< copy of peer->host
+    const SBuf host; ///< copy of peer->host
     const char *req_result = nullptr;     /**< text status of the last request */
 
     struct {
@@ -115,7 +115,6 @@ public:
 
 extern const Version CacheDigestVer;
 
-void peerDigestCreate(CachePeer * p);
 void peerDigestNeeded(PeerDigest * pd);
 void peerDigestNotePeerGone(PeerDigest * pd);
 void peerDigestStatsReport(const PeerDigest * pd, StoreEntry * e);

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index aa12ba54f006d2c0f7bc349bc7263c7c38d48170..71fb82d848925712aaf4c6889217f3ccc2d04210 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -2391,7 +2391,7 @@ parse_peer(CachePeer ** head)
 
 #if USE_CACHE_DIGESTS
     if (!p->options.no_digest)
-        peerDigestCreate(p);
+        p->digest = new PeerDigest(p);
 #endif
 
     if (p->secure.encryptTransport)

diff --git a/src/neighbors.h b/src/neighbors.h
index 9bff223529e077b3722046a8430b9dac8f8e3ac5..be360139dd3fdaba00a84d9f013a65d899e6ba05 100644 (file)
--- a/src/neighbors.h
+++ b/src/neighbors.h
@@ -60,7 +60,6 @@ void peerAlive(CachePeer *);
 lookup_t peerDigestLookup(CachePeer * p, PeerSelector *);
 CachePeer *neighborsDigestSelect(PeerSelector *);
 void peerNoteDigestLookup(HttpRequest * request, CachePeer * p, lookup_t lookup);
-void peerNoteDigestGone(CachePeer * p);
 int neighborUp(const CachePeer * e);
 const char *neighborTypeStr(const CachePeer * e);
 peer_t neighborType(const CachePeer *, const AnyP::Uri &);

diff --git a/src/peer_digest.cc b/src/peer_digest.cc
index 45f9e0715050c886b95a808e32506cba5f9c3831..2788fad776509212f642bde36d355f733d0c60bb 100644 (file)
--- a/src/peer_digest.cc
+++ b/src/peer_digest.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #if USE_CACHE_DIGESTS
+#include "base/IoManip.h"
 #include "CacheDigest.h"
 #include "CachePeer.h"
 #include "event.h"
@@ -65,19 +66,10 @@ static const time_t GlobDigestReqMinGap = 1 * 60;   /* seconds */
 
 static time_t pd_last_req_time = 0; /* last call to Check */
 
-PeerDigest::PeerDigest(CachePeer * p)
+PeerDigest::PeerDigest(CachePeer * const p):
+    peer(p),
+    host(peer->host) // if peer disappears, we will know its name
 {
-    assert(p);
-
-    /*
-     * DPW 2007-04-12
-     * Lock on to the peer here.  The corresponding cbdataReferenceDone()
-     * is in peerDigestDestroy().
-     */
-    peer = cbdataReference(p);
-    /* if peer disappears, we will know it's name */
-    host = p->host;
-
     times.initialized = squid_curtime;
 }
 
@@ -124,47 +116,6 @@ DigestFetchState::~DigestFetchState()
     assert(pd == nullptr);
 }
 
-/* allocate new peer digest, call Init, and lock everything */
-void
-peerDigestCreate(CachePeer * p)
-{
-    assert(p);
-
-    PeerDigest *pd = new PeerDigest(p);
-
-    // TODO: make CachePeer member a CbcPointer
-    p->digest = cbdataReference(pd);
-
-    // lock a reference to pd again to prevent the PeerDigest
-    // disappearing during peerDigestDestroy() when
-    // cbdataReferenceValidDone is called.
-    // TODO test if it can be moved into peerDigestDestroy() or
-    //      if things can break earlier (eg CachePeer death).
-    (void)cbdataReference(pd);
-}
-
-/* call Clean and free/unlock everything */
-static void
-peerDigestDestroy(PeerDigest * pd)
-{
-    void *p;
-    assert(pd);
-    void * peerTmp = pd->peer;
-
-    /*
-     * DPW 2007-04-12
-     * We locked the peer in PeerDigest constructor, this is
-     * where we unlock it.
-     */
-    if (cbdataReferenceValidDone(peerTmp, &p)) {
-        // we locked the p->digest in peerDigestCreate()
-        // this is where we unlock that
-        cbdataReferenceDone(static_cast<CachePeer *>(p)->digest);
-    }
-
-    delete pd;
-}
-
 PeerDigest::~PeerDigest()
 {
     if (times.next_check && eventFind(peerDigestCheck, this))
@@ -229,7 +180,7 @@ peerDigestNotePeerGone(PeerDigest * pd)
         /* do nothing now, the fetching chain will notice and take action */
     } else {
         debugs(72, 2, "peerDigest: peer " << pd->host << " is gone, destroying now.");
-        peerDigestDestroy(pd);
+        delete pd;
     }
 }
 
@@ -246,12 +197,12 @@ peerDigestCheck(void *data)
 
     pd->times.next_check = 0;   /* unknown */
 
-    if (!cbdataReferenceValid(pd->peer)) {
+    if (pd->peer.set() && !pd->peer.valid()) {
         peerDigestNotePeerGone(pd);
         return;
     }
 
-    debugs(72, 3, "cache_peer " << *pd->peer);
+    debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil());
     debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<
            ", last received: " << (long int) pd->times.received << "  (" <<
            std::showpos << (int) (squid_curtime - pd->times.received) << ")");
@@ -291,7 +242,7 @@ peerDigestCheck(void *data)
 static void
 peerDigestRequest(PeerDigest * pd)
 {
-    CachePeer *p = pd->peer;
+    const auto p = pd->peer.get(); // TODO: Replace with a reference.
     StoreEntry *e, *old_e;
     char *url = nullptr;
     HttpRequest *req;
@@ -701,13 +652,13 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
     const char *reason = nullptr;  /* reason for completion */
     const char *no_bug = nullptr;  /* successful completion if set */
     const int pdcb_valid = cbdataReferenceValid(fetch->pd);
-    const int pcb_valid = cbdataReferenceValid(fetch->pd->peer);
+    const int pcb_valid = pdcb_valid && fetch->pd->peer.valid();
 
     /* test possible exiting conditions (the same for most steps!)
      * cases marked with '?!' should not happen */
 
     if (!reason) {
-        if (!(pd = fetch->pd))
+        if (!pdcb_valid || !(pd = fetch->pd))
             reason = "peer digest disappeared?!";
         else
             host = pd->host;
@@ -719,7 +670,7 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
     /* continue checking (with pd and host known and valid) */
 
     if (!reason) {
-        if (!cbdataReferenceValid(pd->peer))
+        if (!pd->peer)
             reason = "peer disappeared";
         else if (size < 0)
             reason = "swap failure";