From: Eduard Bagdasaryan Date: Mon, 12 Jun 2023 13:51:16 +0000 (+0000) Subject: Stop leaking PeerDigests on reconfiguration (#1371) X-Git-Tag: SQUID_7_0_1~429 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0f78379;p=thirdparty%2Fsquid.git 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 2864e35fdf..f76923db39 100644 --- 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 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 aa12ba54f0..71fb82d848 100644 --- 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 9bff223529..be360139dd 100644 --- 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 45f9e07150..2788fad776 100644 --- 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(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";