From: Eduard Bagdasaryan Date: Mon, 8 Jul 2024 10:28:08 +0000 (+0000) Subject: Removed effectively unused acl_checklist members (#1860) X-Git-Tag: SQUID_7_0_1~94 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f69f4cee5cbe44bf60f8acb30b777a24e631d8ff;p=thirdparty%2Fsquid.git Removed effectively unused acl_checklist members (#1860) Four classes had problematic acl_checklist data members: * SnmpRequest does not use acl_checklist at all. * ClientRequestContext and Adaptation::AccessCheck do not use acl_checklist beyond checklist creation/configuration code. A local variable works better for creation/configuration, and having a data member complicates upcoming checklist creation code improvements. * PeerSelector creates/configures a checklist and uses acl_checklist during destruction, but the latter code is marked as a Squid BUG and is itself buggy: Checklist objects are cbdata-protected. They must have one owner at any time. Starting with a nonBlockingCheck() call, that owner is the checklist object itself. That owner destructs the checklist object (i.e. "this"). If that Squid BUG code could be reached, Squid would delete the same object twice. There are no known ways to trigger that bug. --- diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index 893a9c6710..5657cf1eb4 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -62,7 +62,6 @@ public: #endif ClientHttpRequest *http; - ACLChecklist *acl_checklist = nullptr; ///< need ptr back so we can unregister if needed int redirect_state = REDIRECT_NONE; int store_id_state = REDIRECT_NONE; diff --git a/src/PeerSelectState.h b/src/PeerSelectState.h index 86c168dfed..c11d79968a 100644 --- a/src/PeerSelectState.h +++ b/src/PeerSelectState.h @@ -158,7 +158,6 @@ private: */ CachePeer *hit; peer_t hit_type; - ACLChecklist *acl_checklist; typedef CbcPointer Initiator; Initiator initiator_; ///< recipient of the destinations we select; use interestedInitiator() to access diff --git a/src/SnmpRequest.h b/src/SnmpRequest.h index a9113e8b9b..9adeb9089b 100644 --- a/src/SnmpRequest.h +++ b/src/SnmpRequest.h @@ -28,7 +28,6 @@ public: Ip::Address from; struct snmp_pdu *PDU; - ACLChecklist *acl_checklist; u_char *community; struct snmp_session session; diff --git a/src/adaptation/AccessCheck.cc b/src/adaptation/AccessCheck.cc index a28df6b236..475ecf2588 100644 --- a/src/adaptation/AccessCheck.cc +++ b/src/adaptation/AccessCheck.cc @@ -46,8 +46,7 @@ Adaptation::AccessCheck::Start(Method method, VectPoint vp, Adaptation::AccessCheck::AccessCheck(const ServiceFilter &aFilter, Adaptation::Initiator *initiator): AsyncJob("AccessCheck"), filter(aFilter), - theInitiator(initiator), - acl_checklist(nullptr) + theInitiator(initiator) { #if ICAP_CLIENT Adaptation::Icap::History::Pointer h = filter.request->icapHistory(); @@ -129,7 +128,7 @@ Adaptation::AccessCheck::checkCandidates() while (!candidates.empty()) { if (AccessRule *r = FindRule(topCandidate())) { /* BUG 2526: what to do when r->acl is empty?? */ - acl_checklist = new ACLFilledChecklist(r->acl, filter.request); + const auto acl_checklist = new ACLFilledChecklist(r->acl, filter.request); acl_checklist->updateAle(filter.al); acl_checklist->updateReply(filter.reply); acl_checklist->syncAle(filter.request, nullptr); diff --git a/src/adaptation/AccessCheck.h b/src/adaptation/AccessCheck.h index 1807c4da80..eb65e30bf5 100644 --- a/src/adaptation/AccessCheck.h +++ b/src/adaptation/AccessCheck.h @@ -46,7 +46,6 @@ protected: private: const ServiceFilter filter; CbcPointer theInitiator; ///< the job which ordered this access check - ACLFilledChecklist *acl_checklist; typedef int Candidate; typedef std::vector Candidates; diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 09027b6cf9..1d432a8f52 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -438,13 +438,13 @@ clientFollowXForwardedForCheck(Acl::Answer answer, void *data) if ((addr = asciiaddr)) { request->indirect_client_addr = addr; request->x_forwarded_for_iterator.cut(l); - calloutContext->acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); + const auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http); if (!Config.onoff.acl_uses_indirect_client) { /* override the default src_addr tested if we have to go deeper than one level into XFF */ - Filled(calloutContext->acl_checklist)->src_addr = request->indirect_client_addr; + ch->src_addr = request->indirect_client_addr; } if (++calloutContext->currentXffHopNumber < SQUID_X_FORWARDED_FOR_HOP_MAX) { - calloutContext->acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, data); + ch->nonBlockingCheck(clientFollowXForwardedForCheck, data); return; } const auto headerName = Http::HeaderLookupTable.lookup(Http::HdrType::X_FORWARDED_FOR).name; @@ -666,14 +666,14 @@ ClientRequestContext::clientAccessCheck() http->request->x_forwarded_for_iterator = http->request->header.getList(Http::HdrType::X_FORWARDED_FOR); /* begin by checking to see if we trust direct client enough to walk XFF */ - acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http); acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, this); return; } #endif if (Config.accessList.http) { - acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http); acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); } else { debugs(0, DBG_CRITICAL, "No http_access configuration found. This will block ALL traffic"); @@ -690,7 +690,7 @@ void ClientRequestContext::clientAccessCheck2() { if (Config.accessList.adapted_http) { - acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http); acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this); } else { debugs(85, 2, "No adapted_http_access configuration. default: ALLOW"); @@ -712,7 +712,6 @@ clientAccessCheckDoneWrapper(Acl::Answer answer, void *data) void ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) { - acl_checklist = nullptr; Http::StatusCode status; debugs(85, 2, "The request " << http->request->method << ' ' << http->uri << " is " << answer << @@ -821,7 +820,6 @@ clientRedirectAccessCheckDone(Acl::Answer answer, void *data) { ClientRequestContext *context = (ClientRequestContext *)data; ClientHttpRequest *http = context->http; - context->acl_checklist = nullptr; if (answer.allowed()) redirectStart(http, clientRedirectDoneWrapper, context); @@ -837,7 +835,7 @@ ClientRequestContext::clientRedirectStart() debugs(33, 5, "'" << http->uri << "'"); http->al->syncNotes(http->request); if (Config.accessList.redirector) { - acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http); acl_checklist->nonBlockingCheck(clientRedirectAccessCheckDone, this); } else redirectStart(http, clientRedirectDoneWrapper, this); @@ -852,7 +850,6 @@ clientStoreIdAccessCheckDone(Acl::Answer answer, void *data) { ClientRequestContext *context = static_cast(data); ClientHttpRequest *http = context->http; - context->acl_checklist = nullptr; if (answer.allowed()) storeIdStart(http, clientStoreIdDoneWrapper, context); @@ -874,7 +871,7 @@ ClientRequestContext::clientStoreIdStart() debugs(33, 5,"'" << http->uri << "'"); if (Config.accessList.store_id) { - acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http); acl_checklist->nonBlockingCheck(clientStoreIdAccessCheckDone, this); } else storeIdStart(http, clientStoreIdDoneWrapper, this); @@ -1313,7 +1310,7 @@ void ClientRequestContext::checkNoCache() { if (Config.accessList.noCache) { - acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); + const auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http); acl_checklist->nonBlockingCheck(checkNoCacheDoneWrapper, this); } else { /* unless otherwise specified, we try to cache. */ @@ -1335,7 +1332,6 @@ checkNoCacheDoneWrapper(Acl::Answer answer, void *data) void ClientRequestContext::checkNoCacheDone(const Acl::Answer &answer) { - acl_checklist = nullptr; if (answer.denied()) { http->request->flags.disableCacheUse("a cache deny rule matched"); } diff --git a/src/peer_select.cc b/src/peer_select.cc index 988fe60bbb..ead91c0f99 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -243,11 +243,6 @@ PeerSelector::~PeerSelector() entry->ping_status = PING_DONE; } - if (acl_checklist) { - debugs(44, DBG_IMPORTANT, "ERROR: Squid BUG: peer selector gone while waiting for a slow ACL"); - delete acl_checklist; - } - HTTPMSGUNLOCK(request); if (entry) { @@ -342,7 +337,6 @@ PeerSelectionInitiator::startSelectingDestinations(HttpRequest *request, const A void PeerSelector::checkNeverDirectDone(const Acl::Answer answer) { - acl_checklist = nullptr; debugs(44, 3, answer); never_direct = answer; switch (answer) { @@ -370,7 +364,6 @@ PeerSelector::CheckNeverDirectDone(Acl::Answer answer, void *data) void PeerSelector::checkAlwaysDirectDone(const Acl::Answer answer) { - acl_checklist = nullptr; debugs(44, 3, answer); always_direct = answer; switch (answer) { @@ -622,18 +615,16 @@ PeerSelector::selectMore() /** check always_direct; */ const auto ch = new ACLFilledChecklist(Config.accessList.AlwaysDirect, request); ch->al = al; - acl_checklist = ch; - acl_checklist->syncAle(request, nullptr); - acl_checklist->nonBlockingCheck(CheckAlwaysDirectDone, this); + ch->syncAle(request, nullptr); + ch->nonBlockingCheck(CheckAlwaysDirectDone, this); return; } else if (never_direct == ACCESS_DUNNO) { debugs(44, 3, "direct = " << DirectStr[direct] << " (never_direct to be checked)"); /** check never_direct; */ const auto ch = new ACLFilledChecklist(Config.accessList.NeverDirect, request); ch->al = al; - acl_checklist = ch; - acl_checklist->syncAle(request, nullptr); - acl_checklist->nonBlockingCheck(CheckNeverDirectDone, this); + ch->syncAle(request, nullptr); + ch->nonBlockingCheck(CheckNeverDirectDone, this); return; } else if (request->flags.noDirect) { /** if we are accelerating, direct is not an option. */ @@ -1119,7 +1110,6 @@ PeerSelector::PeerSelector(PeerSelectionInitiator *initiator): closest_parent_miss(), hit(nullptr), hit_type(PEER_NONE), - acl_checklist (nullptr), initiator_(initiator) { ; // no local defaults.