From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 27 Jun 2023 22:47:36 +0000 (+0000)
Subject: Fix store_client caller memory leak on certain errors (#1347)
X-Git-Tag: SQUID_6_4~5
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d189b5a5a5e705a16b2d12e4e37f96f647c0d74f;p=thirdparty%2Fsquid.git

Fix store_client caller memory leak on certain errors (#1347)

When a storeUnregister() code path destroys store_client before the
latter has a chance to deliver the answer, the cbdataReferenceDone()
call in store_client::finishCallback() is not reached, keeping the
callback data (e.g., clientReplyContext) alive forever. These
storeClientCopy() "cancellations" may happen, for example, when the
client-to-Squid connection is closed while store_client waits for Store.

Use CallbackData to guarantee cbdataReferenceDone() when store_client is
destructed before it can finishCallback(). These synchronous callbacks
will be replaced with AsyncCalls. For now, we use the "discouraged"
CallbackData API to accommodate the existing legacy callbacks.

Probably broken since 2002 commit fa80a8e.
---

diff --git a/src/StoreClient.h b/src/StoreClient.h
index 29147433a3..13f718ffe7 100644
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -193,7 +193,7 @@ private:
 public:
 
     struct Callback {
-        Callback ():callback_handler(nullptr), callback_data(nullptr) {}
+        Callback() = default;
 
         Callback (STCB *, void *);
 
@@ -202,8 +202,8 @@ public:
         /// delivery to the STCB callback_handler.
         bool pending() const;
 
-        STCB *callback_handler;
-        void *callback_data;
+        STCB *callback_handler = nullptr; ///< where to deliver the answer
+        CallbackData cbData; ///< the first STCB callback parameter
         CodeContextPointer codeContext; ///< Store client context
 
         /// a scheduled asynchronous finishCallback() call (or nil)
diff --git a/src/store_client.cc b/src/store_client.cc
index 9a190d8315..ec7b97b004 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -170,14 +170,12 @@ store_client::finishCallback()
     ++answers;
 
     STCB *temphandler = _callback.callback_handler;
-    void *cbdata = _callback.callback_data;
-    _callback = Callback(nullptr, nullptr);
+    const auto cbdata = _callback.cbData.validDone();
+    _callback = Callback();
     copyInto.data = nullptr;
 
-    if (cbdataReferenceValid(cbdata))
+    if (cbdata)
         temphandler(cbdata, result);
-
-    cbdataReferenceDone(cbdata);
 }
 
 store_client::store_client(StoreEntry *e) :
@@ -243,7 +241,7 @@ store_client::copy(StoreEntry * anEntry,
 #endif
 
     assert(!_callback.pending());
-    _callback = Callback (callback_fn, cbdataReference(data));
+    _callback = Callback(callback_fn, data);
     copyInto.data = copyRequest.data;
     copyInto.length = copyRequest.length;
     copyInto.offset = copyRequest.offset;
@@ -1049,7 +1047,7 @@ store_client::dumpStats(MemBuf * output, int clientNumber) const
     if (_callback.pending())
         return;
 
-    output->appendf("\tClient #%d, %p\n", clientNumber, _callback.callback_data);
+    output->appendf("\tClient #%d, %p\n", clientNumber, this);
     output->appendf("\t\tcopy_offset: %" PRId64 "\n", copyInto.offset);
     output->appendf("\t\tcopy_size: %" PRIuSIZE "\n", copyInto.length);
     output->append("\t\tflags:", 8);
@@ -1074,7 +1072,7 @@ store_client::Callback::pending() const
 
 store_client::Callback::Callback(STCB *function, void *data):
     callback_handler(function),
-    callback_data(data),
+    cbData(data),
     codeContext(CodeContext::Current())
 {
 }