From 940e8b0c764443d7e156106af8086554b9db787d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Wed, 18 Feb 2026 21:13:26 +0000 Subject: [PATCH] ICP: Fix HttpRequest lifetime for ICP v3 queries (#2377) ACLFilledChecklist correctly locks and unlocks HttpRequest. Thus, when given an unlocked request object, an on-stack checklist destroys it. Upon icpAccessAllowed() return, Squid uses the destroyed request object. This bug was probably introduced in 2003 commit 8000a965 that started automatically unlocking requests in ACLChecklist destructor. However, the bug did not affect allowed ICP v3 queries until 2007 commit f72fb56b started _using_ the request object for them. 2005 commit 319bf5a7 fixed an equivalent ICP v2 bug for denied queries but missed the ICP v3 case. The scope, age, and effect of this bug imply that Squid v3+ deployments receive no ICP v3 queries since 2007 (or earlier). Squid itself does not send ICP v3 messages, responding with ICP v2 replies to ICP v3 queries. TODO: Consider dropping ICP v3 support. Also moved icpAccessAllowed() inside icpGetRequest() to deduplicate code and reduce the risk of allowing a request without consulting icp_access. --- src/ICP.h | 5 +---- src/icp_v2.cc | 33 +++++++++++++++------------------ src/icp_v3.cc | 10 ++-------- src/tests/stub_icp.cc | 4 ++-- 4 files changed, 20 insertions(+), 32 deletions(-) diff --git a/src/ICP.h b/src/ICP.h index e9ad8d0cef1..a042d2d745f 100644 --- a/src/ICP.h +++ b/src/ICP.h @@ -94,10 +94,7 @@ extern Ip::Address theIcpPublicHostID; const char *icpGetUrl(const Ip::Address &from, const char *, const icp_common_t &); /// \ingroup ServerProtocolICPAPI -HttpRequest *icpGetRequest(const char *url, int reqnum, int fd, const Ip::Address &from); - -/// \ingroup ServerProtocolICPAPI -bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request); +HttpRequestPointer icpGetRequest(const char *url, int reqnum, int fd, const Ip::Address &from); /// \ingroup ServerProtocolICPAPI void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from, AccessLogEntryPointer); diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 312104024de..33baf9787cf 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -438,8 +438,9 @@ icpDenyAccess(const Ip::Address &from, const char * const url, const int reqnum, } } -bool -icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) +/// icpGetRequest() helper that determines whether squid.conf allows the given ICP query +static bool +icpAccessAllowed(const Ip::Address &from, HttpRequest * icp_request) { if (!Config.accessList.icp) { debugs(12, 2, "Access Denied due to lack of ICP access rules."); @@ -490,7 +491,7 @@ icpGetUrl(const Ip::Address &from, const char * const buf, const icp_common_t &h return url; } -HttpRequest * +HttpRequest::Pointer icpGetRequest(const char * const url, const int reqnum, const int fd, const Ip::Address &from) { if (strpbrk(url, w_space)) { @@ -499,12 +500,17 @@ icpGetRequest(const char * const url, const int reqnum, const int fd, const Ip:: } const auto mx = MasterXaction::MakePortless(); - auto *result = HttpRequest::FromUrlXXX(url, mx); - if (!result) - icpCreateAndSend(ICP_ERR, 0, url, reqnum, 0, fd, from, nullptr); + if (const HttpRequest::Pointer request = HttpRequest::FromUrlXXX(url, mx)) { + if (!icpAccessAllowed(from, request.getRaw())) { + icpDenyAccess(from, url, reqnum, fd); + return nullptr; + } - return result; + return request; + } + icpCreateAndSend(ICP_ERR, 0, url, reqnum, 0, fd, from, nullptr); + return nullptr; } static void @@ -520,18 +526,11 @@ doV2Query(const int fd, Ip::Address &from, const char * const buf, icp_common_t return; } - HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from); + const auto icp_request = icpGetRequest(url, header.reqnum, fd, from); if (!icp_request) return; - HTTPMSGLOCK(icp_request); - - if (!icpAccessAllowed(from, icp_request)) { - icpDenyAccess(from, url, header.reqnum, fd); - HTTPMSGUNLOCK(icp_request); - return; - } #if USE_ICMP if (header.flags & ICP_FLAG_SRC_RTT) { rtt = netdbHostRtt(icp_request->url.host()); @@ -544,7 +543,7 @@ doV2Query(const int fd, Ip::Address &from, const char * const buf, icp_common_t #endif /* USE_ICMP */ /* The peer is allowed to use this cache */ - ICP2State state(header, icp_request); + ICP2State state(header, icp_request.getRaw()); state.fd = fd; state.from = from; state.url = xstrdup(url); @@ -573,8 +572,6 @@ doV2Query(const int fd, Ip::Address &from, const char * const buf, icp_common_t } icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, state.al); - - HTTPMSGUNLOCK(icp_request); } void diff --git a/src/icp_v3.cc b/src/icp_v3.cc index 844768aa0bc..3864b0b7476 100644 --- a/src/icp_v3.cc +++ b/src/icp_v3.cc @@ -40,19 +40,13 @@ doV3Query(int fd, Ip::Address &from, const char * const buf, icp_common_t header return; } - HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from); + const auto icp_request = icpGetRequest(url, header.reqnum, fd, from); if (!icp_request) return; - if (!icpAccessAllowed(from, icp_request)) { - icpDenyAccess (from, url, header.reqnum, fd); - delete icp_request; - return; - } - /* The peer is allowed to use this cache */ - ICP3State state(header, icp_request); + ICP3State state(header, icp_request.getRaw()); state.fd = fd; state.from = from; state.url = xstrdup(url); diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc index d148ab66d48..4c9037495ef 100644 --- a/src/tests/stub_icp.cc +++ b/src/tests/stub_icp.cc @@ -9,6 +9,7 @@ #include "squid.h" #include "AccessLogEntry.h" #include "comm/Connection.h" +#include "HttpRequest.h" #include "ICP.h" #define STUB_API "icp_*.cc" @@ -30,8 +31,7 @@ Comm::ConnectionPointer icpOutgoingConn; Ip::Address theIcpPublicHostID; const char *icpGetUrl(const Ip::Address &, const char *, const icp_common_t &) STUB_RETVAL(nullptr) -HttpRequest* icpGetRequest(const char *, int, int, const Ip::Address &) STUB_RETVAL(nullptr) -bool icpAccessAllowed(Ip::Address &, HttpRequest *) STUB_RETVAL(false) +HttpRequest::Pointer icpGetRequest(const char *, int, int, const Ip::Address &) STUB_RETVAL(nullptr) void icpCreateAndSend(icp_opcode, int, char const *, int, int, int, const Ip::Address &, AccessLogEntryPointer) STUB icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID) void icpDenyAccess(const Ip::Address &, const char *, int, int) STUB