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 893a9c6710c2931c5cf7136f1afe76011530edb0..5657cf1eb40f48bfac007aafc90f6005666bd69e 100644 (file)
--- 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 86c168dfed32e3d5bee28747295feda2afc92757..c11d79968abd7db2d78f11dfb4fdf7fc9ab3eabf 100644 (file)
--- a/src/PeerSelectState.h
+++ b/src/PeerSelectState.h
@@ -158,7 +158,6 @@ private:
      */
     CachePeer *hit;
     peer_t hit_type;
-    ACLChecklist *acl_checklist;
 
     typedef CbcPointer<PeerSelectionInitiator> Initiator;
     Initiator initiator_; ///< recipient of the destinations we select; use interestedInitiator() to access

diff --git a/src/SnmpRequest.h b/src/SnmpRequest.h
index a9113e8b9b7ceb386dcd8c4df428426ca9724d9a..9adeb9089bfe6b5c6a904f5f8a002fba5164b9b7 100644 (file)
--- 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 a28df6b2368ee02606d6c11ba93f8acc08ada511..475ecf25881aeabbc6ed5df9c7f7a66e0f49fb3e 100644 (file)
--- 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 1807c4da80f748617b59029dda1d02d780e1a86b..eb65e30bf547476e44c265e17bc60ac7da730155 100644 (file)
--- a/src/adaptation/AccessCheck.h
+++ b/src/adaptation/AccessCheck.h
@@ -46,7 +46,6 @@ protected:
 private:
     const ServiceFilter filter;
     CbcPointer<Adaptation::Initiator> theInitiator; ///< the job which ordered this access check
-    ACLFilledChecklist *acl_checklist;
 
     typedef int Candidate;
     typedef std::vector<Candidate> Candidates;

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 09027b6cf9992de08ebd8212598f76dbd64f4f0e..1d432a8f52418e61876fef13e60b261d19a6a23d 100644 (file)
--- 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<ClientRequestContext *>(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 988fe60bbba481843ec04843435ad82d35e79dc2..ead91c0f99163a86bce8111e0e20baff080e4c97 100644 (file)
--- 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.