From db8b598f02b6adbdf0acc26770b3ab34282dcdfa Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 1 Apr 2024 02:53:39 +0000 Subject: [PATCH 01/16] Bug 5360: FwdState::noteDestinationsEnd() assertion "err" (#1767) FATAL: assertion failed: FwdState.cc:660: "err" When FwdState decides to re-forward a request, it forgets the original response[^1] but does not create an error object. Since commit e2bbd3b, FwdState::noteDestinationsEnd() correctly assumed that we only idly wait for additional destinations after encountering a problem, but incorrectly asserted that past problems imply error object existence. Now Squid generates an HTTP 502 (Bad Gateway) response while setting %err_code/%err_detail to ERR_CANNOT_FORWARD/REFORWARD_TO_NONE. TunnelStateData::noteDestinationsEnd() code is similar, but it probably does not suffer from the same bug because an error object is created before every retryOrBail() call, and there is no re-forwarding code that forgets an HTTP error response without creating an error. Those invariants are not well tracked, and this change mimics FwdState changes in TunnelStateData just in case and to keep the two methods in sync. In those hypothetical cases, ERR_CANNOT_FORWARD/RETRY_TO_NONE is logged. [^1]: Long-term we probably want to preserve that original response so that we do not have to replace it with a generated error, but doing so requires significant refactoring and is outside this minimal fix scope. --- src/FwdState.cc | 8 +++++++- src/tunnel.cc | 10 +++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index abae1d6a5a..34cbd873ca 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -647,7 +647,13 @@ FwdState::noteDestinationsEnd(ErrorState *selectionError) } // destinationsFound, but none of them worked, and we were waiting for more - assert(err); + debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts"); + if (!err) { + const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request, al); + static const auto d = MakeNamedErrorDetail("REFORWARD_TO_NONE"); + finalError->detailError(d); + fail(finalError); + } // else use actual error from last forwarding attempt stopAndDestroy("all found paths have failed"); } diff --git a/src/tunnel.cc b/src/tunnel.cc index dca34bdc5d..81cd72cbb7 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -1364,7 +1364,15 @@ TunnelStateData::noteDestinationsEnd(ErrorState *selectionError) } // destinationsFound, but none of them worked, and we were waiting for more - assert(savedError); + debugs(17, 7, "no more destinations to try after " << n_tries << " failed attempts"); + if (!savedError) { + // retryOrBail() must be preceded by saveError(), but in case we forgot: + const auto finalError = new ErrorState(ERR_CANNOT_FORWARD, Http::scBadGateway, request.getRaw(), al); + static const auto d = MakeNamedErrorDetail("RETRY_TO_NONE"); + finalError->detailError(d); + saveError(finalError); + } // else use actual error from last forwarding attempt + // XXX: Honor clientExpectsConnectResponse() before replying. sendError(savedError, "all found paths have failed"); } -- 2.39.2 From 8f4adbf753df8b8a56e07ee91938848a19a88d94 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 1 Apr 2024 19:43:21 +0000 Subject: [PATCH 02/16] scripts/find-alive.pl: Auto-detect auto-added ctors/dtors names (#1769) debugs(24, 9, "constructed, this=" << static_cast(this) ... MemBlob.cc(54) MemBlob: constructed, this=0x557f06b3ef00 ... Class constructor and destructor sources should not manually duplicate class names in debugs() statements. Many existing classes do the right thing, but find-alive.pl still insists on a manually added class name, forcing its users to add a colon: `find-alive.pl MemBlob:`. We now accommodate both legacy/manual and modern/auto use cases. --- scripts/find-alive.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/find-alive.pl b/scripts/find-alive.pl index 40422c15b1..f0331eef08 100755 --- a/scripts/find-alive.pl +++ b/scripts/find-alive.pl @@ -75,8 +75,8 @@ my %Pairs = ( if (!$Pairs{$Thing}) { warn("guessing construction/destruction pattern for $Thing\n"); $Pairs{$Thing} = [ - "\\b$Thing construct.*, this=(\\S+)", - "\\b$Thing destruct.*, this=(\\S+)", + "\\b${Thing}:? construct.*, this=(\\S+)", + "\\b${Thing}:? destruct.*, this=(\\S+)", ]; } -- 2.39.2 From 1891ce596237b45e0a675f75c49a5f6a1111840d Mon Sep 17 00:00:00 2001 From: xiaoxiaoafeifei Date: Tue, 2 Apr 2024 07:04:31 +0000 Subject: [PATCH 03/16] Fix heap buffer overead in ConfigParser::UnQuote() (#1763) Detected by using AddressSanitizer. --- CONTRIBUTORS | 1 + src/ConfigParser.cc | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTORS b/CONTRIBUTORS index 9e4a3214c9..cc104be4e8 100644 --- a/CONTRIBUTORS +++ b/CONTRIBUTORS @@ -300,6 +300,7 @@ Thank you! Leeann Bent Leonardo Taccari Leonid Evdokimov + Liangliang Zhai libit Lubos Uhliarik Luigi Gangitano diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc index c65124ece6..0e405eb85a 100644 --- a/src/ConfigParser.cc +++ b/src/ConfigParser.cc @@ -181,7 +181,7 @@ ConfigParser::UnQuote(const char *token, const char **next) *d = '\0'; // We are expecting a separator after quoted string, space or one of "()#" - if (*(s + 1) != '\0' && !strchr(w_space "()#", *(s + 1)) && !errorStr) { + if (!errorStr && *(s + 1) != '\0' && !strchr(w_space "()#", *(s + 1))) { errorStr = "Expecting space after the end of quoted token"; errorPos = token; } -- 2.39.2 From 2e7dea3cedd3ef2f071dee82867c4147f17376dd Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 2 Apr 2024 20:37:31 +0000 Subject: [PATCH 04/16] Do not blame cache_peer for CONNECT errors (#1772) ERROR: Connection to [such-and-such-cache_peer] failed TCP_TUNNEL/503 CONNECT nxdomain.test:443 FIRSTUP_PARENT Squid does not alert an admin about (and decrease health level of) a cache_peer that responded with an error to a GET request. Just like GET responses from a cache_peer, CONNECT responses may (and often do!) reflect client or origin server failures. We should not penalize cache_peers (and alert admins) until we can distinguish these frequent client/origin failures from (relatively rare) cache_peer problems. This change absolves cache_peers of CONNECT problems, restoring parity with GETs and restoring v4 behavior changed (probably by accident) in v5. Also removed Http::StatusCode parameter from failure notification functions because it became essentially unused after the primary Http::Tunneler changes. Tunneler was the only source of status code information that (in some cases) used received HTTP response to compute that status code. All other cases extracted that status code from Squid-generated errors. Those errors were arguably never meant to supply status code information for "this failure is not our fault" decision, and they do not supply 4xx status codes driving that decision. ### Problem evolution 2019 commit f5e1794 effectively started blaming cache_peer for all FwdState CONNECT errors. That functionality change was probably accidental, likely influenced by the names of noteConnectFailure() and peerConnectFailed() functions that abbreviated "Connection", making the functions look as applicable to CONNECT failures. Prior to that commit, the functions were never used for CONNECT errors. After it, FwdState started calling peerConnectFailed() for all CONNECT failures. In 2020 commit 25b0ce4, TunnelStateData started blaming cache_peers as well (by moving that FwdState-only error handling code into Tunneler). The same "accidental functionality change" speculations apply here. In 2022 commit 022dbab, we made an exception for 4xx CONNECT errors as folks deploying newer code started complaining about cache_peers getting blamed for client-caused errors (e.g., HTTP 403 Forbidden replies). We did not realize that the blaming code itself was an unwanted accident. Now we are getting complaints about cache_peers getting blamed for 502 and 503 CONNECT errors caused by, for example, domain names without IPs: As these CONNECT error responses are propagated from parent to child caches, every child cache in the chain logs ERRORs and every cache_peer in the chain gets its health counter decreased! --- src/CachePeer.cc | 11 +---------- src/CachePeer.h | 12 +++++------- src/HappyConnOpener.cc | 2 +- src/PeerPoolMgr.cc | 2 +- src/clients/HttpTunneler.cc | 10 ++++++---- src/clients/HttpTunneler.h | 2 +- src/neighbors.cc | 2 +- src/security/BlindPeerConnector.cc | 2 +- src/security/PeerConnector.cc | 8 ++++---- src/security/PeerConnector.h | 2 +- src/tests/stub_libsecurity.cc | 2 +- 11 files changed, 23 insertions(+), 32 deletions(-) diff --git a/src/CachePeer.cc b/src/CachePeer.cc index 63c6fb60a9..98a750cb41 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -69,20 +69,11 @@ CachePeer::noteSuccess() } } -void -CachePeer::noteFailure(const Http::StatusCode code) -{ - if (Http::Is4xx(code)) - return; // this failure is not our fault - - countFailure(); -} - // TODO: Require callers to detail failures instead of using one (and often // misleading!) "connection failed" phrase for all of them. /// noteFailure() helper for handling failures attributed to this peer void -CachePeer::countFailure() +CachePeer::noteFailure() { stats.last_connect_failure = squid_curtime; if (tcp_up > 0) diff --git a/src/CachePeer.h b/src/CachePeer.h index 783c17eea0..f888a0cdde 100644 --- a/src/CachePeer.h +++ b/src/CachePeer.h @@ -38,9 +38,8 @@ public: /// reacts to a successful establishment of a connection to this cache_peer void noteSuccess(); - /// reacts to a failure on a connection to this cache_peer - /// \param code a received response status code, if any - void noteFailure(Http::StatusCode code); + /// reacts to a failed attempt to establish a connection to this cache_peer + void noteFailure(); /// (re)configure cache_peer name=value void rename(const char *); @@ -236,14 +235,13 @@ NoteOutgoingConnectionSuccess(CachePeer * const peer) peer->noteSuccess(); } -/// reacts to a failure on a connection to an origin server or cache_peer +/// reacts to a failed attempt to establish a connection to an origin server or cache_peer /// \param peer nil if the connection is to an origin server -/// \param code a received response status code, if any inline void -NoteOutgoingConnectionFailure(CachePeer * const peer, const Http::StatusCode code) +NoteOutgoingConnectionFailure(CachePeer * const peer) { if (peer) - peer->noteFailure(code); + peer->noteFailure(); } /// identify the given cache peer in cache.log messages and such diff --git a/src/HappyConnOpener.cc b/src/HappyConnOpener.cc index 981216030c..a2d3052583 100644 --- a/src/HappyConnOpener.cc +++ b/src/HappyConnOpener.cc @@ -638,7 +638,7 @@ HappyConnOpener::handleConnOpenerAnswer(Attempt &attempt, const CommConnectCbPar lastError = makeError(ERR_CONNECT_FAIL); lastError->xerrno = params.xerrno; - NoteOutgoingConnectionFailure(params.conn->getPeer(), lastError->httpStatus); + NoteOutgoingConnectionFailure(params.conn->getPeer()); if (spareWaiting) updateSpareWaitAfterPrimeFailure(); diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 6b6b440b6b..a65e6bbdcb 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -87,7 +87,7 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams ¶ms) } if (params.flag != Comm::OK) { - NoteOutgoingConnectionFailure(peer, Http::scNone); + NoteOutgoingConnectionFailure(peer); checkpoint("conn opening failure"); // may retry return; } diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index 1df7b15b0c..d52dd457e8 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -90,7 +90,7 @@ Http::Tunneler::handleConnectionClosure(const CommCloseCbParams &) { closer = nullptr; if (connection) { - countFailingConnection(nullptr); + countFailingConnection(); connection->noteClosure(); connection = nullptr; } @@ -355,7 +355,7 @@ Http::Tunneler::bailWith(ErrorState *error) if (const auto failingConnection = connection) { // TODO: Reuse to-peer connections after a CONNECT error response. - countFailingConnection(error); + countFailingConnection(); disconnect(); failingConnection->close(); } @@ -374,10 +374,12 @@ Http::Tunneler::sendSuccess() } void -Http::Tunneler::countFailingConnection(const ErrorState * const error) +Http::Tunneler::countFailingConnection() { assert(connection); - NoteOutgoingConnectionFailure(connection->getPeer(), error ? error->httpStatus : Http::scNone); + // No NoteOutgoingConnectionFailure(connection->getPeer()) call here because + // we do not blame cache_peer for CONNECT failures (on top of a successfully + // established connection to that cache_peer). if (noteFwdPconnUse && connection->isOpen()) fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); } diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index eb8dcee685..d5090d3ac0 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -80,7 +80,7 @@ private: void disconnect(); /// updates connection usage history before the connection is closed - void countFailingConnection(const ErrorState *); + void countFailingConnection(); AsyncCall::Pointer writer; ///< called when the request has been written AsyncCall::Pointer reader; ///< called when the response should be read diff --git a/src/neighbors.cc b/src/neighbors.cc index a2719f628f..34a4383df9 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -1227,7 +1227,7 @@ peerProbeConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int if (status == Comm::OK) p->noteSuccess(); else - p->noteFailure(Http::scNone); + p->noteFailure(); -- p->testing_now; conn->close(); diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 1811ae96f6..d57a4ea25e 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -76,7 +76,7 @@ Security::BlindPeerConnector::noteNegotiationDone(ErrorState *error) // based on TCP results, SSL results, or both. And the code is probably not // consistent in this aspect across tunnelling and forwarding modules. if (peer && peer->secure.encryptTransport) - peer->noteFailure(error->httpStatus); + peer->noteFailure(); return; } diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index f12229403e..96c2f0ed34 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -115,7 +115,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) err->detailError(d); if (serverConn) { - countFailingConnection(err); + countFailingConnection(); serverConn->noteClosure(); serverConn = nullptr; } @@ -507,7 +507,7 @@ Security::PeerConnector::bail(ErrorState *error) answer().error = error; if (const auto failingConnection = serverConn) { - countFailingConnection(error); + countFailingConnection(); disconnect(); failingConnection->close(); } @@ -525,10 +525,10 @@ Security::PeerConnector::sendSuccess() } void -Security::PeerConnector::countFailingConnection(const ErrorState * const error) +Security::PeerConnector::countFailingConnection() { assert(serverConn); - NoteOutgoingConnectionFailure(serverConn->getPeer(), error ? error->httpStatus : Http::scNone); + NoteOutgoingConnectionFailure(serverConn->getPeer()); // TODO: Calling PconnPool::noteUses() should not be our responsibility. if (noteFwdPconnUse && serverConn->isOpen()) fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index 3c7c01b8dc..b00f3852a8 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -150,7 +150,7 @@ protected: void disconnect(); /// updates connection usage history before the connection is closed - void countFailingConnection(const ErrorState *); + void countFailingConnection(); /// If called the certificates validator will not used void bypassCertValidator() {useCertValidator_ = false;} diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index c4d3accab9..456f3122ae 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -105,7 +105,7 @@ void PeerConnector::bail(ErrorState *) STUB void PeerConnector::sendSuccess() STUB void PeerConnector::callBack() STUB void PeerConnector::disconnect() STUB -void PeerConnector::countFailingConnection(const ErrorState *) STUB +void PeerConnector::countFailingConnection() STUB void PeerConnector::recordNegotiationDetails() STUB EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer) } -- 2.39.2 From 47c9c9376d11a828a675e96ad6fa96c9fbcbceb0 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 3 Apr 2024 09:25:55 +0000 Subject: [PATCH 05/16] Fix const-correctness of ACLHTTPHeaderData::match() parameter (#1771) ACLHTTPHeaderData::match() required a pointer to non-const HttpHeader but does not (and should not) modify the supplied HttpHeader. Also removed support for nil HttpHeader in that method. All callers already require HttpHeader presence. If that changes, it is the _caller_ that should decide what HttpHeader absence means (match, mismatch, exception/dunno, etc.); ACLHTTPHeaderData does not have enough information to make the right choice and, hence, should not be asked to choose. Also polished related #includes to remove unnecessary ones. --- src/acl/HttpHeaderData.cc | 11 ++++------- src/acl/HttpHeaderData.h | 4 ++-- src/acl/HttpRepHeader.cc | 5 +---- src/acl/HttpRepHeader.h | 4 ++-- src/acl/HttpReqHeader.cc | 5 +---- src/acl/HttpReqHeader.h | 4 ++-- src/http/forward.h | 2 ++ 7 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/acl/HttpHeaderData.cc b/src/acl/HttpHeaderData.cc index 2db401e143..7e1efdab0c 100644 --- a/src/acl/HttpHeaderData.cc +++ b/src/acl/HttpHeaderData.cc @@ -36,20 +36,17 @@ ACLHTTPHeaderData::~ACLHTTPHeaderData() } bool -ACLHTTPHeaderData::match(HttpHeader* hdr) +ACLHTTPHeaderData::match(const HttpHeader &hdr) { - if (hdr == nullptr) - return false; - debugs(28, 3, "aclHeaderData::match: checking '" << hdrName << "'"); String value; if (hdrId != Http::HdrType::BAD_HDR) { - if (!hdr->has(hdrId)) + if (!hdr.has(hdrId)) return false; - value = hdr->getStrOrList(hdrId); + value = hdr.getStrOrList(hdrId); } else { - if (!hdr->hasNamed(hdrName, &value)) + if (!hdr.hasNamed(hdrName, &value)) return false; } diff --git a/src/acl/HttpHeaderData.h b/src/acl/HttpHeaderData.h index 5036d114dc..fc103ebec1 100644 --- a/src/acl/HttpHeaderData.h +++ b/src/acl/HttpHeaderData.h @@ -14,14 +14,14 @@ #include "sbuf/SBuf.h" #include "SquidString.h" -class ACLHTTPHeaderData : public ACLData +class ACLHTTPHeaderData: public ACLData { MEMPROXY_CLASS(ACLHTTPHeaderData); public: ACLHTTPHeaderData(); ~ACLHTTPHeaderData() override; - bool match(HttpHeader* hdr) override; + bool match(const HttpHeader &) override; SBufList dump() const override; void parse() override; bool empty() const override; diff --git a/src/acl/HttpRepHeader.cc b/src/acl/HttpRepHeader.cc index afa7a52c3f..54c29c7942 100644 --- a/src/acl/HttpRepHeader.cc +++ b/src/acl/HttpRepHeader.cc @@ -8,15 +8,12 @@ #include "squid.h" #include "acl/FilledChecklist.h" -#include "acl/HttpHeaderData.h" #include "acl/HttpRepHeader.h" #include "HttpReply.h" int Acl::HttpRepHeaderCheck::match(ACLChecklist * const ch) { - const auto checklist = Filled(ch); - - return data->match (&checklist->reply->header); + return data->match(Filled(ch)->reply->header); } diff --git a/src/acl/HttpRepHeader.h b/src/acl/HttpRepHeader.h index 845f50b95a..e92ca71da9 100644 --- a/src/acl/HttpRepHeader.h +++ b/src/acl/HttpRepHeader.h @@ -11,13 +11,13 @@ #include "acl/Data.h" #include "acl/ParameterizedNode.h" -#include "HttpHeader.h" +#include "http/forward.h" namespace Acl { /// a "rep_header" ACL -class HttpRepHeaderCheck: public ParameterizedNode< ACLData > +class HttpRepHeaderCheck: public ParameterizedNode< ACLData > { public: /* Acl::Node API */ diff --git a/src/acl/HttpReqHeader.cc b/src/acl/HttpReqHeader.cc index 1d8bf42a7f..b6d3a7da8c 100644 --- a/src/acl/HttpReqHeader.cc +++ b/src/acl/HttpReqHeader.cc @@ -8,15 +8,12 @@ #include "squid.h" #include "acl/FilledChecklist.h" -#include "acl/HttpHeaderData.h" #include "acl/HttpReqHeader.h" #include "HttpRequest.h" int Acl::HttpReqHeaderCheck::match(ACLChecklist * const ch) { - const auto checklist = Filled(ch); - - return data->match (&checklist->request->header); + return data->match(Filled(ch)->request->header); } diff --git a/src/acl/HttpReqHeader.h b/src/acl/HttpReqHeader.h index e313a06de6..1e3f0d394b 100644 --- a/src/acl/HttpReqHeader.h +++ b/src/acl/HttpReqHeader.h @@ -11,13 +11,13 @@ #include "acl/Data.h" #include "acl/ParameterizedNode.h" -#include "HttpHeader.h" +#include "http/forward.h" namespace Acl { /// a "req_header" ACL -class HttpReqHeaderCheck: public ParameterizedNode< ACLData > +class HttpReqHeaderCheck: public ParameterizedNode< ACLData > { public: /* Acl::Node API */ diff --git a/src/http/forward.h b/src/http/forward.h index ad9cd54742..e5bf185588 100644 --- a/src/http/forward.h +++ b/src/http/forward.h @@ -39,6 +39,8 @@ typedef enum { class HttpHdrSc; +class HttpHeader; + class HttpRequestMethod; class HttpRequest; -- 2.39.2 From 8800478b8fda415d24f3939323307b0bae7805a5 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 5 Apr 2024 03:39:57 +0000 Subject: [PATCH 06/16] Fix eCAP header includes (#1753) Squid style guidelines require .h files to be wrapped with HAVE_*_H protection and placed after all Squid internal file includes. Add the missing ./configure header checks to generate the needed wrappers and refactor the include sequences to meet current guidelines. --- configure.ac | 39 +++++++++++++++++++++++++++---- src/adaptation/ecap/Host.cc | 13 ++++++++--- src/adaptation/ecap/Host.h | 2 ++ src/adaptation/ecap/MessageRep.cc | 23 +++++++++++------- src/adaptation/ecap/MessageRep.h | 10 ++++++-- src/adaptation/ecap/ServiceRep.cc | 9 +++++++ src/adaptation/ecap/ServiceRep.h | 5 ++++ src/adaptation/ecap/XactionRep.cc | 19 ++++++++++----- src/adaptation/ecap/XactionRep.h | 9 ++++--- 9 files changed, 101 insertions(+), 28 deletions(-) diff --git a/configure.ac b/configure.ac index 6054c881a9..26804fa2e5 100644 --- a/configure.ac +++ b/configure.ac @@ -886,12 +886,41 @@ AS_IF([test "x$enable_ecap" != "xno"],[ ]) SQUID_STATE_SAVE(squid_ecap_state) + CPPFLAGS="$EXT_LIBECAP_CFLAGS $CPPFLAGS" + LIBS="$EXT_LIBECAP_LIBS $LIBS" + AC_CHECK_HEADERS([ \ + libecap/adapter/service.h \ + libecap/adapter/xaction.h \ + libecap/common/area.h \ + libecap/common/body.h \ + libecap/common/delay.h \ + libecap/common/forward.h \ + libecap/common/header.h \ + libecap/common/memory.h \ + libecap/common/message.h \ + libecap/common/name.h \ + libecap/common/named_values.h \ + libecap/common/names.h \ + libecap/common/options.h \ + libecap/common/registry.h \ + libecap/common/version.h \ + libecap/host/host.h \ + libecap/host/xaction.h \ + ],,,[ +/* libecap-1.0.1 headers do not build without autoconf.h magic */ +#define HAVE_CONFIG_H +/* libecap/common/delay.h fails to include */ +#include + ]) AC_MSG_CHECKING([whether -lecap will link]) - CXXFLAGS="$CXXFLAGS $EXT_LIBECAP_CFLAGS" - LIBS="$LIBS $EXT_LIBECAP_LIBS" - AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include ]],[[ - const libecap::Name test("test", libecap::Name::NextId()); - ]])],[ + AC_LINK_IFELSE([AC_LANG_PROGRAM([[ +#if HAVE_LIBECAP_COMMON_NAMES_H +#include +#endif + ]],[[ + const libecap::Name test("test", libecap::Name::NextId()); + ]]) + ],[ AC_MSG_RESULT(yes) squid_opt_use_adaptation=yes ],[ diff --git a/src/adaptation/ecap/Host.cc b/src/adaptation/ecap/Host.cc index defa5a1f26..46a7a8b661 100644 --- a/src/adaptation/ecap/Host.cc +++ b/src/adaptation/ecap/Host.cc @@ -9,9 +9,6 @@ /* DEBUG: section 93 eCAP Interface */ #include "squid.h" -#include -#include -#include #include "adaptation/ecap/Host.h" #include "adaptation/ecap/MessageRep.h" #include "adaptation/ecap/ServiceRep.h" @@ -20,6 +17,16 @@ #include "HttpRequest.h" #include "MasterXaction.h" +#if HAVE_LIBECAP_ADAPTER_SERVICE_H +#include +#endif +#if HAVE_LIBECAP_COMMON_NAMES_H +#include +#endif +#if HAVE_LIBECAP_COMMON_REGISTRY_H +#include +#endif + const libecap::Name Adaptation::Ecap::protocolInternal("internal", libecap::Name::NextId()); const libecap::Name Adaptation::Ecap::protocolIcp("ICP", libecap::Name::NextId()); #if USE_HTCP diff --git a/src/adaptation/ecap/Host.h b/src/adaptation/ecap/Host.h index 11fc67ac10..d84b5c86e4 100644 --- a/src/adaptation/ecap/Host.h +++ b/src/adaptation/ecap/Host.h @@ -11,7 +11,9 @@ #ifndef SQUID_SRC_ADAPTATION_ECAP_HOST_H #define SQUID_SRC_ADAPTATION_ECAP_HOST_H +#if HAVE_LIBECAP_HOST_HOST_H #include +#endif namespace Adaptation { diff --git a/src/adaptation/ecap/MessageRep.cc b/src/adaptation/ecap/MessageRep.cc index 1c4bdfd298..5a24732d35 100644 --- a/src/adaptation/ecap/MessageRep.cc +++ b/src/adaptation/ecap/MessageRep.cc @@ -9,19 +9,24 @@ /* DEBUG: section 93 eCAP Interface */ #include "squid.h" -#include "BodyPipe.h" -#include "HttpReply.h" -#include "HttpRequest.h" -#include -#include -#include -#include -#include "adaptation/ecap/Host.h" /* for protocol constants */ +#include "adaptation/ecap/Host.h" #include "adaptation/ecap/MessageRep.h" #include "adaptation/ecap/XactionRep.h" #include "base/TextException.h" +#include "HttpReply.h" -/* HeaderRep */ +#if HAVE_LIBECAP_COMMON_AREA_H +#include +#endif +#if HAVE_LIBECAP_COMMON_NAMED_VALUES_H +#include +#endif +#if HAVE_LIBECAP_COMMON_NAMES_H +#include +#endif +#if HAVE_LIBECAP_COMMON_VERSION_H +#include +#endif Adaptation::Ecap::HeaderRep::HeaderRep(Http::Message &aMessage): theHeader(aMessage.header), theMessage(aMessage) diff --git a/src/adaptation/ecap/MessageRep.h b/src/adaptation/ecap/MessageRep.h index fb13706386..eed9dbfdec 100644 --- a/src/adaptation/ecap/MessageRep.h +++ b/src/adaptation/ecap/MessageRep.h @@ -18,9 +18,15 @@ #include "http/forward.h" #include "HttpHeader.h" -#include -#include +#if HAVE_LIBECAP_COMMON_BODY_H #include +#endif +#if HAVE_LIBECAP_COMMON_HEADER_H +#include +#endif +#if HAVE_LIBECAP_COMMON_MESSAGE_H +#include +#endif namespace Adaptation { diff --git a/src/adaptation/ecap/ServiceRep.cc b/src/adaptation/ecap/ServiceRep.cc index dac4a1f92d..1d3339020f 100644 --- a/src/adaptation/ecap/ServiceRep.cc +++ b/src/adaptation/ecap/ServiceRep.cc @@ -18,10 +18,19 @@ #include "debug/Stream.h" #include "EventLoop.h" +#if HAVE_LIBECAP_ADAPTER_SERVICE_H #include +#endif +#if HAVE_LIBECAP_COMMON_OPTIONS_H #include +#endif +#if HAVE_LIBECAP_COMMON_NAME_H #include +#endif +#if HAVE_LIBECAP_COMMON_NAMED_VALUES_H #include +#endif + #include #include diff --git a/src/adaptation/ecap/ServiceRep.h b/src/adaptation/ecap/ServiceRep.h index 1fa7affd3c..febbb3a284 100644 --- a/src/adaptation/ecap/ServiceRep.h +++ b/src/adaptation/ecap/ServiceRep.h @@ -13,8 +13,13 @@ #include "adaptation/forward.h" #include "adaptation/Service.h" + +#if HAVE_LIBECAP_COMMON_FORWARD_H #include +#endif +#if HAVE_LIBECAP_COMMON_MEMORY_H #include +#endif namespace Adaptation { diff --git a/src/adaptation/ecap/XactionRep.cc b/src/adaptation/ecap/XactionRep.cc index 264faac87b..c167cd6acf 100644 --- a/src/adaptation/ecap/XactionRep.cc +++ b/src/adaptation/ecap/XactionRep.cc @@ -9,11 +9,6 @@ /* DEBUG: section 93 eCAP Interface */ #include "squid.h" -#include -#include -#include -#include -#include #include "adaptation/Answer.h" #include "adaptation/ecap/Config.h" #include "adaptation/ecap/XactionRep.h" @@ -22,9 +17,21 @@ #include "base/TextException.h" #include "format/Format.h" #include "HttpReply.h" -#include "HttpRequest.h" #include "MasterXaction.h" +#if HAVE_LIBECAP_COMMON_AREA_H +#include +#endif +#if HAVE_LIBECAP_COMMON_DELAY_H +#include +#endif +#if HAVE_LIBECAP_COMMON_NAMED_VALUES_H +#include +#endif +#if HAVE_LIBECAP_COMMON_NAMES_H +#include +#endif + CBDATA_NAMESPACED_CLASS_INIT(Adaptation::Ecap::XactionRep, XactionRep); /// a libecap Visitor for converting adapter transaction options to HttpHeader diff --git a/src/adaptation/ecap/XactionRep.h b/src/adaptation/ecap/XactionRep.h index cc43b37179..a3a622ecc3 100644 --- a/src/adaptation/ecap/XactionRep.h +++ b/src/adaptation/ecap/XactionRep.h @@ -16,10 +16,13 @@ #include "adaptation/Initiate.h" #include "adaptation/Message.h" #include "BodyPipe.h" -#include -#include -#include + +#if HAVE_LIBECAP_ADAPTER_XACTION_H #include +#endif +#if HAVE_LIBECAP_HOST_XACTION_H +#include +#endif namespace Adaptation { -- 2.39.2 From 47a3129cebf5a0cb3788c46b8052b7ff461c17b6 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Fri, 5 Apr 2024 13:58:27 +0000 Subject: [PATCH 07/16] Maintenance: Remove several unused files (#1773) --- src/adaptation/ecap/Makefile.am | 1 - src/adaptation/ecap/MinimalAdapter.cc | 11 ----------- src/adaptation/ecap/Registry.h | 15 --------------- 3 files changed, 27 deletions(-) delete mode 100644 src/adaptation/ecap/MinimalAdapter.cc delete mode 100644 src/adaptation/ecap/Registry.h diff --git a/src/adaptation/ecap/Makefile.am b/src/adaptation/ecap/Makefile.am index 9045a0c0a8..4fc7f12ed2 100644 --- a/src/adaptation/ecap/Makefile.am +++ b/src/adaptation/ecap/Makefile.am @@ -16,7 +16,6 @@ libecapsquid_la_SOURCES = \ Host.h \ MessageRep.cc \ MessageRep.h \ - Registry.h \ ServiceRep.cc \ ServiceRep.h \ XactionRep.cc \ diff --git a/src/adaptation/ecap/MinimalAdapter.cc b/src/adaptation/ecap/MinimalAdapter.cc deleted file mode 100644 index 5c8c9313ad..0000000000 --- a/src/adaptation/ecap/MinimalAdapter.cc +++ /dev/null @@ -1,11 +0,0 @@ -/* - * Copyright (C) 1996-2023 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#include "squid.h" -// TBD - diff --git a/src/adaptation/ecap/Registry.h b/src/adaptation/ecap/Registry.h deleted file mode 100644 index 62172ae668..0000000000 --- a/src/adaptation/ecap/Registry.h +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright (C) 1996-2023 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#ifndef SQUID_SRC_ADAPTATION_ECAP_REGISTRY_H -#define SQUID_SRC_ADAPTATION_ECAP_REGISTRY_H - -// TBD - -#endif /* SQUID_SRC_ADAPTATION_ECAP_REGISTRY_H */ - -- 2.39.2 From 597b7f1283f9aa602efd1d8edbcdae0f17f26b52 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Sun, 7 Apr 2024 11:34:49 +0000 Subject: [PATCH 08/16] Have SQUID_CHECK_LIB_WORKS do state SAVE/RESTORE (#1774) Removing a lot of duplicated code and further simplifying library detection. --- acinclude/squid-util.m4 | 13 +++++++++---- configure.ac | 29 ----------------------------- 2 files changed, 9 insertions(+), 33 deletions(-) diff --git a/acinclude/squid-util.m4 b/acinclude/squid-util.m4 index c71f975aac..4a33cce7f3 100644 --- a/acinclude/squid-util.m4 +++ b/acinclude/squid-util.m4 @@ -217,14 +217,19 @@ AC_DEFUN([SQUID_YESNO],[ dnl Check that a library is actually available, useable, dnl and where its pieces are (eg headers and hack macros) dnl Parameters for this macro are: -dnl 1) library name (without 'lib' prefix) -dnl 2) necessary library checks (to be executed by this macro unless the use of the library is disabled) -dnl These checks should set LIBFOO_LIBS automake variable (on success) -dnl and ensure that it is empty or unset (on failures). +dnl 1) library name without 'lib' prefix +dnl 2) necessary library checks to be executed by this macro +dnl - These checks are not run when library use is explicitly disabled. +dnl - These checks should set LIBFOO_LIBS automake variable on success +dnl and ensure that it is empty or unset on failures. +dnl - These checks may set or change LIBS and xxFLAGS variables as needed. +dnl This macro restores those variables afterward (see SQUID_STATE_SAVE for details). AC_DEFUN([SQUID_CHECK_LIB_WORKS],[ AH_TEMPLATE(m4_toupper(m4_translit([HAVE_LIB$1], [-+.], [___])),[Define as 1 to enable '$1' library support.]) AS_IF([m4_translit([test "x$with_$1" != "xno"], [-+.], [___])],[ + SQUID_STATE_SAVE(check_lib_works_state) $2 + SQUID_STATE_ROLLBACK(check_lib_works_state) AS_IF([! test -z m4_toupper(m4_translit(["$LIB$1_LIBS"], [-+.], [___]))],[ m4_toupper(m4_translit([CPPFLAGS="$LIB$1_CFLAGS $CPPFLAGS"], [-+.], [___])) m4_toupper(m4_translit([LIB$1_LIBS="$LIB$1_PATH $LIB$1_LIBS"], [-+.], [___])) diff --git a/configure.ac b/configure.ac index 26804fa2e5..f429725822 100644 --- a/configure.ac +++ b/configure.ac @@ -394,7 +394,6 @@ SQUID_AUTO_LIB(dl,[dynamic linking],[LIBDL]) SQUID_CHECK_LIB_WORKS(dl,[ LDFLAGS="$LIBDL_PATH $LDFLAGS" AC_CHECK_LIB(dl, dlopen,[LIBDL_LIBS="-ldl"]) - LIBS="$LIBDL_LIBS $LIBS" ]) AC_DEFUN([LIBATOMIC_CHECKER],[ @@ -432,7 +431,6 @@ AC_SUBST(ATOMICLIB) SQUID_AUTO_LIB(psapi,[Windows PSAPI.dll],[LIBPSAPI]) SQUID_CHECK_LIB_WORKS(psapi,[ - SQUID_STATE_SAVE(squid_psapi_state) CPPFLAGS="$LIBPSAPI_CFLAGS $CPPFLAGS" LIBS="$LIBPSAPI_PATH $LIBS" AC_CHECK_LIB([psapi],[K32GetProcessMemoryInfo],[ @@ -443,7 +441,6 @@ SQUID_CHECK_LIB_WORKS(psapi,[ #endif ]) ]) - SQUID_STATE_ROLLBACK(squid_psapi_state) ]) AC_SEARCH_LIBS([shm_open], [rt]) @@ -834,21 +831,17 @@ AC_MSG_NOTICE([Enable ESI processor: ${enable_esi:=no (auto)}]) # ESI support libraries: expat SQUID_AUTO_LIB(expat,[ESI expat library],[LIBEXPAT]) SQUID_CHECK_LIB_WORKS(expat,[ - SQUID_STATE_SAVE(squid_expat_state) PKG_CHECK_MODULES([LIBEXPAT],[expat],[:],[:]) CPPFLAGS="$LIBEXPAT_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(expat.h) - SQUID_STATE_ROLLBACK(squid_expat_state) ]) # ESI support libraries: xml2 SQUID_AUTO_LIB(xml2,[ESI xml2 library],[LIBXML2]) SQUID_CHECK_LIB_WORKS(xml2,[ - SQUID_STATE_SAVE([squid_libxml2_save]) PKG_CHECK_MODULES([LIBXML2],[libxml-2.0],[:],[:]) CPPFLAGS="$LIBXML2_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(libxml/parser.h libxml/HTMLparser.h libxml/HTMLtree.h) - SQUID_STATE_ROLLBACK([squid_libxml2_save]) ]) AS_IF([test "x$enable_esi" = "xyes" -a "x$LIBXML2_LIBS" = "x" -a "x$LIBEXPAT_LIBS" = "x"],[ @@ -1041,12 +1034,10 @@ AC_MSG_NOTICE([HTCP support enabled: $enable_htcp]) # Cryptograhic libraries SQUID_AUTO_LIB(nettle,[Nettle crypto],[LIBNETTLE]) SQUID_CHECK_LIB_WORKS(nettle,[ - SQUID_STATE_SAVE(squid_nettle_state) PKG_CHECK_MODULES([LIBNETTLE],[nettle >= 3.4],[ CPPFLAGS="$LIBNETTLE_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(nettle/base64.h nettle/md5.h) ],[:]) - SQUID_STATE_ROLLBACK(squid_nettle_state) ]) dnl Check for libcrypt @@ -1060,12 +1051,10 @@ AC_SUBST(CRYPTLIB) SQUID_AUTO_LIB(gnutls,[GnuTLS crypto],[LIBGNUTLS]) SQUID_CHECK_LIB_WORKS(gnutls,[ - SQUID_STATE_SAVE(squid_gnutls_state) PKG_CHECK_MODULES([LIBGNUTLS],[gnutls >= 3.4.0],[ CPPFLAGS="$LIBGNUTLS_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(gnutls/gnutls.h gnutls/x509.h gnutls/abstract.h) ],[:]) - SQUID_STATE_ROLLBACK(squid_gnutls_state) ]) SSLLIB="" @@ -1158,7 +1147,6 @@ SQUID_AUTO_LIB(mit-krb5,[MIT Kerberos],[LIB_KRB5]) AH_TEMPLATE(USE_APPLE_KRB5,[Apple Kerberos support is available]) AH_TEMPLATE(USE_SOLARIS_KRB5,[Solaris Kerberos support is available]) SQUID_CHECK_LIB_WORKS(mit-krb5,[ - SQUID_STATE_SAVE(squid_mit_krb5_save) AS_IF([test "x$squid_host_os" = "xsolaris"],[ # pkg-config not available for Solaris krb5 implementation SQUID_CHECK_SOLARIS_KRB5 @@ -1179,14 +1167,12 @@ SQUID_CHECK_LIB_WORKS(mit-krb5,[ SQUID_CHECK_KRB5_FUNCS ],[:]) ]) - SQUID_STATE_ROLLBACK(squid_mit_krb5_save) ]) # Kerberos support libraries: Heimdal SQUID_AUTO_LIB(heimdal-krb5,[Heimdal Kerberos],[LIBHEIMDAL_KRB5]) SQUID_CHECK_LIB_WORKS(heimdal-krb5,[ AS_IF([test "x$LIBMIT_KRB5_LIBS" = "x"],[ - SQUID_STATE_SAVE(squid_heimdal_krb5_save) PKG_CHECK_MODULES([LIBHEIMDAL_KRB5],[heimdal-krb5 heimdal-gssapi],[ CPPFLAGS="$LIBHEIMDAL_KRB5_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(gssapi.h gssapi/gssapi.h gssapi/gssapi_krb5.h) @@ -1196,7 +1182,6 @@ SQUID_CHECK_LIB_WORKS(heimdal-krb5,[ SQUID_CHECK_KRB5_HEIMDAL_BROKEN_KRB5_H SQUID_CHECK_KRB5_FUNCS ],[:]) - SQUID_STATE_ROLLBACK(squid_heimdal_krb5_save) ]) ]) @@ -1204,7 +1189,6 @@ SQUID_CHECK_LIB_WORKS(heimdal-krb5,[ SQUID_AUTO_LIB(gss,[GNU gss],[LIBGSS]) SQUID_CHECK_LIB_WORKS(gss,[ AS_IF([test "x$LIBMIT_KRB5_LIBS" = "x" -a "x$LIBHEIMDAL_KRB5_LIBS" = "x"],[ - SQUID_STATE_SAVE(squid_gss_save) PKG_CHECK_MODULES([LIBGSS],[gss],[ CPPFLAGS="$LIBGSS_CFLAGS $CPPFLAGS" AC_CHECK_HEADERS(gss.h) @@ -1216,13 +1200,11 @@ SQUID_CHECK_LIB_WORKS(gss,[ SQUID_CHECK_WORKING_KRB5 SQUID_DEFINE_BOOL(HAVE_KRB5,$squid_cv_working_krb5,[KRB5 support]) ],[:]) - SQUID_STATE_ROLLBACK(squid_gss_save) ]) ]) SQUID_AUTO_LIB(ldap,[LDAP],[LIBLDAP]) SQUID_CHECK_LIB_WORKS(ldap,[ - SQUID_STATE_SAVE(squid_ldap_state) PKG_CHECK_MODULES([LIBLDAP],[ldap],[:],[:]) AS_IF([test "$squid_host_os" = "mingw" -a "x$LIBLDAP_LIBS" = "x"],[ dnl On MinGW OpenLDAP is not available, try Windows LDAP libraries @@ -1233,27 +1215,22 @@ SQUID_CHECK_LIB_WORKS(ldap,[ AC_CHECK_HEADERS(ldap.h lber.h) AC_CHECK_HEADERS(mozldap/ldap.h) SQUID_CHECK_LDAP_API - SQUID_STATE_ROLLBACK(squid_ldap_state) ]) SQUID_AUTO_LIB(sasl,[Cyrus SASL],[LIBSASL]) SQUID_CHECK_LIB_WORKS([sasl],[ - SQUID_STATE_SAVE(sasl_state) PKG_CHECK_MODULES([LIBSASL],[libsasl2],[:],[:]) CPPFLAGS="$LIBSASL_CFLAGS $CPPFLAGS" LIBS="$LIBSASL_LIBS $LIBS" AC_CHECK_HEADERS([sasl/sasl.h sasl.h]) - SQUID_STATE_ROLLBACK(sasl_state) ]) SQUID_AUTO_LIB(systemd,[systemd API for start-up notification],[LIBSYSTEMD]) SQUID_CHECK_LIB_WORKS(systemd,[ - SQUID_STATE_SAVE(squid_systemd_state) LIBS="$LIBS $LIBSYSTEMD_PATH" PKG_CHECK_MODULES(LIBSYSTEMD,[libsystemd >= 209],[ AC_CHECK_HEADERS(systemd/sd-daemon.h) ],[:]) - SQUID_STATE_ROLLBACK(squid_systemd_state) ]) AC_ARG_ENABLE(forw-via-db, @@ -1443,7 +1420,6 @@ AC_MSG_NOTICE([Linux Netfilter support requested: ${enable_linux_netfilter:=auto dnl Look for libnetfilter_conntrack options (needed for QOS netfilter marking) SQUID_AUTO_LIB(netfilter-conntrack,[Netfilter conntrack],[LIBNETFILTER_CONNTRACK]) SQUID_CHECK_LIB_WORKS(netfilter-conntrack,[ - SQUID_STATE_SAVE(squid_netfilter_conntrack_state) PKG_CHECK_MODULES([LIBNETFILTER_CONNTRACK],[libnetfilter_conntrack],[:],[:]) CPPFLAGS="$LIBNETFILTER_CONNTRACK_CFLAGS $CPPFLAGS" LIBS="$LIBNETFILTER_CONNTRACK_PATH $LIBNETFILTER_CONNTRACK_LIBS $LIBS" @@ -1451,7 +1427,6 @@ SQUID_CHECK_LIB_WORKS(netfilter-conntrack,[ libnetfilter_conntrack/libnetfilter_conntrack.h \ libnetfilter_conntrack/libnetfilter_conntrack_tcp.h \ ]) - SQUID_STATE_ROLLBACK(squid_netfilter_conntrack_state) ]) dnl Enable Large file support @@ -1787,11 +1762,9 @@ SQUID_CHECK_LIB_WORKS(cppunit,[ AC_MSG_NOTICE([using system installed cppunit version $squid_cv_cppunit_version]) AS_UNSET(squid_cv_cppunit_version) - SQUID_STATE_SAVE(squid_cppunit_state) CXXFLAGS="${CXXFLAGS} ${LIBCPPUNIT_CFLAGS}" LIBS="${LIBS} ${LIBCPPUNIT_LIBS}" AC_CHECK_HEADERS(cppunit/extensions/HelperMacros.h) - SQUID_STATE_ROLLBACK(squid_cppunit_state) ],[ AC_MSG_WARN([cppunit does not appear to be installed. Squid does not require this, but code testing with 'make check' will fail.]) ]) @@ -2006,14 +1979,12 @@ AC_FUNC_ALLOCA SQUID_AUTO_LIB(cap,[Linux capabilities],[LIBCAP]) SQUID_CHECK_LIB_WORKS(cap,[ - SQUID_STATE_SAVE(squid_libcap_state) PKG_CHECK_MODULES([LIBCAP],[libcap >= 2.09],[:],[:]) CPPFLAGS="$LIBCAP_CFLAGS $CPPFLAGS" LIBS="$LIBCAP_PATH $LIBCAP_LIBS $LIBS" SQUID_CHECK_FUNCTIONAL_LIBCAP2 AC_MSG_NOTICE([libcap headers are ok: $squid_cv_sys_capability_works]) AS_IF([test "x$squid_cv_sys_capability_works" = "xno"],[LIBCAP_LIBS=""]) - SQUID_STATE_ROLLBACK(squid_libcap_state) ]) dnl Check for needed libraries -- 2.39.2 From 9e6c696c80946964c7b6fb3aee38d253493cb603 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Sun, 7 Apr 2024 16:51:02 +0000 Subject: [PATCH 09/16] NoNewGlobals for MemBlob::Stats (#1749) Detected by Coverity. CID 1554656: Initialization or destruction ordering is unspecified (GLOBAL_INIT_ORDER). Also update MemBlobStats initialization. --- src/sbuf/MemBlob.cc | 39 ++++++++++++++++++++++----------------- src/sbuf/MemBlob.h | 12 ++++-------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/sbuf/MemBlob.cc b/src/sbuf/MemBlob.cc index f611ff9dbf..3bb1af36b7 100644 --- a/src/sbuf/MemBlob.cc +++ b/src/sbuf/MemBlob.cc @@ -14,14 +14,10 @@ #include -MemBlobStats MemBlob::Stats; InstanceIdDefinitions(MemBlob, "blob"); /* MemBlobStats */ -MemBlobStats::MemBlobStats(): alloc(0), live(0), append(0), liveBytes(0) -{} - MemBlobStats& MemBlobStats::operator += (const MemBlobStats& s) { @@ -46,6 +42,19 @@ MemBlobStats::dump(std::ostream &os) const return os; } +static auto & +WriteableStats() +{ + static const auto stats = new MemBlobStats(); + return *stats; +} + +const MemBlobStats & +MemBlob::GetStats() +{ + return WriteableStats(); +} + /* MemBlob */ MemBlob::MemBlob(const MemBlob::size_type reserveSize) : @@ -72,8 +81,9 @@ MemBlob::~MemBlob() { if (mem || capacity) memFreeBuf(capacity, mem); - Stats.liveBytes -= capacity; - --Stats.live; + auto &stats = WriteableStats(); + stats.liveBytes -= capacity; + --stats.live; SBufStats::RecordMemBlobSizeAtDestruct(capacity); debugs(MEMBLOB_DEBUGSECTION,9, "destructed, this=" @@ -99,9 +109,10 @@ MemBlob::memAlloc(const size_type minSize) debugs(MEMBLOB_DEBUGSECTION, 8, id << " memAlloc: requested=" << minSize << ", received=" << capacity); - ++Stats.live; - ++Stats.alloc; - Stats.liveBytes += capacity; + auto &stats = WriteableStats(); + ++stats.live; + ++stats.alloc; + stats.liveBytes += capacity; } void @@ -109,7 +120,7 @@ MemBlob::appended(const size_type n) { Must(willFit(n)); size += n; - ++Stats.append; + ++WriteableStats().append; } void @@ -121,7 +132,7 @@ MemBlob::append(const char *source, const size_type n) memmove(mem + size, source, n); size += n; } - ++Stats.append; + ++WriteableStats().append; } void @@ -145,12 +156,6 @@ MemBlob::consume(const size_type rawN) } } -const MemBlobStats& -MemBlob::GetStats() -{ - return Stats; -} - std::ostream& MemBlob::dump(std::ostream &os) const { diff --git a/src/sbuf/MemBlob.h b/src/sbuf/MemBlob.h index 1932118f15..e9304b9b5b 100644 --- a/src/sbuf/MemBlob.h +++ b/src/sbuf/MemBlob.h @@ -19,18 +19,16 @@ class MemBlobStats { public: - MemBlobStats(); - /// dumps class-wide statistics std::ostream& dump(std::ostream& os) const; MemBlobStats& operator += (const MemBlobStats&); public: - uint64_t alloc; ///< number of MemBlob instances created so far - uint64_t live; ///< number of MemBlob instances currently alive - uint64_t append; ///< number of MemBlob::append() calls - uint64_t liveBytes; ///< the total size of currently allocated storage + uint64_t alloc = 0; ///< number of MemBlob instances created so far + uint64_t live = 0; ///< number of MemBlob instances currently alive + uint64_t append = 0; ///< number of MemBlob::append() calls + uint64_t liveBytes = 0; ///< the total size of currently allocated storage }; /** Refcountable, fixed-size, content-agnostic memory buffer. @@ -115,8 +113,6 @@ public: const InstanceId id; ///< blob identifier private: - static MemBlobStats Stats; ///< class-wide statistics - void memAlloc(const size_type memSize); /// whether the offset points to the end of the used area -- 2.39.2 From 1a66c4fb96df8dcc26b7973e160b42def59bedbe Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Mon, 8 Apr 2024 12:01:32 +0000 Subject: [PATCH 10/16] Refactor and improve ErrorState::Dump (#1730) Rework the internals for generating output in ErrorState::Dump, used for expanding the '%W' token in error page templates. Also fix a bug with excessive html-quoting of the output. --- src/errorpage.cc | 86 ++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/src/errorpage.cc b/src/errorpage.cc index a56998dd72..c01a2c4365 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -10,6 +10,8 @@ #include "squid.h" #include "AccessLogEntry.h" +#include "base/CharacterSet.h" +#include "base/IoManip.h" #include "cache_cf.h" #include "clients/forward.h" #include "comm/Connection.h" @@ -810,67 +812,58 @@ ErrorState::~ErrorState() int ErrorState::Dump(MemBuf * mb) { - MemBuf str; - char ntoabuf[MAX_IPSTRLEN]; + PackableStream out(*mb); + const auto &encoding = CharacterSet::RFC3986_UNRESERVED(); + + out << "?subject=" << + AnyP::Uri::Encode(SBuf("CacheErrorInfo - "),encoding) << + AnyP::Uri::Encode(SBuf(errorPageName(type)), encoding); + + SBufStream body; + body << "CacheHost: " << getMyHostname() << "\r\n" << + "ErrPage: " << errorPageName(type) << "\r\n" << + "TimeStamp: " << Time::FormatRfc1123(squid_curtime) << "\r\n" << + "\r\n"; + + body << "ClientIP: " << src_addr << "\r\n"; + + if (request && request->hier.host[0] != '\0') + body << "ServerIP: " << request->hier.host << "\r\n"; + + if (xerrno) + body << "Err: (" << xerrno << ") " << strerror(xerrno) << "\r\n"; - str.reset(); - /* email subject line */ - str.appendf("CacheErrorInfo - %s", errorPageName(type)); - mb->appendf("?subject=%s", rfc1738_escape_part(str.buf)); - str.reset(); - /* email body */ - str.appendf("CacheHost: %s\r\n", getMyHostname()); - /* - Err Msgs */ - str.appendf("ErrPage: %s\r\n", errorPageName(type)); - - if (xerrno) { - str.appendf("Err: (%d) %s\r\n", xerrno, strerror(xerrno)); - } else { - str.append("Err: [none]\r\n", 13); - } #if USE_AUTH if (auth_user_request.getRaw() && auth_user_request->denyMessage()) - str.appendf("Auth ErrMsg: %s\r\n", auth_user_request->denyMessage()); + body << "Auth ErrMsg: " << auth_user_request->denyMessage() << "\r\n"; #endif - if (dnsError) - str.appendf("DNS ErrMsg: " SQUIDSBUFPH "\r\n", SQUIDSBUFPRINT(*dnsError)); - - /* - TimeStamp */ - str.appendf("TimeStamp: %s\r\n\r\n", Time::FormatRfc1123(squid_curtime)); - /* - IP stuff */ - str.appendf("ClientIP: %s\r\n", src_addr.toStr(ntoabuf,MAX_IPSTRLEN)); + if (dnsError) + body << "DNS ErrMsg: " << *dnsError << "\r\n"; - if (request && request->hier.host[0] != '\0') { - str.appendf("ServerIP: %s\r\n", request->hier.host); - } + body << "\r\n"; - str.append("\r\n", 2); - /* - HTTP stuff */ - str.append("HTTP Request:\r\n", 15); if (request) { - str.appendf(SQUIDSBUFPH " " SQUIDSBUFPH " %s/%d.%d\n", - SQUIDSBUFPRINT(request->method.image()), - SQUIDSBUFPRINT(request->url.path()), - AnyP::ProtocolType_str[request->http_ver.protocol], - request->http_ver.major, request->http_ver.minor); - request->header.packInto(&str); + body << "HTTP Request:\r\n"; + MemBuf r; + r.init(); + request->pack(&r); + body << r.content(); } - str.append("\r\n", 2); /* - FTP stuff */ if (ftp.request) { - str.appendf("FTP Request: %s\r\n", ftp.request); - str.appendf("FTP Reply: %s\r\n", (ftp.reply? ftp.reply:"[none]")); - str.append("FTP Msg: ", 9); - wordlistCat(ftp.server_msg, &str); - str.append("\r\n", 2); + body << "FTP Request: " << ftp.request << "\r\n"; + if (ftp.reply) + body << "FTP Reply: " << ftp.reply << "\r\n"; + if (ftp.server_msg) + body << "FTP Msg: " << AsList(*ftp.server_msg).delimitedBy("\n") << "\r\n"; + body << "\r\n"; } - str.append("\r\n", 2); - mb->appendf("&body=%s", rfc1738_escape_part(str.buf)); - str.clean(); + out << "&body=" << AnyP::Uri::Encode(body.buf(), encoding); + return 0; } @@ -1203,6 +1196,7 @@ ErrorState::compileLegacyCode(Build &build) if (Config.adminEmail && Config.onoff.emailErrData) Dump(&mb); no_urlescape = 1; + do_quote = 0; break; case 'x': -- 2.39.2 From 3996c0dc867a1f0a016aa828e3928a9270735419 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Mon, 8 Apr 2024 15:24:39 +0000 Subject: [PATCH 11/16] Add AtMostOnce stream manipulator (#1742) --- src/base/IoManip.h | 43 ++++++++++++++++++++++++++++++++++++++++ src/tests/testIoManip.cc | 30 ++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/src/base/IoManip.h b/src/base/IoManip.h index 58fdd7f2be..3983c79664 100644 --- a/src/base/IoManip.h +++ b/src/base/IoManip.h @@ -220,5 +220,48 @@ operator <<(std::ostream &os, const AsList &manipulator) template inline auto asList(const Container &c) { return AsList(c); } +/// Helps print T object at most once per AtMostOnce object lifetime. +/// T objects are printed to std::ostream using operator "<<". +/// +/// \code +/// auto headerOnce = AtMostOnce("Transaction Details:\n"); +/// if (detailOne) +/// os << headerOnce << *detailOne; +/// if (const auto detailTwo = findAnotherDetail()) +/// os << headerOnce << *detailTwo; +/// \endcode +template +class AtMostOnce +{ +public: + /// caller must ensure `t` lifetime extends to the last use of this AtMostOnce instance + explicit AtMostOnce(const T &t): toPrint(t) {} + + void print(std::ostream &os) { + if (!printed) { + os << toPrint; + printed = true; + } + } + +private: + const T &toPrint; + bool printed = false; +}; + +/// Prints AtMostOnce argument if needed. The argument is not constant to +/// prevent wrong usage: +/// +/// \code +/// /* Compiler error: cannot bind non-const lvalue reference to an rvalue */ +/// os << AtMostOnce(x); +/// \endcode +template +inline auto & +operator <<(std::ostream &os, AtMostOnce &a) { + a.print(os); + return os; +} + #endif /* SQUID_SRC_BASE_IOMANIP_H */ diff --git a/src/tests/testIoManip.cc b/src/tests/testIoManip.cc index 22abc60993..80b0f024f7 100644 --- a/src/tests/testIoManip.cc +++ b/src/tests/testIoManip.cc @@ -19,10 +19,12 @@ class TestIoManip: public CPPUNIT_NS::TestFixture { CPPUNIT_TEST_SUITE(TestIoManip); CPPUNIT_TEST(testAsHex); + CPPUNIT_TEST(testAtMostOnce); CPPUNIT_TEST_SUITE_END(); protected: void testAsHex(); + void testAtMostOnce(); }; CPPUNIT_TEST_SUITE_REGISTRATION( TestIoManip ); @@ -153,6 +155,34 @@ TestIoManip::testAsHex() resetStream(ss); } +void +TestIoManip::testAtMostOnce() +{ + { + std::ostringstream ss; + auto textOnce = AtMostOnce("text1"); + ss << textOnce; + CPPUNIT_ASSERT_EQUAL(std::string("text1"), ss.str()); + ss << textOnce; + ss << textOnce; + CPPUNIT_ASSERT_EQUAL(std::string("text1"), ss.str()); + } + + { + std::ostringstream ss; + // Cannot create std::string when creating textOnce because the string may be + // destroyed before we are done with textOnce: + // auto textOnce = AtMostOnce(std::string("do not do this")); + const std::string s("text2"); + auto textOnce = AtMostOnce(s); + ss << textOnce; + CPPUNIT_ASSERT_EQUAL(std::string("text2"), ss.str()); + ss << textOnce; + ss << textOnce; + CPPUNIT_ASSERT_EQUAL(std::string("text2"), ss.str()); + } +} + int main(int argc, char *argv[]) { -- 2.39.2 From e3aef579a39219309993d6ae24014b929b36aee1 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Mon, 8 Apr 2024 18:50:51 +0000 Subject: [PATCH 12/16] NoNewGlobals for HttpHdrCc:ccLookupTable (#1750) Detected by Coverity. CID 1554655: Initialization or destruction ordering is unspecified (GLOBAL_INIT_ORDER). Also switched to compile-time checks for table initialization records. --- src/HttpHdrCc.cc | 69 ++++++++++++++++++++++++++--------------- src/HttpHdrCc.h | 1 - src/HttpHeader.cc | 1 - src/base/EnumIterator.h | 15 +++++++++ 4 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/HttpHdrCc.cc b/src/HttpHdrCc.cc index b95b133e61..2bd9303208 100644 --- a/src/HttpHdrCc.cc +++ b/src/HttpHdrCc.cc @@ -9,6 +9,7 @@ /* DEBUG: section 65 HTTP Cache Control Header */ #include "squid.h" +#include "base/EnumIterator.h" #include "base/LookupTable.h" #include "HttpHdrCc.h" #include "HttpHeader.h" @@ -16,6 +17,7 @@ #include "HttpHeaderStat.h" #include "HttpHeaderTools.h" #include "sbuf/SBuf.h" +#include "SquidMath.h" #include "StatHist.h" #include "Store.h" #include "StrList.h" @@ -23,10 +25,10 @@ #include #include +#include #include -// invariant: row[j].id == j -static LookupTable::Record CcAttrs[] = { +constexpr LookupTable::Record attrsList[] = { {"public", HttpHdrCcType::CC_PUBLIC}, {"private", HttpHdrCcType::CC_PRIVATE}, {"no-cache", HttpHdrCcType::CC_NO_CACHE}, @@ -44,7 +46,40 @@ static LookupTable::Record CcAttrs[] = { {"Other,", HttpHdrCcType::CC_OTHER}, /* ',' will protect from matches */ {nullptr, HttpHdrCcType::CC_ENUM_END} }; -LookupTable ccLookupTable(HttpHdrCcType::CC_OTHER,CcAttrs); + +constexpr const auto & +CcAttrs() { + // TODO: Move these compile-time checks into LookupTable + ConstexprForEnum([](const auto ev) { + const auto idx = static_cast::type>(ev); + // invariant: each row has a name except the last one + static_assert(!attrsList[idx].name == (ev == HttpHdrCcType::CC_ENUM_END)); + // invariant: row[idx].id == idx + static_assert(attrsList[idx].id == ev); + }); + return attrsList; +} + +static auto +ccTypeByName(const SBuf &name) { + const static auto table = new LookupTable(HttpHdrCcType::CC_OTHER, CcAttrs()); + return table->lookup(name); +} + +/// Safely converts an integer into a Cache-Control directive name. +/// \returns std::nullopt if the given integer is not a valid index of a named attrsList entry +template +static std::optional +ccNameByType(const RawId rawId) +{ + // TODO: Store a by-ID index in (and move this functionality into) LookupTable. + if (!Less(rawId, 0) && Less(rawId, int(HttpHdrCcType::CC_ENUM_END))) { + const auto idx = static_cast::type>(rawId); + return CcAttrs()[idx].name; + } + return std::nullopt; +} + std::vector ccHeaderStats(HttpHdrCcType::CC_ENUM_END); /// used to walk a table of http_header_cc_type structs @@ -56,16 +91,6 @@ operator++ (HttpHdrCcType &aHeader) return aHeader; } -/// Module initialization hook -void -httpHdrCcInitModule(void) -{ - // check invariant on initialization table - for (unsigned int j = 0; CcAttrs[j].name != nullptr; ++j) { - assert(static_cast(CcAttrs[j].id) == j); - } -} - void HttpHdrCc::clear() { @@ -113,7 +138,7 @@ HttpHdrCc::parse(const String & str) } /* find type */ - const HttpHdrCcType type = ccLookupTable.lookup(SBuf(item,nlen)); + const auto type = ccTypeByName(SBuf(item, nlen)); // ignore known duplicate directives if (isSet(type)) { @@ -258,7 +283,7 @@ HttpHdrCc::packInto(Packable * p) const if (isSet(flag) && flag != HttpHdrCcType::CC_OTHER) { /* print option name for all options */ - p->appendf((pcount ? ", %s": "%s"), CcAttrs[flag].name); + p->appendf((pcount ? ", %s": "%s"), *ccNameByType(flag)); /* for all options having values, "=value" after the name */ switch (flag) { @@ -332,22 +357,16 @@ httpHdrCcStatDumper(StoreEntry * sentry, int, double val, double, int count) { extern const HttpHeaderStat *dump_stat; /* argh! */ const int id = static_cast(val); - const bool valid_id = id >= 0 && id < static_cast(HttpHdrCcType::CC_ENUM_END); - const char *name = valid_id ? CcAttrs[id].name : "INVALID"; - - if (count || valid_id) + const auto name = ccNameByType(id); + if (count || name) storeAppendPrintf(sentry, "%2d\t %-20s\t %5d\t %6.2f\n", - id, name, count, xdiv(count, dump_stat->ccParsedCount)); + id, name.value_or("INVALID"), count, xdiv(count, dump_stat->ccParsedCount)); } std::ostream & operator<< (std::ostream &s, HttpHdrCcType c) { - const unsigned char ic = static_cast(c); - if (c < HttpHdrCcType::CC_ENUM_END) - s << CcAttrs[ic].name << '[' << ic << ']' ; - else - s << "*invalid hdrcc* [" << ic << ']'; + s << ccNameByType(c).value_or("INVALID") << '[' << static_cast(c) << ']'; return s; } diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h index b192b59ec6..7feec5a58d 100644 --- a/src/HttpHdrCc.h +++ b/src/HttpHdrCc.h @@ -210,7 +210,6 @@ public: class StatHist; class StoreEntry; -void httpHdrCcInitModule(void); void httpHdrCcUpdateStats(const HttpHdrCc * cc, StatHist * hist); void httpHdrCcStatDumper(StoreEntry * sentry, int idx, double val, double size, int count); diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index 1a52b992c3..a251328c3d 100644 --- a/src/HttpHeader.cc +++ b/src/HttpHeader.cc @@ -135,7 +135,6 @@ httpHeaderInitModule(void) assert(HttpHeaderStats[hoEnd-1].label && "HttpHeaderStats created with all elements"); /* init dependent modules */ - httpHdrCcInitModule(); httpHdrScInitModule(); httpHeaderRegisterWithCacheManager(); diff --git a/src/base/EnumIterator.h b/src/base/EnumIterator.h index 134b76f293..02b3e13196 100644 --- a/src/base/EnumIterator.h +++ b/src/base/EnumIterator.h @@ -224,5 +224,20 @@ public: WholeEnum() : EnumRangeT(EnumType::enumBegin_, EnumType::enumEnd_) {} }; +/// Compile-time iteration of sequential enum values from First to Last, +/// inclusive. The given function is called once on each iteration, with the +/// current enum value as an argument. +template +constexpr void +ConstexprForEnum(F &&f) +{ + using enumType = decltype(First); + using underlyingType = typename std::underlying_type::type; + // std::integral_constant trick makes f(First) argument a constexpr + f(std::integral_constant()); // including when First is Last + if constexpr (First < Last) + ConstexprForEnum(First) + 1), Last>(f); +} + #endif /* SQUID_SRC_BASE_ENUMITERATOR_H */ -- 2.39.2 From 25aa6c9a505f0b0f8a2c68b10d7a0933680f3772 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 8 Apr 2024 22:06:02 +0000 Subject: [PATCH 13/16] Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766) AclMatchedName global has been localized into a regular Acl::Answer data member (Acl::Answer maintains the result of ACLChecklist evaluations). This long overdue change resolves an old TODO and XXXs, paving the way for Acl::Node reference counting. No significant functionality changes are expected, but it is possible that some deny_info configurations will now be handled better in reconfiguration corner cases (because Squid no longer forgets the name of the last checked ACL when a slow ACL check crosses reconfiguration barrier). Most of these changes are performance-neutral or -positive because they eliminate or reduce memory allocations and associated name copying (and more reduction will become possible after upgrading squid.conf parsers to use SBuf). This change adds SBuf object copies when Acl::Answer is propagated to ACLCB callbacks, but those read-only copies are cheap. Also renamed and polished aclGetDenyInfoPage() because we had to update its parameter type (to supply the last evaluated ACL). All callers were also supplying the same first argument (that is unlikely to change in the foreseeable future); that argument is now gone. We did not fix the redirect_allowed name and debugs(): Fixing that behavior deserves a dedicated change. Also polished legacy aclIsProxyAuth() profile and description because we have to change the parameter type (to supply the last evaluated ACL). Also removed 63-character aclname parameter limit for acl directives. --- src/AccessLogEntry.cc | 2 - src/AccessLogEntry.h | 2 +- src/FwdState.cc | 4 +- src/acl/Acl.cc | 69 +++++++++++++++++-------------- src/acl/Acl.h | 17 ++++---- src/acl/AllOf.cc | 15 ++----- src/acl/BoolOps.cc | 9 ++-- src/acl/Checklist.cc | 5 ++- src/acl/Checklist.h | 10 ++++- src/acl/Gadgets.cc | 55 ++++++++---------------- src/acl/Gadgets.h | 14 +++++-- src/acl/InnerNode.cc | 4 +- src/acl/Node.h | 17 ++++++-- src/acl/forward.h | 2 - src/adaptation/Answer.cc | 11 ++++- src/adaptation/Answer.h | 11 +++-- src/adaptation/ecap/XactionRep.cc | 3 +- src/cache_cf.cc | 10 ++--- src/client_side_reply.cc | 5 +-- src/client_side_request.cc | 18 ++------ src/clients/Client.cc | 7 ++-- src/external_acl.cc | 10 ++--- src/format/Format.cc | 3 +- src/icp_v2.cc | 14 ++++--- src/tests/stub_acl.cc | 3 +- 25 files changed, 162 insertions(+), 158 deletions(-) diff --git a/src/AccessLogEntry.cc b/src/AccessLogEntry.cc index dfd03d6d3b..8481b37c80 100644 --- a/src/AccessLogEntry.cc +++ b/src/AccessLogEntry.cc @@ -133,8 +133,6 @@ AccessLogEntry::~AccessLogEntry() safe_free(headers.adapted_request); HTTPMSGUNLOCK(adapted_request); - safe_free(lastAclName); - HTTPMSGUNLOCK(request); #if ICAP_CLIENT HTTPMSGUNLOCK(icap.reply); diff --git a/src/AccessLogEntry.h b/src/AccessLogEntry.h index eb51087398..cab6ae8b00 100644 --- a/src/AccessLogEntry.h +++ b/src/AccessLogEntry.h @@ -188,7 +188,7 @@ public: } adapt; #endif - const char *lastAclName = nullptr; ///< string for external_acl_type %ACL format code + SBuf lastAclName; ///< string for external_acl_type %ACL format code SBuf lastAclData; ///< string for external_acl_type %DATA format code HierarchyLogEntry hier; diff --git a/src/FwdState.cc b/src/FwdState.cc index 34cbd873ca..94d9610437 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -353,9 +353,7 @@ FwdState::Start(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, Ht ch.src_addr = request->client_addr; ch.syncAle(request, nullptr); if (ch.fastCheck().denied()) { - err_type page_id; - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); - + auto page_id = FindDenyInfoPage(ch.currentAnswer(), true); if (page_id == ERR_NONE) page_id = ERR_FORWARDING_DENIED; diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index d2e9c5faec..b08ab442e5 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -27,8 +27,6 @@ #include #include -const char *AclMatchedName = nullptr; - namespace Acl { /// Acl::Node type name comparison functor @@ -115,6 +113,14 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey) Here()); } +const SBuf & +Acl::Answer::lastCheckDescription() const +{ + static const auto none = new SBuf("[no-ACL]"); + // no value_or() because it would create a new SBuf object here + return lastCheckedName ? *lastCheckedName : *none; +} + /* Acl::ParsingContext */ ScopedId @@ -143,27 +149,39 @@ void Acl::Node::operator delete(void *) fatal ("unusable Acl::Node::delete"); } -Acl::Node * -Acl::Node::FindByName(const char *name) +/// implements both Acl::Node::FindByName() variations +template +static Acl::Node * +FindByNameT(const SBufOrCString name) { - debugs(28, 9, "name=" << name); - - for (auto *a = Config.aclList; a; a = a->next) - if (!strcasecmp(a->name, name)) + for (auto a = Config.aclList; a; a = a->next) { + if (a->name.caseCmp(name) == 0) { + debugs(28, 8, "found " << a->name); return a; + } + } - debugs(28, 9, "found no match"); - + debugs(28, 8, "cannot find " << name); return nullptr; } +Acl::Node * +Acl::Node::FindByName(const SBuf &name) +{ + return FindByNameT(name); +} + +Acl::Node * +Acl::Node::FindByName(const char *name) +{ + return FindByNameT(name); +} + Acl::Node::Node() : cfgline(nullptr), next(nullptr), registered(false) -{ - *name = 0; -} +{} bool Acl::Node::valid() const @@ -176,10 +194,7 @@ Acl::Node::matches(ACLChecklist *checklist) const { debugs(28, 5, "checking " << name); - // XXX: AclMatchedName does not contain a matched ACL name when the acl - // does not match. It contains the last (usually leaf) ACL name checked - // (or is NULL if no ACLs were checked). - AclMatchedName = name; + checklist->setLastCheckedName(name); int result = 0; if (!checklist->hasAle() && requiresAle()) { @@ -206,11 +221,9 @@ Acl::Node::matches(ACLChecklist *checklist) const } void -Acl::Node::context(const char *aName, const char *aCfgLine) +Acl::Node::context(const SBuf &aName, const char *aCfgLine) { - name[0] = '\0'; - if (aName) - xstrncpy(name, aName, ACL_NAME_SZ-1); + name = aName; safe_free(cfgline); if (aCfgLine) cfgline = xstrdup(aCfgLine); @@ -230,22 +243,15 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) return; } - if (strlen(t) >= ACL_NAME_SZ) { - debugs(28, DBG_CRITICAL, "aclParseAclLine: aclParseAclLine: ACL name '" << t << - "' too long, max " << ACL_NAME_SZ - 1 << " characters supported"); - parser.destruct(); - return; - } - SBuf aclname(t); CallParser(ParsingContext::Pointer::Make(aclname), [&] { - ParseNamed(parser, head, aclname.c_str()); // TODO: Convert Node::name to SBuf + ParseNamed(parser, head, aclname); }); } /// parses acl directive parts that follow aclname void -Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * const aclname) +Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &aclname) { /* snarf the ACL type */ const char *theType; @@ -278,7 +284,7 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * con } theType = "localport"; debugs(28, DBG_IMPORTANT, "WARNING: UPGRADE: ACL 'myport' type has been renamed to 'localport' and matches the port the client connected to."); - } else if (strcmp(theType, "proto") == 0 && strcmp(aclname, "manager") == 0) { + } else if (strcmp(theType, "proto") == 0 && aclname.cmp("manager") == 0) { // ACL manager is now a built-in and has a different type. debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: UPGRADE: ACL 'manager' is now a built-in ACL. Remove it from your config file."); return; // ignore the line @@ -446,7 +452,6 @@ Acl::Node::~Node() { debugs(28, 3, "freeing ACL " << name); safe_free(cfgline); - AclMatchedName = nullptr; // in case it was pointing to our name } void diff --git a/src/acl/Acl.h b/src/acl/Acl.h index f8345efe28..130682b2c4 100644 --- a/src/acl/Acl.h +++ b/src/acl/Acl.h @@ -12,9 +12,10 @@ #include "acl/forward.h" #include "defines.h" #include "dlink.h" -#include "sbuf/forward.h" +#include "sbuf/SBuf.h" #include +#include #include namespace Acl { @@ -66,7 +67,7 @@ public: return !(*this == aCode); } - bool operator ==(const Answer allow) const { + bool operator ==(const Answer &allow) const { return code == allow.code && kind == allow.kind; } @@ -89,6 +90,9 @@ public: /// whether Squid is uncertain about the allowed() or denied() answer bool conflicted() const { return !allowed() && !denied(); } + /// describes the ACL that was evaluated last while obtaining this answer (for debugging) + const SBuf &lastCheckDescription() const; + aclMatchCode code = ACCESS_DUNNO; ///< ACCESS_* code /// the matched custom access list verb (or zero) @@ -96,10 +100,13 @@ public: /// whether we were computed by the "negate the last explicit action" rule bool implicit = false; + + /// the name of the ACL (if any) that was evaluated last while obtaining this answer + std::optional lastCheckedName; }; inline std::ostream & -operator <<(std::ostream &o, const Answer a) +operator <<(std::ostream &o, const Answer &a) { switch (a) { case ACCESS_DENIED: @@ -136,9 +143,5 @@ public: void *acl_data; }; -/// \ingroup ACLAPI -/// XXX: find a way to remove or at least use a refcounted Acl::Node pointer -extern const char *AclMatchedName; /* NULL */ - #endif /* SQUID_SRC_ACL_ACL_H */ diff --git a/src/acl/AllOf.cc b/src/acl/AllOf.cc index b9e92b50e2..731f56781b 100644 --- a/src/acl/AllOf.cc +++ b/src/acl/AllOf.cc @@ -14,6 +14,7 @@ #include "cache_cf.h" #include "MemBuf.h" #include "sbuf/SBuf.h" +#include "sbuf/Stream.h" char const * Acl::AllOf::typeString() const @@ -56,13 +57,8 @@ Acl::AllOf::parse() } else if (oldNode) { // this acl saw a single line before; create a new OR inner node - MemBuf wholeCtx; - wholeCtx.init(); - wholeCtx.appendf("(%s lines)", name); - wholeCtx.terminate(); - Acl::OrNode *newWhole = new Acl::OrNode; - newWhole->context(wholeCtx.content(), oldNode->cfgline); + newWhole->context(ToSBuf('(', name, " lines)"), oldNode->cfgline); newWhole->add(oldNode); // old (i.e. first) line nodes.front() = whole = newWhole; aclRegister(newWhole); @@ -74,13 +70,8 @@ Acl::AllOf::parse() assert(whole); const int lineId = whole->childrenCount() + 1; - MemBuf lineCtx; - lineCtx.init(); - lineCtx.appendf("(%s line #%d)", name, lineId); - lineCtx.terminate(); - Acl::AndNode *line = new AndNode; - line->context(lineCtx.content(), config_input_line); + line->context(ToSBuf('(', name, " line #", lineId, ')'), config_input_line); line->lineParse(); whole->add(line); diff --git a/src/acl/BoolOps.cc b/src/acl/BoolOps.cc index e117965422..e418c567b2 100644 --- a/src/acl/BoolOps.cc +++ b/src/acl/BoolOps.cc @@ -17,10 +17,9 @@ Acl::NotNode::NotNode(Acl::Node *acl) { assert(acl); - Must(strlen(acl->name) <= sizeof(name)-2); - name[0] = '!'; - name[1] = '\0'; - xstrncpy(&name[1], acl->name, sizeof(name)-1); // -1 for '!' + name.reserveCapacity(1 + acl->name.length()); + name.append('!'); + name.append(acl->name); add(acl); } @@ -56,7 +55,7 @@ SBufList Acl::NotNode::dump() const { SBufList text; - text.push_back(SBuf(name)); + text.push_back(name); return text; } diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index 3e697881f6..c04014500c 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -60,6 +60,7 @@ ACLChecklist::markFinished(const Acl::Answer &finalAnswer, const char *reason) assert (!finished() && !asyncInProgress()); finished_ = true; answer_ = finalAnswer; + answer_.lastCheckedName = lastCheckedName_; debugs(28, 3, this << " answer " << answer_ << " for " << reason); } @@ -74,7 +75,7 @@ ACLChecklist::preCheck(const char *what) occupied_ = true; asyncLoopDepth_ = 0; - AclMatchedName = nullptr; + lastCheckedName_.reset(); finished_ = false; } @@ -154,7 +155,7 @@ ACLChecklist::goAsync(AsyncStarter starter, const Acl::Node &acl) // ACLFilledChecklist overwrites this to unclock something before we // "delete this" void -ACLChecklist::checkCallback(Acl::Answer answer) +ACLChecklist::checkCallback(const Acl::Answer &answer) { ACLCB *callback_; void *cbdata_; diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 0ee5844114..3f7fcd96bf 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -12,6 +12,8 @@ #include "acl/Acl.h" #include "acl/InnerNode.h" #include "cbdata.h" + +#include #include #include @@ -152,9 +154,12 @@ public: return old; } + /// remember the name of the last ACL being evaluated + void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; } + private: /// Calls non-blocking check callback with the answer and destroys self. - void checkCallback(Acl::Answer answer); + void checkCallback(const Acl::Answer &answer); void matchAndFinish(); @@ -209,6 +214,9 @@ private: /* internal methods */ std::stack matchPath; /// the list of actions which must ignored during acl checks std::vector bannedActions_; + + /// the name of the last evaluated ACL (if any ACLs were evaluated) + std::optional lastCheckedName_; }; #endif /* SQUID_SRC_ACL_CHECKLIST_H */ diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc index fa886af756..fb0492cf41 100644 --- a/src/acl/Gadgets.cc +++ b/src/acl/Gadgets.cc @@ -25,6 +25,7 @@ #include "errorpage.h" #include "globals.h" #include "HttpRequest.h" +#include "SquidConfig.h" #include "src/sbuf/Stream.h" #include @@ -34,20 +35,17 @@ using AclSet = std::set; /// Accumulates all ACLs to facilitate their clean deletion despite reuse. static AclSet *RegisteredAcls; // TODO: Remove when ACLs are refcounted -/* does name lookup, returns page_id */ err_type -aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allowed) +FindDenyInfoPage(const Acl::Answer &answer, const bool redirect_allowed) { - if (!name) { - debugs(28, 3, "ERR_NONE due to a NULL name"); + if (!answer.lastCheckedName) { + debugs(28, 3, "ERR_NONE because access was denied without evaluating ACLs"); return ERR_NONE; } - AclDenyInfoList *A = nullptr; - - debugs(28, 8, "got called for " << name); + const auto &name = *answer.lastCheckedName; - for (A = *head; A; A = A->next) { + for (auto A = Config.denyInfoList; A; A = A->next) { if (!redirect_allowed && strchr(A->err_page_name, ':') ) { debugs(28, 8, "Skip '" << A->err_page_name << "' 30x redirects not allowed as response here."); continue; @@ -55,33 +53,30 @@ aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allow for (const auto &aclName: A->acl_list) { if (aclName.cmp(name) == 0) { - debugs(28, 8, "match on " << name); + debugs(28, 8, "matched " << name << "; returning " << A->err_page_id << ' ' << A->err_page_name); return A->err_page_id; } } } - debugs(28, 8, "aclGetDenyInfoPage: no match"); + debugs(28, 8, "no match for " << name << (Config.denyInfoList ? "" : "; no deny_info rules")); return ERR_NONE; } -/* does name lookup, returns if it is a proxy_auth acl */ -int -aclIsProxyAuth(const char *name) +bool +aclIsProxyAuth(const std::optional &name) { if (!name) { - debugs(28, 3, "false due to a NULL name"); + debugs(28, 3, "no; caller did not supply an ACL name"); return false; } - debugs(28, 5, "aclIsProxyAuth: called for " << name); - - if (const auto *a = Acl::Node::FindByName(name)) { - debugs(28, 5, "aclIsProxyAuth: returning " << a->isProxyAuth()); + if (const auto a = Acl::Node::FindByName(*name)) { + debugs(28, 5, "returning " << a->isProxyAuth() << " for ACL " << *name); return a->isProxyAuth(); } - debugs(28, 3, "aclIsProxyAuth: WARNING, called for nonexistent ACL"); + debugs(28, 3, "WARNING: Called for nonexistent ACL " << *name); return false; } @@ -153,13 +148,9 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) } const int ruleId = ((treep && *treep) ? (*treep)->childrenCount() : 0) + 1; - MemBuf ctxBuf; - ctxBuf.init(); - ctxBuf.appendf("%s#%d", directive, ruleId); - ctxBuf.terminate(); Acl::AndNode *rule = new Acl::AndNode; - rule->context(ctxBuf.content(), config_input_line); + rule->context(ToSBuf(directive, '#', ruleId), config_input_line); rule->lineParse(); if (rule->empty()) { debugs(28, DBG_CRITICAL, "aclParseAccessLine: " << cfg_filename << " line " << config_lineno << ": " << config_input_line); @@ -173,7 +164,7 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) assert(treep); if (!*treep) { *treep = new Acl::Tree; - (*treep)->context(directive, config_input_line); + (*treep)->context(SBuf(directive), config_input_line); } (*treep)->add(rule, action); @@ -189,24 +180,14 @@ aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label) if (!label) label = "..."; - MemBuf ctxLine; - ctxLine.init(); - ctxLine.appendf("(%s %s line)", cfg_directive, label); - ctxLine.terminate(); - Acl::AndNode *rule = new Acl::AndNode; - rule->context(ctxLine.content(), config_input_line); + rule->context(ToSBuf('(', cfg_directive, ' ', label, " line)"), config_input_line); const auto aclCount = rule->lineParse(); - MemBuf ctxTree; - ctxTree.init(); - ctxTree.appendf("%s %s", cfg_directive, label); - ctxTree.terminate(); - // We want a cbdata-protected Tree (despite giving it only one child node). Acl::Tree *tree = new Acl::Tree; tree->add(rule); - tree->context(ctxTree.content(), config_input_line); + tree->context(ToSBuf(cfg_directive, ' ', label), config_input_line); assert(treep); assert(!*treep); diff --git a/src/acl/Gadgets.h b/src/acl/Gadgets.h index 0f41899e5b..1e09be11b1 100644 --- a/src/acl/Gadgets.h +++ b/src/acl/Gadgets.h @@ -11,7 +11,9 @@ #include "acl/forward.h" #include "error/forward.h" +#include "sbuf/forward.h" +#include #include class ConfigParser; @@ -45,10 +47,14 @@ aclParseAclList(ConfigParser &parser, Acl::Tree **tree, const Any any) return aclParseAclList(parser, tree, buf.str().c_str()); } -/// \ingroup ACLAPI -int aclIsProxyAuth(const char *name); -/// \ingroup ACLAPI -err_type aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allowed); +/// Whether the given name names an Acl::Node object with true isProxyAuth() result. +/// This is a safe variation of Acl::Node::FindByName(*name)->isProxyAuth(). +bool aclIsProxyAuth(const std::optional &name); + +/// The first configured deny_info error page ID matching the given access check outcome (or ERR_NONE). +/// \param allowCustomStatus whether to consider deny_info rules containing custom HTTP response status code +err_type FindDenyInfoPage(const Acl::Answer &, bool allowCustomStatus); + /// \ingroup ACLAPI void aclParseDenyInfoLine(AclDenyInfoList **); /// \ingroup ACLAPI diff --git a/src/acl/InnerNode.cc b/src/acl/InnerNode.cc index 585c9e6247..761b56814c 100644 --- a/src/acl/InnerNode.cc +++ b/src/acl/InnerNode.cc @@ -81,8 +81,8 @@ SBufList Acl::InnerNode::dump() const { SBufList rv; - for (Nodes::const_iterator i = nodes.begin(); i != nodes.end(); ++i) - rv.push_back(SBuf((*i)->name)); + for (const auto &node: nodes) + rv.push_back(node->name); return rv; } diff --git a/src/acl/Node.h b/src/acl/Node.h index 747f76a5cf..cd264f5224 100644 --- a/src/acl/Node.h +++ b/src/acl/Node.h @@ -12,6 +12,7 @@ #include "acl/forward.h" #include "acl/Options.h" #include "dlink.h" +#include "sbuf/SBuf.h" class ConfigParser; @@ -30,6 +31,11 @@ public: static void ParseAclLine(ConfigParser &parser, Acl::Node **head); static void Initialize(); + + /// A configured ACL with a given name or nil. + static Acl::Node *FindByName(const SBuf &); + /// \copydoc FindByName() + /// \deprecated Use to avoid performance regressions; remove with the last caller. static Acl::Node *FindByName(const char *name); Node(); @@ -37,7 +43,7 @@ public: virtual ~Node(); /// sets user-specified ACL name and squid.conf context - void context(const char *name, const char *configuration); + void context(const SBuf &aName, const char *configuration); /// Orchestrates matching checklist against the Acl::Node using match(), /// after checking preconditions and while providing debugging. @@ -67,7 +73,12 @@ public: /// printed parameters are collected from all same-name "acl" directives. void dumpWhole(const char *directiveName, std::ostream &); - char name[ACL_NAME_SZ]; + /// Either aclname parameter from the explicitly configured acl directive or + /// a label generated for an internal ACL tree node. All Node objects + /// corresponding to one Squid configuration have unique names. + /// See also: context() and FindByName(). + SBuf name; + char *cfgline; Acl::Node *next; // XXX: remove or at least use refcounting bool registered; ///< added to the global list of ACLs via aclRegister() @@ -91,7 +102,7 @@ private: /// \see Acl::Node::options() virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); } - static void ParseNamed(ConfigParser &, Node **head, const char *name); + static void ParseNamed(ConfigParser &, Node **head, const SBuf &name); }; } // namespace Acl diff --git a/src/acl/forward.h b/src/acl/forward.h index 2dcbd9e7a9..d11747ff72 100644 --- a/src/acl/forward.h +++ b/src/acl/forward.h @@ -38,8 +38,6 @@ void Init(void); typedef void ACLCB(Acl::Answer, void *); -#define ACL_NAME_SZ 64 - // TODO: Consider renaming all users and removing. Cons: hides the difference // between ACLList tree without actions and acl_access Tree with actions. #define acl_access Acl::Tree diff --git a/src/adaptation/Answer.cc b/src/adaptation/Answer.cc index 8ca8c9d14a..0f5c8c567f 100644 --- a/src/adaptation/Answer.cc +++ b/src/adaptation/Answer.cc @@ -32,7 +32,7 @@ Adaptation::Answer::Forward(Http::Message *aMsg) } Adaptation::Answer -Adaptation::Answer::Block(const String &aRule) +Adaptation::Answer::Block(const SBuf &aRule) { Answer answer(akBlock); answer.ruleId = aRule; @@ -40,6 +40,15 @@ Adaptation::Answer::Block(const String &aRule) return answer; } +Acl::Answer +Adaptation::Answer::blockedToChecklistAnswer() const +{ + assert(kind == akBlock); + Acl::Answer answer(ACCESS_DENIED); + answer.lastCheckedName = ruleId; + return answer; +} + std::ostream & Adaptation::Answer::print(std::ostream &os) const { diff --git a/src/adaptation/Answer.h b/src/adaptation/Answer.h index 2fcb51459e..5b06c46bfd 100644 --- a/src/adaptation/Answer.h +++ b/src/adaptation/Answer.h @@ -9,11 +9,13 @@ #ifndef SQUID_SRC_ADAPTATION_ANSWER_H #define SQUID_SRC_ADAPTATION_ANSWER_H +#include "acl/Acl.h" #include "adaptation/forward.h" #include "http/forward.h" -#include "SquidString.h" +#include "sbuf/SBuf.h" #include +#include namespace Adaptation { @@ -31,13 +33,16 @@ public: static Answer Error(bool final); ///< create an akError answer static Answer Forward(Http::Message *aMsg); ///< create an akForward answer - static Answer Block(const String &aRule); ///< create an akBlock answer + static Answer Block(const SBuf &aRule); ///< create an akBlock answer + + /// creates an Acl::Answer from akBlock answer + Acl::Answer blockedToChecklistAnswer() const; std::ostream &print(std::ostream &os) const; public: Http::MessagePointer message; ///< HTTP request or response to forward - String ruleId; ///< ACL (or similar rule) name that blocked forwarding + std::optional ruleId; ///< ACL (or similar rule) name that blocked forwarding bool final; ///< whether the error, if any, cannot be bypassed Kind kind; ///< the type of the answer diff --git a/src/adaptation/ecap/XactionRep.cc b/src/adaptation/ecap/XactionRep.cc index c167cd6acf..b7cfe0d845 100644 --- a/src/adaptation/ecap/XactionRep.cc +++ b/src/adaptation/ecap/XactionRep.cc @@ -18,6 +18,7 @@ #include "format/Format.h" #include "HttpReply.h" #include "MasterXaction.h" +#include "sbuf/StringConvert.h" #if HAVE_LIBECAP_COMMON_AREA_H #include @@ -456,7 +457,7 @@ Adaptation::Ecap::XactionRep::blockVirgin() sinkVb("blockVirgin"); updateHistory(nullptr); - sendAnswer(Answer::Block(service().cfg().key)); + sendAnswer(Answer::Block(StringToSBuf(service().cfg().key))); Must(done()); } diff --git a/src/cache_cf.cc b/src/cache_cf.cc index f8f8757ccc..fafb9867b0 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -2029,15 +2029,12 @@ static void ParseAclWithAction(acl_access **access, const Acl::Answer &action, const char *desc, Acl::Node *acl) { assert(access); - SBuf name; if (!*access) { *access = new Acl::Tree; - name.Printf("(%s rules)", desc); - (*access)->context(name.c_str(), config_input_line); + (*access)->context(ToSBuf('(', desc, " rules)"), config_input_line); } Acl::AndNode *rule = new Acl::AndNode; - name.Printf("(%s rule)", desc); - rule->context(name.c_str(), config_input_line); + rule->context(ToSBuf('(', desc, " rule)"), config_input_line); if (acl) rule->add(acl); else @@ -4726,7 +4723,8 @@ static void parse_ftp_epsv(acl_access **ftp_epsv) *ftp_epsv = nullptr; if (ftpEpsvDeprecatedAction == Acl::Answer(ACCESS_DENIED)) { - if (auto *a = Acl::Node::FindByName("all")) + static const auto all = new SBuf("all"); + if (const auto a = Acl::Node::FindByName(*all)) ParseAclWithAction(ftp_epsv, ftpEpsvDeprecatedAction, "ftp_epsv", a); else { self_destruct(); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index ef9e05a0f5..5803bbadeb 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1855,12 +1855,11 @@ clientReplyContext::processReplyAccessResult(const Acl::Answer &accessAllowed) { debugs(88, 2, "The reply for " << http->request->method << ' ' << http->uri << " is " << accessAllowed << ", because it matched " - << (AclMatchedName ? AclMatchedName : "NO ACL's")); + << accessAllowed.lastCheckDescription()); if (!accessAllowed.allowed()) { ErrorState *err; - err_type page_id; - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); + auto page_id = FindDenyInfoPage(accessAllowed, true); http->updateLoggingTags(LOG_TCP_DENIED_REPLY); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 9ed54f2c72..2133db3790 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -713,11 +713,10 @@ void ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) { acl_checklist = nullptr; - err_type page_id; Http::StatusCode status; debugs(85, 2, "The request " << http->request->method << ' ' << http->uri << " is " << answer << - "; last ACL checked: " << (AclMatchedName ? AclMatchedName : "[none]")); + "; last ACL checked: " << answer.lastCheckDescription()); #if USE_AUTH char const *proxy_auth_msg = ""; @@ -732,21 +731,14 @@ ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) /* Send an auth challenge or error */ // XXX: do we still need aclIsProxyAuth() ? - bool auth_challenge = (answer == ACCESS_AUTH_REQUIRED || aclIsProxyAuth(AclMatchedName)); + const auto auth_challenge = (answer == ACCESS_AUTH_REQUIRED || aclIsProxyAuth(answer.lastCheckedName)); debugs(85, 5, "Access Denied: " << http->uri); - debugs(85, 5, "AclMatchedName = " << (AclMatchedName ? AclMatchedName : "")); #if USE_AUTH if (auth_challenge) debugs(33, 5, "Proxy Auth Message = " << (proxy_auth_msg ? proxy_auth_msg : "")); #endif - /* - * NOTE: get page_id here, based on AclMatchedName because if - * USE_DELAY_POOLS is enabled, then AclMatchedName gets clobbered in - * the clientCreateStoreEntry() call just below. Pedro Ribeiro - * - */ - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, answer != ACCESS_AUTH_REQUIRED); + auto page_id = FindDenyInfoPage(answer, answer != ACCESS_AUTH_REQUIRED); http->updateLoggingTags(LOG_TCP_DENIED); @@ -2050,10 +2042,8 @@ ClientHttpRequest::handleAdaptationBlock(const Adaptation::Answer &answer) { static const auto d = MakeNamedErrorDetail("REQMOD_BLOCK"); request->detailError(ERR_ACCESS_DENIED, d); - AclMatchedName = answer.ruleId.termedBuf(); assert(calloutContext); - calloutContext->clientAccessCheckDone(ACCESS_DENIED); - AclMatchedName = nullptr; + calloutContext->clientAccessCheckDone(answer.blockedToChecklistAnswer()); } void diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 60ab7d0878..5d62fa0c67 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -923,7 +923,9 @@ Client::handledEarlyAdaptationAbort() void Client::handleAdaptationBlocked(const Adaptation::Answer &answer) { - debugs(11,5, answer.ruleId); + const auto blockedAnswer = answer.blockedToChecklistAnswer(); + + debugs(11,5, blockedAnswer.lastCheckDescription()); if (abortOnBadEntry("entry went bad while ICAP aborted")) return; @@ -939,8 +941,7 @@ Client::handleAdaptationBlocked(const Adaptation::Answer &answer) debugs(11,7, "creating adaptation block response"); - err_type page_id = - aclGetDenyInfoPage(&Config.denyInfoList, answer.ruleId.termedBuf(), 1); + auto page_id = FindDenyInfoPage(blockedAnswer, true); if (page_id == ERR_NONE) page_id = ERR_ACCESS_DENIED; diff --git a/src/external_acl.cc b/src/external_acl.cc index a64155baf4..84ddc3e549 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -486,11 +486,11 @@ class external_acl_data CBDATA_CLASS(external_acl_data); public: - explicit external_acl_data(external_acl *aDef) : def(cbdataReference(aDef)), name(nullptr), arguments(nullptr) {} + explicit external_acl_data(external_acl * const aDef): def(cbdataReference(aDef)), arguments(nullptr) {} ~external_acl_data(); external_acl *def; - const char *name; + SBuf name; wordlist *arguments; }; @@ -498,7 +498,6 @@ CBDATA_CLASS_INIT(external_acl_data); external_acl_data::~external_acl_data() { - xfree(name); wordlistDestroy(&arguments); cbdataReferenceDone(def); } @@ -528,7 +527,7 @@ ACLExternal::parse() // def->name is the name of the external_acl_type. // this is the name of the 'acl' directive being tested - data->name = xstrdup(name); + data->name = name; while ((token = ConfigParser::strtokFile())) { wordlistAdd(&data->arguments, token); @@ -768,8 +767,7 @@ ACLExternal::makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl if (t->type == Format::LFT_EXT_ACL_NAME) { // setup for %ACL - safe_free(ch->al->lastAclName); - ch->al->lastAclName = xstrdup(acl_data->name); + ch->al->lastAclName = acl_data->name; } if (t->type == Format::LFT_EXT_ACL_DATA) { diff --git a/src/format/Format.cc b/src/format/Format.cc index abd919c5aa..cdce6a75b4 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -1438,7 +1438,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS break; case LFT_EXT_ACL_NAME: - out = al->lastAclName; + if (!al->lastAclName.isEmpty()) + out = al->lastAclName.c_str(); break; case LFT_EXT_ACL_DATA: diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 7c0f571aea..6171358fa1 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -427,8 +427,6 @@ icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int void icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) { - debugs(12, 2, "icpDenyAccess: Access Denied for " << from << " by " << AclMatchedName << "."); - if (clientdbCutoffDenied(from)) { /* * count this DENIED query in the clientdb, even though @@ -443,14 +441,20 @@ icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) { - /* absent any explicit rules, we deny all */ - if (!Config.accessList.icp) + if (!Config.accessList.icp) { + debugs(12, 2, "Access Denied due to lack of ICP access rules."); return false; + } ACLFilledChecklist checklist(Config.accessList.icp, icp_request, nullptr); checklist.src_addr = from; checklist.my_addr.setNoAddr(); - return checklist.fastCheck().allowed(); + const auto &answer = checklist.fastCheck(); + if (answer.allowed()) + return true; + + debugs(12, 2, "Access Denied for " << from << " by " << answer.lastCheckDescription() << "."); + return false; } HttpRequest * diff --git a/src/tests/stub_acl.cc b/src/tests/stub_acl.cc index 1583504c27..29c1027bc6 100644 --- a/src/tests/stub_acl.cc +++ b/src/tests/stub_acl.cc @@ -13,8 +13,7 @@ #define STUB_API "acl/" #include "tests/STUB.h" -#include "acl/Acl.h" -const char *AclMatchedName = nullptr; +#include "acl/forward.h" #include "acl/Gadgets.h" size_t aclParseAclList(ConfigParser &, Acl::Tree **, const char *) STUB_RETVAL(0) -- 2.39.2 From 4f3f75b7db90017510686a3db3139e7b3b70b6e6 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Tue, 9 Apr 2024 02:26:59 +0000 Subject: [PATCH 14/16] Maintenance: update --with-tdb detection (#1776) --- acinclude/tdb.m4 | 34 +++------------------ src/acl/external/helpers.m4 | 2 ++ src/acl/external/session/ext_session_acl.cc | 22 ++++++------- src/acl/external/session/required.m4 | 5 +-- src/acl/external/time_quota/required.m4 | 5 +-- 5 files changed, 20 insertions(+), 48 deletions(-) diff --git a/acinclude/tdb.m4 b/acinclude/tdb.m4 index be1eb0ec12..d666659944 100644 --- a/acinclude/tdb.m4 +++ b/acinclude/tdb.m4 @@ -6,42 +6,18 @@ ## dnl check for --with-tdb option -AC_DEFUN([SQUID_CHECK_LIBTDB],[ -AC_ARG_WITH(tdb, - AS_HELP_STRING([--without-tdb], - [Do not use Samba TrivialDB. Default: auto-detect]),[ - AS_CASE(["$with_tdb"], - [yes|no|auto],[:], - [ - AS_IF([test ! -d "$withval"], - AC_MSG_ERROR([--with-tdb path ($with_tdb) does not point to a directory]) - ) - LIBTDB_PATH="-L$withval/lib" - CPPFLAGS="-I$withval/include $CPPFLAGS" - ] - ) -]) -AH_TEMPLATE(USE_TRIVIALDB,[Samba TrivialDB support is available]) -AS_IF([test "x$with_tdb" != "xno"],[ +AC_DEFUN_ONCE([SQUID_CHECK_LIBTDB],[ +SQUID_AUTO_LIB(tdb,[Samba TrivialDB],[LIBTDB]) +SQUID_CHECK_LIB_WORKS(tdb,[ SQUID_STATE_SAVE(squid_libtdb_state) LIBS="$LIBS $LIBTDB_PATH" - PKG_CHECK_MODULES([LIBTDB],[tdb],[CPPFLAGS="$CPPFLAGS $LIBTDB_CFLAGS"],[:]) + PKG_CHECK_MODULES([LIBTDB],[tdb],[:],[:]) + CPPFLAGS="$CPPFLAGS $LIBTDB_CFLAGS" AC_CHECK_HEADERS([sys/stat.h tdb.h],,,[ #if HAVE_SYS_STAT_H #include #endif ]) SQUID_STATE_ROLLBACK(squid_libtdb_state) #de-pollute LIBS - - AS_IF([test "x$with_tdb" = "xyes" -a "x$LIBTDB_LIBS" = "x"], - AC_MSG_ERROR([Required TrivialDB library not found]) - ) - AS_IF([test "x$LIBTDB_LIBS" != "x"],[ - CXXFLAGS="$LIBTDB_CFLAGS $CXXFLAGS" - LIBTDB_LIBS="$LIBTDB_PATH $LIBTDB_LIBS" - AC_DEFINE_UNQUOTED(USE_TRIVIALDB, HAVE_TDB_H, [Samba TrivialDB support is available]) - ],[with_tdb=no]) ]) -AC_MSG_NOTICE([Samba TrivialDB library support: ${with_tdb:=auto} ${LIBTDB_PATH} ${LIBTDB_LIBS}]) -AC_SUBST(LIBTDB_LIBS) ]) diff --git a/src/acl/external/helpers.m4 b/src/acl/external/helpers.m4 index ff95c578d7..79ae726244 100644 --- a/src/acl/external/helpers.m4 +++ b/src/acl/external/helpers.m4 @@ -5,6 +5,8 @@ ## Please see the COPYING and CONTRIBUTORS files for details. ## +SQUID_CHECK_LIBTDB + EXTERNAL_ACL_HELPERS="" SQUID_HELPER_FEATURE_CHECK([external_acl_helpers],[yes],[acl/external],[ # NP: we only need this list because m4_include() does not accept variables diff --git a/src/acl/external/session/ext_session_acl.cc b/src/acl/external/session/ext_session_acl.cc index 1d4c4a28a2..da84b5ad9a 100644 --- a/src/acl/external/session/ext_session_acl.cc +++ b/src/acl/external/session/ext_session_acl.cc @@ -68,7 +68,7 @@ DB *db = nullptr; DB_ENV *db_env = nullptr; typedef DBT DB_ENTRY; -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB TDB_CONTEXT *db = nullptr; typedef TDB_DATA DB_ENTRY; @@ -86,7 +86,7 @@ shutdown_db() if (db_env) { db_env->close(db_env, 0); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB if (tdb_close(db) != 0) { fprintf(stderr, "%s| WARNING: error closing session db '%s'\n", program_name, db_path); exit(EXIT_FAILURE); @@ -113,7 +113,7 @@ static void init_db(void) exit(EXIT_FAILURE); } db_create(&db, db_env, 0); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB std::string newPath(db_path); newPath.append("session", 7); db_path = xstrdup(newPath.c_str()); @@ -136,7 +136,7 @@ static void init_db(void) db = nullptr; } } -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB #if _SQUID_FREEBSD_ && !defined(O_DSYNC) // FreeBSD lacks O_DSYNC, O_SYNC is closest to correct behaviour #define O_DSYNC O_SYNC @@ -157,7 +157,7 @@ dataSize(DB_ENTRY *data) { #if USE_BERKLEYDB return data->size; -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB return data->dsize; #endif } @@ -167,7 +167,7 @@ fetchKey(/*const*/ DB_ENTRY &key, DB_ENTRY *data) { #if USE_BERKLEYDB return (db->get(db, nullptr, &key, data, 0) == 0); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB // NP: API says returns NULL on errors, but return is a struct type WTF?? *data = tdb_fetch(db, key); return (data->dptr != nullptr); @@ -179,7 +179,7 @@ deleteEntry(/*const*/ DB_ENTRY &key) { #if USE_BERKLEYDB db->del(db, nullptr, &key, 0); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB tdb_delete(db, key); #endif } @@ -189,7 +189,7 @@ copyValue(void *dst, const DB_ENTRY *src, size_t sz) { #if USE_BERKLEYDB memcpy(dst, src->data, sz); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB memcpy(dst, src->dptr, sz); #endif } @@ -202,7 +202,7 @@ static int session_active(const char *details, size_t len) key.size = len; DBT data = {}; -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB TDB_DATA key = {}; key.dptr = reinterpret_cast(const_cast(details)); key.dsize = len; @@ -237,7 +237,7 @@ session_login(/*const*/ char *details, size_t len) data.data = &now; data.size = sizeof(now); db->put(db, nullptr, &key, &data, 0); -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB key.dptr = reinterpret_cast(details); key.dsize = len; data.dptr = reinterpret_cast(&now); @@ -253,7 +253,7 @@ session_logout(/*const*/ char *details, size_t len) #if USE_BERKLEYDB key.data = static_cast(details); key.size = len; -#elif USE_TRIVIALDB +#elif HAVE_LIBTDB key.dptr = reinterpret_cast(details); key.dsize = len; #endif diff --git a/src/acl/external/session/required.m4 b/src/acl/external/session/required.m4 index 9c29801ecf..3617c7ce9a 100755 --- a/src/acl/external/session/required.m4 +++ b/src/acl/external/session/required.m4 @@ -5,10 +5,7 @@ ## Please see the COPYING and CONTRIBUTORS files for details. ## -SQUID_CHECK_LIBTDB -if test "$with_tdb" != "no"; then - BUILD_HELPER="session" -fi +AS_IF([test "x$LIBTDB_LIBS" != "x"],[BUILD_HELPER="session"]) LIBBDB_LIBS= AH_TEMPLATE(USE_BERKLEYDB,[BerkleyDB support is available]) diff --git a/src/acl/external/time_quota/required.m4 b/src/acl/external/time_quota/required.m4 index 12dbd8a0e3..d73fdbb89a 100644 --- a/src/acl/external/time_quota/required.m4 +++ b/src/acl/external/time_quota/required.m4 @@ -5,7 +5,4 @@ ## Please see the COPYING and CONTRIBUTORS files for details. ## -SQUID_CHECK_LIBTDB -if test "$with_tdb" != "no"; then - BUILD_HELPER="time_quota" -fi +AS_IF([test "x$LIBTDB_LIBS" != "x"],[BUILD_HELPER="time_quota"]) -- 2.39.2 From 47126086215fae10ff16c5be390d876db3aedf15 Mon Sep 17 00:00:00 2001 From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Thu, 11 Apr 2024 14:22:46 +0000 Subject: [PATCH 15/16] Add AsList::quoted() (#1779) --- src/base/IoManip.h | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/src/base/IoManip.h b/src/base/IoManip.h index 3983c79664..b62865d49d 100644 --- a/src/base/IoManip.h +++ b/src/base/IoManip.h @@ -187,32 +187,57 @@ public: /// a c-string to print between consecutive items (if any). Caller must ensure lifetime. auto &delimitedBy(const char * const d) { delimiter = d; return *this; } + /// c-string to print before and after each item. Caller must ensure lifetime. + auto "ed(const char * const q = "\"") { preQuote = postQuote = q; return *this; } + + /// c-strings to print before and after each item. Caller must ensure lifetime. + auto "ed(const char * const preQ, const char * const postQ) { preQuote = preQ; postQuote = postQ; return *this; } + + /// writes the container to the given stream + void print(std::ostream &) const; + public: const Container &container; ///< zero or more items to print const char *prefix = nullptr; ///< \copydoc prefixedBy() const char *suffix = nullptr; ///< \copydoc suffixedBy() const char *delimiter = nullptr; ///< \copydoc delimitedBy() + const char *preQuote = nullptr; ///< optional c-string to print before each item; \sa quoted() + const char *postQuote = nullptr; ///< optional c-string to print after each item; \sa quoted() }; -template -inline std::ostream & -operator <<(std::ostream &os, const AsList &manipulator) +template +void +AsList::print(std::ostream &os) const { bool opened = false; - for (const auto &item: manipulator.container) { + + for (const auto &item: container) { if (!opened) { - if (manipulator.prefix) - os << manipulator.prefix; + if (prefix) + os << prefix; opened = true; } else { - if (manipulator.delimiter) - os << manipulator.delimiter; + if (delimiter) + os << delimiter; } + + if (preQuote) + os << preQuote; os << item; + if (postQuote) + os << postQuote; } - if (opened && manipulator.suffix) - os << manipulator.suffix; + + if (opened && suffix) + os << suffix; +} + +template +inline std::ostream & +operator <<(std::ostream &os, const AsList &manipulator) +{ + manipulator.print(os); return os; } -- 2.39.2 From cc8b26f9fa6a3dba5b490558652157b926e513f2 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 13 Apr 2024 08:15:00 +0000 Subject: [PATCH 16/16] Bug 5352: Do not get stuck when RESPMOD is slower than read(2) (#1777) ... RESPMOD BodyPipe buffer becomes full ... maybeMakeSpaceAvailable: will not read up to 0 The AsyncCall Client::noteDelayAwareReadChance constructed ... RESPMOD consumes some buffered virgin body data ... entering BodyProducer::noteMoreBodySpaceAvailable leaving BodyProducer::noteMoreBodySpaceAvailable ... read_timeout seconds later ... http.cc(148) httpTimeout FwdState.cc(471) fail: ERR_READ_TIMEOUT "Gateway Timeout" When RESPMOD does not empty its adaptation BodyPipe buffer fast enough, readReply() may eventually fill that buffer and call delayRead(), anticipating a noteDelayAwareReadChance() callback from Store or Server delay pools. That callback never happens if Store and Server are not getting any data -- they do not even start working until RESPMOD service starts releasing adapted/echoed response back to Squid! Meanwhile, our flags.do_next_read (cleared by readReply() caller) remains false. When/if RESPMOD service eventually frees some BodyPipe buffer space, triggering noteMoreBodySpaceAvailable() notification, nothing changes because maybeReadVirginBody() quits when flags.do_next_read is false. noteMoreBodySpaceAvailable() could not just make flags.do_next_read true because that flag may be false for a variety of other/permanent reasons. Instead, we replaced that one-size-fits-all flag with more specific checks so that reading can resume if it is safe to resume it. This change addresses a couple of flag-related XXXs. The bug was introduced in 2023 commit 50c5af88. Prior that that change, delayRead() was not called when RESPMOD BodyPipe buffer became full because maybeMakeSpaceAvailable() returned false in that case, blocking maybeReadVirginBody() from triggering readReply() via Comm::Read(). We missed flags.do_next_read dependency and that Store-specific delayRead() cannot be used to wait for adaptation buffer space to become available. XXX: To reduce risks, this change duplicates a part of calcBufferSpaceToReserve() logic. Removing that duplication requires significant (and risky) refactoring of several related methods. --- src/clients/Client.cc | 3 ++ src/clients/Client.h | 3 ++ src/clients/FtpClient.cc | 2 ++ src/http.cc | 70 ++++++++++++++++++++++++++-------------- src/http.h | 3 ++ src/http/StateFlags.h | 1 - 6 files changed, 57 insertions(+), 25 deletions(-) diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 5d62fa0c67..2719bb9cb2 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -1029,6 +1029,9 @@ Client::adjustBodyBytesRead(const int64_t delta) void Client::delayRead() { + Assure(!waitingForDelayAwareReadChance); + waitingForDelayAwareReadChance = true; + using DeferredReadDialer = NullaryMemFunT; AsyncCall::Pointer call = asyncCall(11, 5, "Client::noteDelayAwareReadChance", DeferredReadDialer(this, &Client::noteDelayAwareReadChance)); diff --git a/src/clients/Client.h b/src/clients/Client.h index f60d8cee18..6a53315637 100644 --- a/src/clients/Client.h +++ b/src/clients/Client.h @@ -194,6 +194,9 @@ protected: #endif bool receivedWholeRequestBody = false; ///< handleRequestBodyProductionEnded called + /// whether we are waiting for MemObject::delayRead() to call us back + bool waitingForDelayAwareReadChance = false; + /// whether we should not be talking to FwdState; XXX: clear fwd instead /// points to a string literal which is used only for debugging const char *doneWithFwd = nullptr; diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index fe73783246..48c8b66f8b 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -907,6 +907,8 @@ Ftp::Client::dataConnection() const void Ftp::Client::noteDelayAwareReadChance() { + // TODO: Merge with HttpStateData::noteDelayAwareReadChance() + waitingForDelayAwareReadChance = false; data.read_pending = false; maybeReadVirginBody(); } diff --git a/src/http.cc b/src/http.cc index ae12b27f0e..1e4d26ec87 100644 --- a/src/http.cc +++ b/src/http.cc @@ -1161,17 +1161,15 @@ HttpStateData::persistentConnStatus() const void HttpStateData::noteDelayAwareReadChance() { - flags.do_next_read = true; + waitingForDelayAwareReadChance = false; maybeReadVirginBody(); } void HttpStateData::readReply(const CommIoCbParams &io) { - Must(!flags.do_next_read); // XXX: should have been set false by mayReadVirginBody() - flags.do_next_read = false; - debugs(11, 5, io.conn); + waitingForCommRead = false; // Bail out early on Comm::ERR_CLOSING - close handlers will tidy up for us if (io.flag == Comm::ERR_CLOSING) { @@ -1205,7 +1203,27 @@ HttpStateData::readReply(const CommIoCbParams &io) const auto readSizeWanted = readSizeMax ? entry->bytesWanted(Range(0, readSizeMax)) : 0; if (readSizeWanted <= 0) { - delayRead(); + // XXX: If we avoid Comm::ReadNow(), we should not Comm::Read() again + // when the wait is over. We should go straight to readReply() instead. + +#if USE_ADAPTATION + // XXX: We are duplicating Client::calcBufferSpaceToReserve() logic. + // XXX: Some other delayRead() cases may lack kickReads() guarantees. + // TODO: Refactor maybeMakeSpaceAvailable() to properly treat each + // no-read case instead of calling delayRead() for the remaining cases. + + if (responseBodyBuffer) { + debugs(11, 5, "avoid delayRead() to give adaptation a chance to drain overflow buffer: " << responseBodyBuffer->contentSize()); + return; // wait for Client::noteMoreBodySpaceAvailable() + } + + if (virginBodyDestination && virginBodyDestination->buf().hasContent()) { + debugs(11, 5, "avoid delayRead() to give adaptation a chance to drain body pipe buffer: " << virginBodyDestination->buf().contentSize()); + return; // wait for Client::noteMoreBodySpaceAvailable() + } +#endif + + delayRead(); /// wait for Client::noteDelayAwareReadChance() return; } @@ -1216,7 +1234,6 @@ HttpStateData::readReply(const CommIoCbParams &io) case Comm::INPROGRESS: if (inBuf.isEmpty()) debugs(33, 2, io.conn << ": no data to process, " << xstrerr(rd.xerrno)); - flags.do_next_read = true; maybeReadVirginBody(); return; @@ -1246,7 +1263,6 @@ HttpStateData::readReply(const CommIoCbParams &io) case Comm::ENDFILE: // close detected by 0-byte read eof = 1; - flags.do_next_read = false; /* Continue to process previously read data */ break; @@ -1257,7 +1273,6 @@ HttpStateData::readReply(const CommIoCbParams &io) const auto err = new ErrorState(ERR_READ_ERROR, Http::scBadGateway, fwd->request, fwd->al); err->xerrno = rd.xerrno; fwd->fail(err); - flags.do_next_read = false; closeServer(); mustStop("HttpStateData::readReply"); return; @@ -1311,7 +1326,6 @@ HttpStateData::continueAfterParsingHeader() if (!flags.headers_parsed && !eof) { debugs(11, 9, "needs more at " << inBuf.length()); - flags.do_next_read = true; /** \retval false If we have not finished parsing the headers and may get more data. * Schedules more reads to retrieve the missing data. */ @@ -1362,7 +1376,6 @@ HttpStateData::continueAfterParsingHeader() assert(error != ERR_NONE); entry->reset(); fwd->fail(new ErrorState(error, Http::scBadGateway, fwd->request, fwd->al)); - flags.do_next_read = false; closeServer(); mustStop("HttpStateData::continueAfterParsingHeader"); return false; // quit on error @@ -1448,7 +1461,6 @@ HttpStateData::decodeAndWriteReplyBody() addVirginReplyBody(decodedData.content(), decodedData.contentSize()); if (doneParsing) { lastChunk = 1; - flags.do_next_read = false; markParsedVirginReplyAsWhole("http parsed last-chunk"); } else if (eof) { markPrematureReplyBodyEofFailure(); @@ -1472,7 +1484,6 @@ void HttpStateData::processReplyBody() { if (!flags.headers_parsed) { - flags.do_next_read = true; maybeReadVirginBody(); return; } @@ -1492,7 +1503,6 @@ HttpStateData::processReplyBody() if (entry->isAccepting()) { if (flags.chunked) { if (!decodeAndWriteReplyBody()) { - flags.do_next_read = false; serverComplete(); return; } @@ -1518,8 +1528,6 @@ HttpStateData::processReplyBody() } else { commSetConnTimeout(serverConnection, Config.Timeout.read, nil); } - - flags.do_next_read = true; } break; @@ -1529,7 +1537,6 @@ HttpStateData::processReplyBody() // TODO: Remove serverConnectionSaved but preserve exception safety. commUnsetConnTimeout(serverConnection); - flags.do_next_read = false; comm_remove_close_handler(serverConnection->fd, closeHandler); closeHandler = nullptr; @@ -1589,29 +1596,45 @@ HttpStateData::mayReadVirginReplyBody() const void HttpStateData::maybeReadVirginBody() { - // too late to read - if (!Comm::IsConnOpen(serverConnection) || fd_table[serverConnection->fd].closing()) + if (!Comm::IsConnOpen(serverConnection) || fd_table[serverConnection->fd].closing()) { + debugs(11, 3, "no, peer connection gone"); return; + } + + if (eof) { + // tolerate hypothetical calls between Comm::ENDFILE and closeServer() + debugs(11, 3, "no, saw EOF"); + return; + } + + if (lastChunk) { + // tolerate hypothetical calls between setting lastChunk and clearing serverConnection + debugs(11, 3, "no, saw last-chunk"); + return; + } if (!canBufferMoreReplyBytes()) { abortTransaction("more response bytes required, but the read buffer is full and cannot be drained"); return; } - // XXX: get rid of the do_next_read flag - // check for the proper reasons preventing read(2) - if (!flags.do_next_read) + if (waitingForDelayAwareReadChance) { + debugs(11, 5, "no, still waiting for noteDelayAwareReadChance()"); return; + } - flags.do_next_read = false; + if (waitingForCommRead) { + debugs(11, 3, "no, already waiting for readReply()"); + return; + } - // must not already be waiting for read(2) ... assert(!Comm::MonitorsRead(serverConnection->fd)); // wait for read(2) to be possible. typedef CommCbMemFunT Dialer; AsyncCall::Pointer call = JobCallback(11, 5, Dialer, this, HttpStateData::readReply); Comm::Read(serverConnection, call); + waitingForCommRead = true; } /// Desired inBuf capacity based on various capacity preferences/limits: @@ -2399,7 +2422,6 @@ HttpStateData::sendRequest() AsyncCall::Pointer timeoutCall = JobCallback(11, 5, TimeoutDialer, this, HttpStateData::httpTimeout); commSetConnTimeout(serverConnection, Config.Timeout.lifetime, timeoutCall); - flags.do_next_read = true; maybeReadVirginBody(); if (request->body_pipe != nullptr) { diff --git a/src/http.h b/src/http.h index e10603123b..71b1c15977 100644 --- a/src/http.h +++ b/src/http.h @@ -152,6 +152,9 @@ private: /// positive when we read more than we wanted int64_t payloadTruncated = 0; + /// whether we are waiting for our Comm::Read() handler to be called + bool waitingForCommRead = false; + /// Whether we received a Date header older than that of a matching /// cached response. bool sawDateGoBack = false; diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h index ae3ca9932e..6d26b3e712 100644 --- a/src/http/StateFlags.h +++ b/src/http/StateFlags.h @@ -58,7 +58,6 @@ public: bool keepalive_broken = false; bool abuse_detected = false; bool request_sent = false; - bool do_next_read = false; bool chunked = false; ///< reading a chunked response; TODO: rename bool chunked_request = false; ///< writing a chunked request bool sentLastChunk = false; ///< do not try to write last-chunk again -- 2.39.2