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 29147433a34d0bfe8421e4bc97e0253dadd564f3..13f718ffe75da4d7cceb313aac9b8e63e83f687b 100644 (file)
--- 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 9a190d8315cfff5f72f324c333b6f9a1e874522d..ec7b97b004d6d47aa5d89094f79ab1215b149029 100644 (file)
--- 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())
 {
 }