From: Alex Rousskov Date: Fri, 3 May 2024 02:00:08 +0000 (+0000) Subject: Bug 5322: Do not leak HttpReply when checking http_reply_access (#1764) X-Git-Tag: SQUID_7_0_1~127 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b1c2ea7ae7cac805f254ddb9922f3851d18547d9;p=thirdparty%2Fsquid.git Bug 5322: Do not leak HttpReply when checking http_reply_access (#1764) ... as well as response_delay_pool and send_hit directives. auto check = clientAclChecklistCreate(...); // sets check->reply check->reply = reply; // self-assignment does nothing HTTPMSGLOCK(check->reply); // an unwanted/extra lock When ACLFilledChecklist::reply is already set to X, resetting it to X should not change HttpReply lock count, but some manual locking code did not check that "already set" precondition and over-locked reply objects set to ClientHttpRequest::al::reply by clientAclChecklistFill(). Current known leaks were probably introduced in 2021 commit e227da8 that started supplying HttpReply to ACLChecklist in clientAclChecklistFill(). The added code locked the reply object correctly, but it was incompatible with unconditional manual locks in three existing indirect clientAclChecklistFill() callers (calling clientAclChecklistCreate()). This change removes all known similar leaks and improves ACLFilledChecklist API to prevent future similar leaks. --- diff --git a/src/DelayId.cc b/src/DelayId.cc index 91acdb1f99..c5837bb2d0 100644 --- a/src/DelayId.cc +++ b/src/DelayId.cc @@ -88,10 +88,7 @@ DelayId::DelayClient(ClientHttpRequest * http, HttpReply *reply) ACLFilledChecklist ch(DelayPools::delay_data[pool].access, r, nullptr); clientAclChecklistFill(ch, http); - if (!ch.reply && reply) { - ch.reply = reply; - HTTPMSGLOCK(reply); - } + ch.updateReply(reply); // overwrite ACLFilledChecklist acl_uses_indirect_client-based decision #if FOLLOW_X_FORWARDED_FOR if (Config.onoff.delay_pool_uses_indirect_client) diff --git a/src/HttpHeaderTools.cc b/src/HttpHeaderTools.cc index c308a79a6f..64f58e779e 100644 --- a/src/HttpHeaderTools.cc +++ b/src/HttpHeaderTools.cc @@ -298,12 +298,7 @@ httpHdrMangle(HttpHeaderEntry * e, HttpRequest * request, HeaderManglers *hms, c } ACLFilledChecklist checklist(hm->access_list, request, nullptr); - - checklist.al = al; - if (al && al->reply) { - checklist.reply = al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } + checklist.updateAle(al); // XXX: The two "It was denied" clauses below mishandle cases with no // matching rules, violating the "If no rules within the set have matching @@ -498,12 +493,7 @@ void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer &al, HeaderWithAclList &headersAdd) { ACLFilledChecklist checklist(nullptr, request, nullptr); - - checklist.al = al; - if (al && al->reply) { - checklist.reply = al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } + checklist.updateAle(al); for (HeaderWithAclList::const_iterator hwa = headersAdd.begin(); hwa != headersAdd.end(); ++hwa) { if (!hwa->aclList || checklist.fastCheck(hwa->aclList).allowed()) { diff --git a/src/HttpReply.cc b/src/HttpReply.cc index 47597be4bb..d03617d3a3 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -595,9 +595,7 @@ HttpReply::calcMaxBodySize(HttpRequest& request) const return; ACLFilledChecklist ch(nullptr, &request, nullptr); - // XXX: cont-cast becomes irrelevant when checklist is HttpReply::Pointer - ch.reply = const_cast(this); - HTTPMSGLOCK(ch.reply); + ch.updateReply(this); for (AclSizeLimit *l = Config.ReplyBodySize; l; l = l -> next) { /* if there is no ACL list or if the ACLs listed match use this size value */ if (!l->aclList || ch.fastCheck(l->aclList).allowed()) { diff --git a/src/Notes.cc b/src/Notes.cc index 207824fc42..a6149408bd 100644 --- a/src/Notes.cc +++ b/src/Notes.cc @@ -70,11 +70,9 @@ bool Note::match(HttpRequest *request, HttpReply *reply, const AccessLogEntry::Pointer &al, SBuf &matched) { ACLFilledChecklist ch(nullptr, request, nullptr); - ch.al = al; - ch.reply = reply; + ch.updateAle(al); + ch.updateReply(reply); ch.syncAle(request, nullptr); - if (reply) - HTTPMSGLOCK(ch.reply); for (const auto &v: values) { assert(v->aclList); diff --git a/src/acl/ConnectionsEncrypted.cc b/src/acl/ConnectionsEncrypted.cc index 3ae45df2b9..12b7eae9f2 100644 --- a/src/acl/ConnectionsEncrypted.cc +++ b/src/acl/ConnectionsEncrypted.cc @@ -55,8 +55,8 @@ Acl::ConnectionsEncrypted::match(ACLChecklist *checklist) const bool safeRequest = !(filled->request->sources & Http::Message::srcUnsafe); - const bool safeReply = !filled->reply || - !(filled->reply->sources & Http::Message::srcUnsafe); + const bool safeReply = !filled->hasReply() || + !(filled->reply().sources & Http::Message::srcUnsafe); return (safeRequest && safeReply) ? 1 : 0; } diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc index 2a1fc7703a..45dbc75e41 100644 --- a/src/acl/FilledChecklist.cc +++ b/src/acl/FilledChecklist.cc @@ -26,7 +26,6 @@ CBDATA_CLASS_INIT(ACLFilledChecklist); ACLFilledChecklist::ACLFilledChecklist() : dst_rdns(nullptr), - reply (nullptr), #if USE_AUTH auth_user_request (nullptr), #endif @@ -51,8 +50,6 @@ ACLFilledChecklist::~ACLFilledChecklist() safe_free(dst_rdns); // created by xstrdup(). - HTTPMSGUNLOCK(reply); - cbdataReferenceDone(conn_); debugs(28, 4, "ACLFilledChecklist destroyed " << this); @@ -104,9 +101,9 @@ ACLFilledChecklist::verifyAle() const } } - if (reply && !al->reply) { + if (hasReply() && !al->reply) { showDebugWarning("HttpReply object"); - al->reply = reply; + al->reply = reply_; } #if USE_IDENT @@ -210,7 +207,6 @@ ACLFilledChecklist::markSourceDomainChecked() */ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_request, const char *ident): dst_rdns(nullptr), - reply(nullptr), #if USE_AUTH auth_user_request(nullptr), #endif @@ -263,3 +259,22 @@ ACLFilledChecklist::setIdent(const char *ident) #endif } +void +ACLFilledChecklist::updateAle(const AccessLogEntry::Pointer &a) +{ + if (!a) + return; + + al = a; // could have been set already (to a different value) + if (!request) + setRequest(a->request); + updateReply(a->reply); +} + +void +ACLFilledChecklist::updateReply(const HttpReply::Pointer &r) +{ + if (r) + reply_ = r; // may already be set, including to r +} + diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h index 51f5052350..927a468f5f 100644 --- a/src/acl/FilledChecklist.h +++ b/src/acl/FilledChecklist.h @@ -15,6 +15,7 @@ #include "acl/forward.h" #include "base/CbcPointer.h" #include "error/forward.h" +#include "HttpReply.h" #include "HttpRequest.h" #include "ip/Address.h" #if USE_AUTH @@ -43,6 +44,11 @@ public: /// configure rfc931 user identity for the first time void setIdent(const char *userIdentity); + /// Remembers the given ALE (if it is not nil) or does nothing (otherwise). + /// When (and only when) remembering ALE, populates other still-unset fields + /// with ALE-derived information, so that the caller does not have to. + void updateAle(const AccessLogEntry::Pointer &); + public: /// The client connection manager ConnStateData * conn() const; @@ -55,6 +61,14 @@ public: /// set the client side FD void fd(int aDescriptor); + /// response added by updateReply() + /// \prec hasReply() + const HttpReply &reply() const { return *reply_; } + + /// Remembers the given response (if it is not nil) or does nothing + /// (otherwise). + void updateReply(const HttpReply::Pointer &); + bool destinationDomainChecked() const; void markDestinationDomainChecked(); bool sourceDomainChecked() const; @@ -62,7 +76,7 @@ public: // ACLChecklist API bool hasRequest() const override { return request != nullptr; } - bool hasReply() const override { return reply != nullptr; } + bool hasReply() const override { return reply_ != nullptr; } bool hasAle() const override { return al != nullptr; } void syncAle(HttpRequest *adaptedRequest, const char *logUri) const override; void verifyAle() const override; @@ -75,7 +89,6 @@ public: char *dst_rdns; HttpRequest::Pointer request; - HttpReply *reply; char rfc931[USER_IDENT_SZ]; #if USE_AUTH @@ -106,6 +119,9 @@ public: private: ConnStateData * conn_; /**< hack for ident and NTLM */ int fd_; /**< may be available when conn_ is not */ + + HttpReply::Pointer reply_; ///< response added by updateReply() or nil + bool destinationDomainChecked_; bool sourceDomainChecked_; /// not implemented; will cause link failures if used diff --git a/src/acl/HttpRepHeader.cc b/src/acl/HttpRepHeader.cc index 54c29c7942..9dcb7f7e10 100644 --- a/src/acl/HttpRepHeader.cc +++ b/src/acl/HttpRepHeader.cc @@ -14,6 +14,6 @@ int Acl::HttpRepHeaderCheck::match(ACLChecklist * const ch) { - return data->match(Filled(ch)->reply->header); + return data->match(Filled(ch)->reply().header); } diff --git a/src/acl/HttpStatus.cc b/src/acl/HttpStatus.cc index 14e21341df..bb9052f38d 100644 --- a/src/acl/HttpStatus.cc +++ b/src/acl/HttpStatus.cc @@ -131,7 +131,7 @@ aclParseHTTPStatusList(Splay **curlist) int ACLHTTPStatus::match(ACLChecklist *checklist) { - return aclMatchHTTPStatus(&data, Filled(checklist)->reply->sline.status()); + return aclMatchHTTPStatus(&data, Filled(checklist)->reply().sline.status()); } int diff --git a/src/acl/ReplyHeaderStrategy.h b/src/acl/ReplyHeaderStrategy.h index d670fca431..63b60b60af 100644 --- a/src/acl/ReplyHeaderStrategy.h +++ b/src/acl/ReplyHeaderStrategy.h @@ -36,7 +36,7 @@ Acl::ReplyHeaderCheck
::match(ACLChecklist * const ch) { const auto checklist = Filled(ch); - char const *theHeader = checklist->reply->header.getStr(header); + const auto theHeader = checklist->reply().header.getStr(header); if (nullptr == theHeader) return 0; diff --git a/src/acl/ReplyMimeType.h b/src/acl/ReplyMimeType.h index 1c379764c1..6723edbcd7 100644 --- a/src/acl/ReplyMimeType.h +++ b/src/acl/ReplyMimeType.h @@ -21,7 +21,7 @@ Acl::ReplyHeaderCheck::match(ACLChecklist * const c { const auto checklist = Filled(ch); - char const *theHeader = checklist->reply->header.getStr(Http::HdrType::CONTENT_TYPE); + auto theHeader = checklist->reply().header.getStr(Http::HdrType::CONTENT_TYPE); if (nullptr == theHeader) theHeader = ""; diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index 279bdc34bd..a65fd81716 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -131,9 +131,8 @@ Adaptation::AccessCheck::checkCandidates() /* BUG 2526: what to do when r->acl is empty?? */ // XXX: we do not have access to conn->rfc931 here. acl_checklist = new ACLFilledChecklist(r->acl, filter.request, dash_str); - if ((acl_checklist->reply = filter.reply)) - HTTPMSGLOCK(acl_checklist->reply); - acl_checklist->al = filter.al; + acl_checklist->updateAle(filter.al); + acl_checklist->updateReply(filter.reply); acl_checklist->syncAle(filter.request, nullptr); acl_checklist->nonBlockingCheck(AccessCheckCallbackWrapper, this); return; diff --git a/src/adaptation/icap/Launcher.cc b/src/adaptation/icap/Launcher.cc index eed697842c..71b881894d 100644 --- a/src/adaptation/icap/Launcher.cc +++ b/src/adaptation/icap/Launcher.cc @@ -142,8 +142,7 @@ bool Adaptation::Icap::Launcher::canRepeat(Adaptation::Icap::XactAbortInfo &info ACLFilledChecklist *cl = new ACLFilledChecklist(TheConfig.repeat, info.icapRequest, dash_str); - cl->reply = info.icapReply; - HTTPMSGLOCK(cl->reply); + cl->updateReply(info.icapReply); bool result = cl->fastCheck().allowed(); delete cl; diff --git a/src/adaptation/icap/icap_log.cc b/src/adaptation/icap/icap_log.cc index 44522f3752..1cb1039ea2 100644 --- a/src/adaptation/icap/icap_log.cc +++ b/src/adaptation/icap/icap_log.cc @@ -61,10 +61,7 @@ void icapLogLog(AccessLogEntry::Pointer &al) { if (IcapLogfileStatus == LOG_ENABLE) { ACLFilledChecklist checklist(nullptr, al->adapted_request, nullptr); - if (al->reply) { - checklist.reply = al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } + checklist.updateAle(al); accessLogLogTo(Config.Log.icaplogs, al, &checklist); } } diff --git a/src/auth/UserRequest.cc b/src/auth/UserRequest.cc index 7dfae3ce89..9045dc87bf 100644 --- a/src/auth/UserRequest.cc +++ b/src/auth/UserRequest.cc @@ -479,8 +479,7 @@ schemesConfig(HttpRequest *request, HttpReply *rep) { if (!Auth::TheConfig.schemeLists.empty() && Auth::TheConfig.schemeAccess) { ACLFilledChecklist ch(nullptr, request, nullptr); - ch.reply = rep; - HTTPMSGLOCK(ch.reply); + ch.updateReply(rep); const auto answer = ch.fastCheck(Auth::TheConfig.schemeAccess); if (answer.allowed()) return Auth::TheConfig.schemeLists.at(answer.kind).authConfigs; diff --git a/src/client_side.cc b/src/client_side.cc index eac7d93528..1ec0a8f4a5 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -435,7 +435,6 @@ ClientHttpRequest::logRequest() #endif - /* Add notes (if we have a request to annotate) */ if (request) { SBuf matched; for (auto h: Config.notes) { @@ -446,31 +445,21 @@ ClientHttpRequest::logRequest() } // The al->notes and request->notes must point to the same object. al->syncNotes(request); - } - ACLFilledChecklist checklist(nullptr, request, nullptr); - if (al->reply) { - checklist.reply = al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } - - if (request) { HTTPMSGUNLOCK(al->adapted_request); al->adapted_request = request; HTTPMSGLOCK(al->adapted_request); } + + ACLFilledChecklist checklist(nullptr, request, nullptr); + checklist.updateAle(al); // no need checklist.syncAle(): already synced - checklist.al = al; accessLogLog(al, &checklist); bool updatePerformanceCounters = true; if (Config.accessList.stats_collection) { ACLFilledChecklist statsCheck(Config.accessList.stats_collection, request, nullptr); - statsCheck.al = al; - if (al->reply) { - statsCheck.reply = al->reply.getRaw(); - HTTPMSGLOCK(statsCheck.reply); - } + statsCheck.updateAle(al); updatePerformanceCounters = statsCheck.fastCheck().allowed(); } @@ -3499,12 +3488,8 @@ clientAclChecklistFill(ACLFilledChecklist &checklist, ClientHttpRequest *http) checklist.setRequest(http->request); if (!checklist.al && http->al) { - checklist.al = http->al; + checklist.updateAle(http->al); checklist.syncAle(http->request, http->log_uri); - if (!checklist.reply && http->al->reply) { - checklist.reply = http->al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } } if (const auto conn = http->getConn()) diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 5803bbadeb..0e13953425 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -843,11 +843,9 @@ clientReplyContext::blockedHit() const if (http->request->flags.internal) return false; // internal content "hits" cannot be blocked - const auto &rep = http->storeEntry()->mem().freshestReply(); { std::unique_ptr chl(clientAclChecklistCreate(Config.accessList.sendHit, http)); - chl->reply = const_cast(&rep); // ACLChecklist API bug - HTTPMSGLOCK(chl->reply); + chl->updateReply(&http->storeEntry()->mem().freshestReply()); return !chl->fastCheck().allowed(); // when in doubt, block } } @@ -1838,8 +1836,7 @@ clientReplyContext::processReplyAccess () /** Process http_reply_access lists */ ACLFilledChecklist *replyChecklist = clientAclChecklistCreate(Config.accessList.reply, http); - replyChecklist->reply = reply; - HTTPMSGLOCK(replyChecklist->reply); + replyChecklist->updateReply(reply); replyChecklist->nonBlockingCheck(ProcessReplyAccessResult, this); } diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 2719bb9cb2..b5d8a8d2b7 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -555,9 +555,8 @@ Client::blockCaching() // This relatively expensive check is not in StoreEntry::checkCachable: // That method lacks HttpRequest and may be called too many times. ACLFilledChecklist ch(acl, originalRequest().getRaw()); - ch.reply = const_cast(&entry->mem().freshestReply()); // ACLFilledChecklist API bug - HTTPMSGLOCK(ch.reply); - ch.al = fwd->al; + ch.updateAle(fwd->al); + ch.updateReply(&entry->mem().freshestReply()); if (!ch.fastCheck().allowed()) { // when in doubt, block debugs(20, 3, "store_miss prohibits caching"); return true; diff --git a/src/http.cc b/src/http.cc index 1e4d26ec87..7afef1db31 100644 --- a/src/http.cc +++ b/src/http.cc @@ -771,10 +771,9 @@ HttpStateData::handle1xx(const HttpReply::Pointer &reply) // check whether the 1xx response forwarding is allowed by squid.conf if (Config.accessList.reply) { ACLFilledChecklist ch(Config.accessList.reply, originalRequest().getRaw()); - ch.al = fwd->al; - ch.reply = reply.getRaw(); + ch.updateAle(fwd->al); + ch.updateReply(reply); ch.syncAle(originalRequest().getRaw(), nullptr); - HTTPMSGLOCK(ch.reply); if (!ch.fastCheck().allowed()) // TODO: support slow lookups? return drop1xx("http_reply_access blocked it"); } diff --git a/src/http/Stream.cc b/src/http/Stream.cc index 1912d4f113..d5d222049c 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -291,8 +291,7 @@ Http::Stream::sendStartOfMessage(HttpReply *rep, StoreIOBuffer bodyData) for (const auto &pool: MessageDelayPools::Instance()->pools) { if (pool->access) { std::unique_ptr chl(clientAclChecklistCreate(pool->access, http)); - chl->reply = rep; - HTTPMSGLOCK(chl->reply); + chl->updateReply(rep); const auto answer = chl->fastCheck(); if (answer.allowed()) { writeQuotaHandler = pool->createBucket(); diff --git a/src/neighbors.cc b/src/neighbors.cc index 34a4383df9..2bf088b062 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -168,11 +168,7 @@ peerAllowedToUse(const CachePeer * p, PeerSelector * ps) return true; ACLFilledChecklist checklist(p->access, request, nullptr); - checklist.al = ps->al; - if (ps->al && ps->al->reply) { - checklist.reply = ps->al->reply.getRaw(); - HTTPMSGLOCK(checklist.reply); - } + checklist.updateAle(ps->al); checklist.syncAle(request, nullptr); return checklist.fastCheck().allowed(); }