From d189b5a5a5e705a16b2d12e4e37f96f647c0d74f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 27 Jun 2023 22:47:36 +0000 Subject: [PATCH] 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. --- src/StoreClient.h | 6 +++--- src/store_client.cc | 14 ++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) 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()) { } -- 2.47.2