From fb5d65bd298a33525db7a189eb2c7d8be58cb057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubo=C5=A1=20Uhliarik?= Date: Tue, 8 Aug 2023 16:53:27 +0200 Subject: [PATCH] Fix "!commHasHalfClosedMonitor(fd)" assertion --- squid-6.1-crash-half-closed.patch | 158 ++++++++++++++++++++++++++++++ squid.spec | 8 +- 2 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 squid-6.1-crash-half-closed.patch diff --git a/squid-6.1-crash-half-closed.patch b/squid-6.1-crash-half-closed.patch new file mode 100644 index 0000000..901ece2 --- /dev/null +++ b/squid-6.1-crash-half-closed.patch @@ -0,0 +1,158 @@ +diff --git a/src/client_side.cc b/src/client_side.cc +index f488fc4..69586df 100644 +--- a/src/client_side.cc ++++ b/src/client_side.cc +@@ -932,7 +932,7 @@ ConnStateData::kick() + * We are done with the response, and we are either still receiving request + * body (early response!) or have already stopped receiving anything. + * +- * If we are still receiving, then clientParseRequest() below will fail. ++ * If we are still receiving, then parseRequests() below will fail. + * (XXX: but then we will call readNextRequest() which may succeed and + * execute a smuggled request as we are not done with the current request). + * +@@ -952,28 +952,12 @@ ConnStateData::kick() + * Attempt to parse a request from the request buffer. + * If we've been fed a pipelined request it may already + * be in our read buffer. +- * +- \par +- * This needs to fall through - if we're unlucky and parse the _last_ request +- * from our read buffer we may never re-register for another client read. + */ + +- if (clientParseRequests()) { +- debugs(33, 3, clientConnection << ": parsed next request from buffer"); +- } ++ parseRequests(); + +- /** \par +- * Either we need to kick-start another read or, if we have +- * a half-closed connection, kill it after the last request. +- * This saves waiting for half-closed connections to finished being +- * half-closed _AND_ then, sometimes, spending "Timeout" time in +- * the keepalive "Waiting for next request" state. +- */ +- if (commIsHalfClosed(clientConnection->fd) && pipeline.empty()) { +- debugs(33, 3, "half-closed client with no pending requests, closing"); +- clientConnection->close(); ++ if (!isOpen()) + return; +- } + + /** \par + * At this point we either have a parsed request (which we've +@@ -1893,16 +1877,11 @@ ConnStateData::receivedFirstByte() + resetReadTimeout(Config.Timeout.request); + } + +-/** +- * Attempt to parse one or more requests from the input buffer. +- * Returns true after completing parsing of at least one request [header]. That +- * includes cases where parsing ended with an error (e.g., a huge request). +- */ +-bool +-ConnStateData::clientParseRequests() ++/// Attempt to parse one or more requests from the input buffer. ++/// May close the connection. ++void ++ConnStateData::parseRequests() + { +- bool parsed_req = false; +- + debugs(33, 5, clientConnection << ": attempting to parse"); + + // Loop while we have read bytes that are not needed for producing the body +@@ -1947,8 +1926,6 @@ ConnStateData::clientParseRequests() + + processParsedRequest(context); + +- parsed_req = true; // XXX: do we really need to parse everything right NOW ? +- + if (context->mayUseConnection()) { + debugs(33, 3, "Not parsing new requests, as this request may need the connection"); + break; +@@ -1961,8 +1938,19 @@ ConnStateData::clientParseRequests() + } + } + +- /* XXX where to 'finish' the parsing pass? */ +- return parsed_req; ++ debugs(33, 7, "buffered leftovers: " << inBuf.length()); ++ ++ if (isOpen() && commIsHalfClosed(clientConnection->fd)) { ++ if (pipeline.empty()) { ++ // we processed what we could parse, and no more data is coming ++ debugs(33, 5, "closing half-closed without parsed requests: " << clientConnection); ++ clientConnection->close(); ++ } else { ++ // we parsed what we could, and no more data is coming ++ debugs(33, 5, "monitoring half-closed while processing parsed requests: " << clientConnection); ++ flags.readMore = false; // may already be false ++ } ++ } + } + + void +@@ -1979,18 +1967,7 @@ ConnStateData::afterClientRead() + if (pipeline.empty()) + fd_note(clientConnection->fd, "Reading next request"); + +- if (!clientParseRequests()) { +- if (!isOpen()) +- return; +- // We may get here if the client half-closed after sending a partial +- // request. See doClientRead() and shouldCloseOnEof(). +- // XXX: This partially duplicates ConnStateData::kick(). +- if (pipeline.empty() && commIsHalfClosed(clientConnection->fd)) { +- debugs(33, 5, clientConnection << ": half-closed connection, no completed request parsed, connection closing."); +- clientConnection->close(); +- return; +- } +- } ++ parseRequests(); + + if (!isOpen()) + return; +@@ -3775,7 +3752,7 @@ ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext pic) + startPinnedConnectionMonitoring(); + + if (pipeline.empty()) +- kick(); // in case clientParseRequests() was blocked by a busy pic.connection ++ kick(); // in case parseRequests() was blocked by a busy pic.connection + } + + /// Forward future client requests using the given server connection. +diff --git a/src/client_side.h b/src/client_side.h +index 6027b31..60b99b1 100644 +--- a/src/client_side.h ++++ b/src/client_side.h +@@ -98,7 +98,6 @@ public: + void doneWithControlMsg() override; + + /// Traffic parsing +- bool clientParseRequests(); + void readNextRequest(); + + /// try to make progress on a transaction or read more I/O +@@ -443,6 +442,7 @@ private: + + void checkLogging(); + ++ void parseRequests(); + void clientAfterReadingRequests(); + bool concurrentRequestQueueFilled() const; + +diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc +index 8c160e5..f49d5dc 100644 +--- a/src/tests/stub_client_side.cc ++++ b/src/tests/stub_client_side.cc +@@ -14,7 +14,7 @@ + #include "tests/STUB.h" + + #include "client_side.h" +-bool ConnStateData::clientParseRequests() STUB_RETVAL(false) ++void ConnStateData::parseRequests() STUB + void ConnStateData::readNextRequest() STUB + bool ConnStateData::isOpen() const STUB_RETVAL(false) + void ConnStateData::kick() STUB diff --git a/squid.spec b/squid.spec index 94cf540..51f5852 100644 --- a/squid.spec +++ b/squid.spec @@ -2,7 +2,7 @@ Name: squid Version: 6.1 -Release: 2%{?dist} +Release: 3%{?dist} Summary: The Squid proxy caching server Epoch: 7 # See CREDITS for breakdown of non GPLv2+ code @@ -36,6 +36,8 @@ Patch203: squid-6.1-perlpath.patch # revert this upstream patch - https://bugzilla.redhat.com/show_bug.cgi?id=1936422 # workaround for #1934919 Patch204: squid-6.1-symlink-lang-err.patch +# Upstream PR: https://github.com/squid-cache/squid/pull/1442 +Patch205: squid-6.1-crash-half-closed.patch # cache_swap.sh Requires: bash gawk @@ -105,6 +107,7 @@ lookup program (dnsserver), a program for retrieving FTP data %patch -P 202 -p1 -b .location %patch -P 203 -p1 -b .perlpath %patch -P 204 -p1 -b .symlink-lang-err +%patch -P 205 -p1 -b .crash-half-closed # https://bugzilla.redhat.com/show_bug.cgi?id=1679526 # Patch in the vendor documentation and used different location for documentation @@ -332,6 +335,9 @@ fi %changelog +* Fri Aug 04 2023 Luboš Uhliarik - 7:6.1-3 +- Fix "!commHasHalfClosedMonitor(fd)" assertion + * Sat Jul 22 2023 Fedora Release Engineering - 7:6.1-2 - Rebuilt for https://fedoraproject.org/wiki/Fedora_39_Mass_Rebuild