rpms/squid
6
0
Fork 0
Code Issues Pull Requests Releases Activity

Resolves: RHEL-50261 - Squid memory usage increases until OOM.

Browse Source
This commit is contained in:
Luboš Uhliarik 2025-07-17 01:47:28 +02:00
parent e2128dd15b
commit 3d289aff11
2 changed files with 171 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

163
squid-5.5-store-client-leak-fix.patch Normal file
View File

@ -0,0 +1,163 @@
diff --git a/src/StoreClient.h b/src/StoreClient.h
index 484b413..b523cc0 100644
--- a/src/StoreClient.h
+++ b/src/StoreClient.h
@@ -203,7 +203,7 @@ private:
public:
struct Callback {
- Callback ():callback_handler(NULL), callback_data(NULL) {}
+ Callback() = default;
Callback (STCB *, void *);
@@ -212,8 +212,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 d8ae028..e7144b4 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -188,14 +188,12 @@ store_client::finishCallback()
++answers;
STCB *temphandler = _callback.callback_handler;
- void *cbdata = _callback.callback_data;
- _callback = Callback(NULL, NULL);
+ const auto cbdata = _callback.cbData.validDone();
+ _callback = Callback();
copyInto.data = NULL;
- if (cbdataReferenceValid(cbdata))
+ if (cbdata)
temphandler(cbdata, result);
-
- cbdataReferenceDone(cbdata);
}
store_client::store_client(StoreEntry *e) :
@@ -255,7 +253,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;
@@ -1129,7 +1127,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);
@@ -1154,7 +1152,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())
{
}
diff --git a/src/tunnel.cc b/src/tunnel.cc
index 4fc5abd..2d37d2e 100644
--- a/src/tunnel.cc
+++ b/src/tunnel.cc
@@ -273,6 +273,7 @@ private:
/// resumes operations after the (possibly failed) HTTP CONNECT exchange
void tunnelEstablishmentDone(Http::TunnelerAnswer &answer);
+ void finishWritingAndDelete(Connection &);
void deleteThis();
void cancelStep(const char *reason);
@@ -319,7 +320,9 @@ TunnelStateData::serverClosed()
{
server.noteClosure();
- retryOrBail(__FUNCTION__);
+ request->hier.stopPeerClock(false);
+
+ finishWritingAndDelete(client);
}
/// TunnelStateData::clientClosed() wrapper
@@ -334,12 +337,48 @@ void
TunnelStateData::clientClosed()
{
client.noteClosure();
+ finishWritingAndDelete(server);
+}
+/// Gracefully handles non-retriable connection closure. If necessary, either
+/// starts closing the given connection or waits for its pending write to
+/// finish. Otherwise, immediately destroys the tunnel object.
+/// \prec The other Connection is not open.
+void
+TunnelStateData::finishWritingAndDelete(Connection &remainingConnection)
+{
if (noConnections())
return deleteThis();
- if (!server.writer)
- server.conn->close();
+ // XXX: The code below should precede the noConnections() check above. When
+ // there is no writer, we should trigger an immediate noConnections()
+ // outcome instead of waiting for an asynchronous call to our own closure
+ // callback (that will call this method again). We should not move this code
+ // until a noConnections() outcome guarantees a nil writer because such a
+ // move will unnecessary delay deleteThis().
+
+ if (remainingConnection.writer) {
+ debugs(26, 5, "waiting to finish writing to " << remainingConnection.conn);
+ // the write completion callback must close its remainingConnection
+ // after noticing that the other connection is gone
+ return;
+ }
+
+ // XXX: Stop abusing connection closure callback for terminating tunneling
+ // in cases like this, where our code knows that tunneling must end. The
+ // closure callback should be dedicated to handling rare connection closures
+ // originated _outside_ of TunnelStateData (e.g., during shutdown). In all
+ // other cases, our own close()-calling code must detail the
+ // closure-triggering error (if any) _and_ clear all callbacks: Our code
+ // does not need to be (asynchronously) notified of the closure that it
+ // itself has initiated! Until that (significant) refactoring,
+ // serverClosed() and clientClosed() callbacks will continue to mishandle
+ // those rare closures as regular ones, and access.log records will continue
+ // to lack some tunneling error indicators/details.
+ //
+ // This asynchronous close() leads to another finishWritingAndDelete() call
+ // but with true noConnections() that finally triggers deleteThis().
+ remainingConnection.conn->close();
}
/// destroys the tunnel (after performing potentially-throwing cleanup)
@@ -451,14 +490,7 @@ TunnelStateData::retryOrBail(const char *context)
return sendError(savedError, bailDescription ? bailDescription : context);
*status_ptr = savedError->httpStatus;
- if (noConnections())
- return deleteThis();
-
- // This is a "Comm::IsConnOpen(client.conn) but !canSendError" case.
- // Closing the connection (after finishing writing) is the best we can do.
- if (!client.writer)
- client.conn->close();
- // else writeClientDone() must notice a closed server and close the client
+ finishWritingAndDelete(client);
}
int

9
squid.spec
View File

@ -2,7 +2,7 @@
Name: squid
Version: 5.5
Release: 20%{?dist}
Release: 21%{?dist}
Summary: The Squid proxy caching server
Epoch: 7
# See CREDITS for breakdown of non GPLv2+ code
@ -54,6 +54,8 @@ Patch211: squid-5.5-large-upload-buffer-dies.patch
Patch212: squid-5.5-fatal-read-data-from-mem.patch
# https://issues.redhat.com/browse/RHEL-77282
Patch213: squid-5.5-cache-peer-connect-errors.patch
# https://issues.redhat.com/browse/RHEL-50261
Patch214: squid-5.5-store-client-leak-fix.patch
# Security patches
# https://bugzilla.redhat.com/show_bug.cgi?id=2100721
@ -185,6 +187,8 @@ lookup program (dnsserver), a program for retrieving FTP data
# patch506 follow-up
%patch212 -p1 -b .fatal-read-data-from-mem
%patch214 -p1 -b .store-client-mem-leak
# https://bugzilla.redhat.com/show_bug.cgi?id=1679526
# Patch in the vendor documentation and used different location for documentation
sed -i 's|@SYSCONFDIR@/squid.conf.documented|%{_pkgdocdir}/squid.conf.documented|' src/squid.8.in
@ -410,6 +414,9 @@ fi
%changelog
* Wed Jul 16 2025 Luboš Uhliarik <luhliari@redhat.com> - 7:5.5-21
- Resolves: RHEL-50261 - Squid memory usage increases until OOM.
* Thu Apr 10 2025 Luboš Uhliarik <luhliari@redhat.com> - 7:5.5-20
- Resolves: RHEL-77282 - ”TCP connection to XX.XX.XX.XX/XXXX failed” message is
output frequently on RHEL9