]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Reduce ACLChecklist::AsyncState to a function pointer (#1576)
authorEduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
Wed, 15 Nov 2023 09:16:02 +0000 (09:16 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Fri, 17 Nov 2023 22:51:38 +0000 (22:51 +0000)
ExternalACLLookup::checkForAsync() -- the code that starts communication
with the external ACL helper -- relies on two implicit assumptions:

* AclMatchedName global matches this->name during ACLExternal::match().
* FindByName("x") result never changes during same-Checklist evaluation.

The first assumption might hold[^1], but the second assumption will fail
once we finally start refcounting ACLs instead of allowing
reconfiguration to break in-flight transactions waiting for a slow ACL
match with ACCESS_DUNNO answers. Today, reconfiguration invalidates all
ACL objects, making repeated same-Checklist checkForAsync() calls
impossible across reconfiguration. Tomorrow, such calls will happen and
violation of the second assumption will lead to assertions or worse[^2].
Thus, removing that assumption is a precondition on ACL refcounting and
smooth reconfiguration support.

ExternalACLLookup used FindByName() to find the ACL being matched. The
source code comment suggested that we "have a pointer to this around
somewhere". That pointer is "this" pointer of an ACL calling goAsync().
It is available in the call stack two frames higher, but is not stored
by the Checklist in any usable form.

To remove that FindByName() call, we could just add a second goAsync()
parameter to pass the ACL pointer to Checklist. Doing that would require
changing all the goAsync() callers and the corresponding
ACLChecklist::AsyncState API. Since that very complex and confusing API
was actually used as a basic function pointer, we decided it is best to
replace the has-to-be-changed-anyway API with a function pointer. That
plan worked well, removing a lot of unnecessary and confusing code!

[^1]: It is difficult to be sure: AclMatchedName global was named and
added for a very different purpose, and no API enforces that invariant.

[^2]: When ACLs are refcountered, ACLExternal::match() will continue to
set AclMatchedName global to the name (e.g., "x") of an ACL object that
resumes matching after Squid reconfigures and receives a helper reply.
However, the same FindByName("x") call may now return nil or a
completely different ACL object (still named "x" in the new
configuration, but possibly no longer an ACLExternal object)!

20 files changed:
src/ExternalACL.h
src/acl/Asn.cc
src/acl/Checklist.cc
src/acl/Checklist.h
src/acl/DestinationDomain.cc
src/acl/DestinationDomain.h
src/acl/DestinationIp.cc
src/acl/DestinationIp.h
src/acl/SourceDomain.cc
src/acl/SourceDomain.h
src/auth/Acl.cc
src/auth/Acl.h
src/auth/AclMaxUserIp.cc
src/auth/AclProxyAuth.cc
src/auth/AclProxyAuth.h
src/external_acl.cc
src/ident/AclIdent.cc
src/ident/AclIdent.h
src/tests/stub_external_acl.cc
src/tests/stub_libauth_acls.cc

index d3b7689d66dcd986f647db67e95a044b67e9669b..0ce746156210a9f51dda1729eeb85386a9392a0b 100644 (file)
@@ -9,6 +9,7 @@
 #ifndef SQUID_EXTERNALACL_H
 #define SQUID_EXTERNALACL_H
 
+#include "acl/Acl.h"
 #include "acl/Checklist.h"
 #include "base/RefCount.h"
 
@@ -16,31 +17,11 @@ class external_acl;
 class external_acl_data;
 class StoreEntry;
 
-class ExternalACLLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static ExternalACLLookup *Instance();
-    void checkForAsync(ACLChecklist *)const override;
-
-    // If possible, starts an asynchronous lookup of an external ACL.
-    // Otherwise, asserts (or bails if background refresh is requested).
-    static void Start(ACLChecklist *checklist, external_acl_data *acl, bool bg);
-
-private:
-    static ExternalACLLookup instance_;
-    static void LookupDone(void *data, const ExternalACLEntryPointer &result);
-};
-
-#include "acl/Acl.h"
-
 class ACLExternal : public ACL
 {
     MEMPROXY_CLASS(ACLExternal);
 
 public:
-    static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *);
-
     ACLExternal(char const *);
     ~ACLExternal() override;
 
@@ -58,7 +39,13 @@ public:
     bool valid () const override;
     bool empty () const override;
 
-protected:
+private:
+    static void StartLookup(ACLFilledChecklist &, const ACL &);
+    static void LookupDone(void *data, const ExternalACLEntryPointer &);
+    void startLookup(ACLFilledChecklist *, external_acl_data *, bool inBackground) const;
+    Acl::Answer aclMatchExternal(external_acl_data *, ACLFilledChecklist *) const;
+    char *makeExternalAclKey(ACLFilledChecklist *, external_acl_data *) const;
+
     external_acl_data *data;
     char const *class_;
 };
index f1e318b1c732c44e8ce455c99900fcb3417c1532..663e74d38ced4186811a67f26429a0413e4d9bcb 100644 (file)
@@ -542,7 +542,7 @@ Acl::DestinationAsnCheck::match(ACLChecklist * const ch)
     } else if (!checklist->request->flags.destinationIpLookedUp) {
         /* No entry in cache, lookup not attempted */
         debugs(28, 3, "can't yet compare '" << AclMatchedName << "' ACL for " << checklist->request->url.host());
-        if (checklist->goAsync(DestinationIPLookup::Instance()))
+        if (checklist->goAsync(ACLDestinationIP::StartLookup, *this))
             return -1;
         // else fall through to noaddr match, hiding the lookup failure (XXX)
     }
index d159dc2d60a425e59e0009838d748e7e562d9760..813e863c01d54ed90ec09d273195985405ffc3ac 100644 (file)
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "acl/Checklist.h"
+#include "acl/FilledChecklist.h"
 #include "acl/Tree.h"
 #include "debug/Stream.h"
 
@@ -111,9 +112,8 @@ ACLChecklist::matchChild(const Acl::InnerNode *current, Acl::Nodes::const_iterat
 }
 
 bool
-ACLChecklist::goAsync(AsyncState *state)
+ACLChecklist::goAsync(AsyncStarter starter, const ACL &acl)
 {
-    assert(state);
     assert(!asyncInProgress());
     assert(matchLoc_.parent);
 
@@ -137,10 +137,9 @@ ACLChecklist::goAsync(AsyncState *state)
     ++asyncLoopDepth_;
 
     asyncStage_ = asyncStarting;
-    changeState(state);
-    state->checkForAsync(this); // this is supposed to go async
+    starter(*Filled(this), acl); // this is supposed to go async
 
-    // Did AsyncState object actually go async? If not, tell the caller.
+    // Did starter() actually go async? If not, tell the caller.
     if (asyncStage_ != asyncStarting) {
         assert(asyncStage_ == asyncFailed);
         asyncStage_ = asyncNone; // sanity restored
@@ -182,7 +181,6 @@ ACLChecklist::ACLChecklist() :
     finished_(false),
     answer_(ACCESS_DENIED),
     asyncStage_(asyncNone),
-    state_(NullState::Instance()),
     asyncLoopDepth_(0)
 {
 }
@@ -196,38 +194,6 @@ ACLChecklist::~ACLChecklist()
     debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this);
 }
 
-ACLChecklist::NullState *
-ACLChecklist::NullState::Instance()
-{
-    return &_instance;
-}
-
-void
-ACLChecklist::NullState::checkForAsync(ACLChecklist *) const
-{
-    assert(false); // or the Checklist will never get out of the async state
-}
-
-ACLChecklist::NullState ACLChecklist::NullState::_instance;
-
-void
-ACLChecklist::changeState (AsyncState *newState)
-{
-    /* only change from null to active and back again,
-     * not active to active.
-     * relax this once conversion to states is complete
-     * RBC 02 2003
-     */
-    assert (state_ == NullState::Instance() || newState == NullState::Instance());
-    state_ = newState;
-}
-
-ACLChecklist::AsyncState *
-ACLChecklist::asyncState() const
-{
-    return state_;
-}
-
 /**
  * Kick off a non-blocking (slow) ACL access list test
  *
@@ -258,11 +224,8 @@ ACLChecklist::nonBlockingCheck(ACLCB * callback_, void *callback_data_)
 }
 
 void
-ACLChecklist::resumeNonBlockingCheck(AsyncState *state)
+ACLChecklist::resumeNonBlockingCheck()
 {
-    assert(asyncState() == state);
-    changeState(NullState::Instance());
-
     if (asyncStage_ == asyncStarting) { // oops, we did not really go async
         asyncStage_ = asyncFailed; // goAsync() checks for that
         // Do not fall through to resume checks from the async callback. Let
index 57800c819e594574c617fcd64ce702f68637841c..ae0096cc0be02601d3741a0f23c22c0ed19f767d 100644 (file)
@@ -28,40 +28,8 @@ class ACLChecklist
 
 public:
 
-    /**
-     * State class.
-     * This abstract class defines the behaviour of
-     * async lookups - which can vary for different ACL types.
-     * Today, every state object must be a singleton.
-     * See NULLState for an example.
-     *
-     \note *no* state should be stored in the state object,
-     * they are used to change the behaviour of the checklist, not
-     * to hold information. If you need to store information in the
-     * state object, consider subclassing ACLChecklist, converting it
-     * to a composite, or changing the state objects from singletons to
-     * refcounted objects.
-     */
-
-    class AsyncState
-    {
-
-    public:
-        virtual void checkForAsync(ACLChecklist *) const = 0;
-        virtual ~AsyncState() {}
-    };
-
-    class NullState : public AsyncState
-    {
-
-    public:
-        static NullState *Instance();
-        void checkForAsync(ACLChecklist *) const override;
-        ~NullState() override {}
-
-    private:
-        static NullState _instance;
-    };
+    /// a function that initiates asynchronous ACL checks; see goAsync()
+    using AsyncStarter = void (ACLFilledChecklist &, const ACL &);
 
 public:
     ACLChecklist();
@@ -136,7 +104,7 @@ public:
 
     /// If slow lookups are allowed, switches into "async in progress" state.
     /// Otherwise, returns false; the caller is expected to handle the failure.
-    bool goAsync(AsyncState *);
+    bool goAsync(AsyncStarter, const ACL &);
 
     /// Matches (or resumes matching of) a child node while maintaning
     /// resumption breadcrumbs if a [grand]child node goes async.
@@ -188,9 +156,6 @@ private:
 
     void matchAndFinish();
 
-    void changeState(AsyncState *);
-    AsyncState *asyncState() const;
-
     const Acl::Tree *accessList;
 public:
 
@@ -199,7 +164,7 @@ public:
 
     /// Resumes non-blocking check started by nonBlockingCheck() and
     /// suspended until some async operation updated Squid state.
-    void resumeNonBlockingCheck(AsyncState *state);
+    void resumeNonBlockingCheck();
 
 private: /* internal methods */
     /// Position of a child node within an ACL tree.
@@ -232,7 +197,6 @@ private: /* internal methods */
 
     enum AsyncStage { asyncNone, asyncStarting, asyncRunning, asyncFailed };
     AsyncStage asyncStage_;
-    AsyncState *state_;
     Breadcrumb matchLoc_; ///< location of the node running matches() now
     Breadcrumb asyncLoc_; ///< currentNode_ that called goAsync()
     unsigned asyncLoopDepth_; ///< how many times the current async state has resumed
index 156cee05746b5dea35645e0dd467c99153804b85..410cab7073a9a6ef926968bd365c2388f1e33f76 100644 (file)
 #include "fqdncache.h"
 #include "HttpRequest.h"
 
-DestinationDomainLookup DestinationDomainLookup::instance_;
+static void LookupDone(const char *, const Dns::LookupDetails &, void *data);
 
-DestinationDomainLookup *
-DestinationDomainLookup::Instance()
+static void
+StartLookup(ACLFilledChecklist &cl, const ACL &)
 {
-    return &instance_;
+    fqdncache_nbgethostbyaddr(cl.dst_addr, LookupDone, &cl);
 }
 
-void
-DestinationDomainLookup::checkForAsync(ACLChecklist *cl) const
-{
-    ACLFilledChecklist *checklist = Filled(cl);
-    fqdncache_nbgethostbyaddr(checklist->dst_addr, LookupDone, checklist);
-}
-
-void
-DestinationDomainLookup::LookupDone(const char *, const Dns::LookupDetails &details, void *data)
+static void
+LookupDone(const char *, const Dns::LookupDetails &details, void *data)
 {
     ACLFilledChecklist *checklist = Filled((ACLChecklist*)data);
     checklist->markDestinationDomainChecked();
     checklist->request->recordLookup(details);
-    checklist->resumeNonBlockingCheck(DestinationDomainLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
 /* Acl::DestinationDomainCheck */
@@ -93,7 +86,7 @@ Acl::DestinationDomainCheck::match(ACLChecklist * const ch)
     } else if (!checklist->destinationDomainChecked()) {
         // TODO: Using AclMatchedName here is not OO correct. Should find a way to the current acl
         debugs(28, 3, "Can't yet compare '" << AclMatchedName << "' ACL for " << checklist->request->url.host());
-        if (checklist->goAsync(DestinationDomainLookup::Instance()))
+        if (checklist->goAsync(StartLookup, *this))
             return -1;
         // else fall through to "none" match, hiding the lookup failure (XXX)
     }
index 8910bf4ec0cebfccb064db4c739a7989bea8b19e..9143bc7a6c89c6f29f601f2514c5f58af59da6fa 100644 (file)
@@ -32,18 +32,5 @@ private:
 
 } // namespace Acl
 
-/// \ingroup ACLAPI
-class DestinationDomainLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static DestinationDomainLookup *Instance();
-    void checkForAsync(ACLChecklist *)const override;
-
-private:
-    static DestinationDomainLookup instance_;
-    static void LookupDone(const char *, const Dns::LookupDetails &, void *);
-};
-
 #endif /* SQUID_ACLDESTINATIONDOMAIN_H */
 
index 60b67da6af8758d6028e18f7b70d9b8a9fa0062e..fa4bf9d700f350e80823a36b3e9921f2d1805174 100644 (file)
@@ -76,7 +76,7 @@ ACLDestinationIP::match(ACLChecklist *cl)
     } else if (!checklist->request->flags.destinationIpLookedUp) {
         /* No entry in cache, lookup not attempted */
         debugs(28, 3, "can't yet compare '" << name << "' ACL for " << checklist->request->url.host());
-        if (checklist->goAsync(DestinationIPLookup::Instance()))
+        if (checklist->goAsync(StartLookup, *this))
             return -1;
         // else fall through to mismatch, hiding the lookup failure (XXX)
     }
@@ -84,27 +84,18 @@ ACLDestinationIP::match(ACLChecklist *cl)
     return 0;
 }
 
-DestinationIPLookup DestinationIPLookup::instance_;
-
-DestinationIPLookup *
-DestinationIPLookup::Instance()
-{
-    return &instance_;
-}
-
 void
-DestinationIPLookup::checkForAsync(ACLChecklist *cl)const
+ACLDestinationIP::StartLookup(ACLFilledChecklist &cl, const ACL &)
 {
-    ACLFilledChecklist *checklist = Filled(cl);
-    ipcache_nbgethostbyname(checklist->request->url.host(), LookupDone, checklist);
+    ipcache_nbgethostbyname(cl.request->url.host(), LookupDone, &cl);
 }
 
 void
-DestinationIPLookup::LookupDone(const ipcache_addrs *, const Dns::LookupDetails &details, void *data)
+ACLDestinationIP::LookupDone(const ipcache_addrs *, const Dns::LookupDetails &details, void *data)
 {
     ACLFilledChecklist *checklist = Filled((ACLChecklist*)data);
     checklist->request->flags.destinationIpLookedUp = true;
     checklist->request->recordLookup(details);
-    checklist->resumeNonBlockingCheck(DestinationIPLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
index 9d3453e0ff3355e328ff80021f9a6671b402691e..4282852e47627e1c6c6c7d75d8ebd5185cbd81cf 100644 (file)
 #include "acl/Ip.h"
 #include "ipcache.h"
 
-class DestinationIPLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static DestinationIPLookup *Instance();
-    void checkForAsync(ACLChecklist *)const override;
-
-private:
-    static DestinationIPLookup instance_;
-    static IPH LookupDone;
-};
-
 class ACLDestinationIP : public ACLIP
 {
     MEMPROXY_CLASS(ACLDestinationIP);
 
 public:
+    static void StartLookup(ACLFilledChecklist &, const ACL &);
+
     char const *typeString() const override;
     const Acl::Options &options() override;
     int match(ACLChecklist *checklist) override;
 
 private:
+    static void LookupDone(const ipcache_addrs *, const Dns::LookupDetails &, void *data);
+
     Acl::BooleanOptionValue lookupBanned; ///< are DNS lookups allowed?
 };
 
index ea892b3eacb6bcc088ea5d7dc5c839e57d301a1e..5f39e489f0b6541cfa799b1a8325287c4ceef6d8 100644 (file)
 #include "fqdncache.h"
 #include "HttpRequest.h"
 
-SourceDomainLookup SourceDomainLookup::instance_;
+static void LookupDone(const char *, const Dns::LookupDetails &, void *data);
 
-SourceDomainLookup *
-SourceDomainLookup::Instance()
+static void
+StartLookup(ACLFilledChecklist &checklist, const ACL &)
 {
-    return &instance_;
+    fqdncache_nbgethostbyaddr(checklist.src_addr, LookupDone, &checklist);
 }
 
-void
-SourceDomainLookup::checkForAsync(ACLChecklist *checklist) const
-{
-    fqdncache_nbgethostbyaddr(Filled(checklist)->src_addr, LookupDone, checklist);
-}
-
-void
-SourceDomainLookup::LookupDone(const char *, const Dns::LookupDetails &details, void *data)
+static void
+LookupDone(const char *, const Dns::LookupDetails &details, void *data)
 {
     ACLFilledChecklist *checklist = Filled((ACLChecklist*)data);
     checklist->markSourceDomainChecked();
     checklist->request->recordLookup(details);
-    checklist->resumeNonBlockingCheck(SourceDomainLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
 int
@@ -53,7 +47,7 @@ Acl::SourceDomainCheck::match(ACLChecklist * const ch)
     } else if (!checklist->sourceDomainChecked()) {
         // TODO: Using AclMatchedName here is not OO correct. Should find a way to the current acl
         debugs(28, 3, "aclMatchAcl: Can't yet compare '" << AclMatchedName << "' ACL for '" << checklist->src_addr << "'");
-        if (checklist->goAsync(SourceDomainLookup::Instance()))
+        if (checklist->goAsync(StartLookup, *this))
             return -1;
         // else fall through to "none" match, hiding the lookup failure (XXX)
     }
index 854d1a2eebadd4e1427d9b34dbe89eeecb6f43bb..9e7acc4572bf4ab9fde55439452425624abe9483 100644 (file)
@@ -27,17 +27,5 @@ public:
 
 } // namespace Acl
 
-class SourceDomainLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static SourceDomainLookup *Instance();
-    void checkForAsync(ACLChecklist *)const override;
-
-private:
-    static SourceDomainLookup instance_;
-    static void LookupDone(const char *, const Dns::LookupDetails &, void *);
-};
-
 #endif /* SQUID_ACLSOURCEDOMAIN_H */
 
index 9f202030a6a66820d8df1e9e585a8020489b493c..19847aae89e9428749d6717b135cbb96a8346f15 100644 (file)
@@ -25,7 +25,7 @@
  * \retval ACCESS_ALLOWED       user authenticated and authorized
  */
 Acl::Answer
-AuthenticateAcl(ACLChecklist *ch)
+AuthenticateAcl(ACLChecklist *ch, const ACL &acl)
 {
     ACLFilledChecklist *checklist = Filled(ch);
     const auto request = checklist->request;
@@ -68,7 +68,7 @@ AuthenticateAcl(ACLChecklist *ch)
         break;
 
     case AUTH_ACL_HELPER:
-        if (checklist->goAsync(ProxyAuthLookup::Instance()))
+        if (checklist->goAsync(ACLProxyAuth::StartLookup, acl))
             debugs(28, 4, "returning " << ACCESS_DUNNO << " sending credentials to helper.");
         else
             debugs(28, 2, "cannot go async; returning " << ACCESS_DUNNO);
index 7486a198d688bbd8e1d6e6e800c1502c50260847..dd8c4a71ac32e8ebabf2f506da1f9458d22138c6 100644 (file)
@@ -19,7 +19,7 @@
 
 class ACLChecklist;
 /// \ingroup AuthAPI
-Acl::Answer AuthenticateAcl(ACLChecklist *ch);
+Acl::Answer AuthenticateAcl(ACLChecklist *, const ACL &);
 
 #endif /* USE_AUTH */
 #endif /* SQUID_AUTH_ACL_H */
index b252a830e07929155e3ecdaa59c50881991494da..ee09cf9801fd1ffce18b350a0ec91c8315acee5c 100644 (file)
@@ -116,7 +116,7 @@ int
 ACLMaxUserIP::match(ACLChecklist *cl)
 {
     ACLFilledChecklist *checklist = Filled(cl);
-    auto answer = AuthenticateAcl(checklist);
+    auto answer = AuthenticateAcl(checklist, *this);
     int ti;
 
     // convert to tri-state ACL match 1,0,-1
index b61ee766ba79432c2251155f3a5be3d7d14abde6..e346852772cf19bc7c751f05b0950667862d5729 100644 (file)
@@ -52,7 +52,7 @@ ACLProxyAuth::parse()
 int
 ACLProxyAuth::match(ACLChecklist *checklist)
 {
-    auto answer = AuthenticateAcl(checklist);
+    auto answer = AuthenticateAcl(checklist, *this);
 
     // convert to tri-state ACL match 1,0,-1
     switch (answer) {
@@ -102,29 +102,19 @@ ACLProxyAuth::valid() const
     return true;
 }
 
-ProxyAuthLookup ProxyAuthLookup::instance_;
-
-ProxyAuthLookup *
-ProxyAuthLookup::Instance()
-{
-    return &instance_;
-}
-
 void
-ProxyAuthLookup::checkForAsync(ACLChecklist *cl) const
+ACLProxyAuth::StartLookup(ACLFilledChecklist &cl, const ACL &)
 {
-    ACLFilledChecklist *checklist = Filled(cl);
-
     debugs(28, 3, "checking password via authenticator");
 
     /* make sure someone created auth_user_request for us */
-    assert(checklist->auth_user_request != nullptr);
-    assert(checklist->auth_user_request->valid());
-    checklist->auth_user_request->start(checklist->request.getRaw(), checklist->al, LookupDone, checklist);
+    assert(cl.auth_user_request != nullptr);
+    assert(cl.auth_user_request->valid());
+    cl.auth_user_request->start(cl.request.getRaw(), cl.al, LookupDone, &cl);
 }
 
 void
-ProxyAuthLookup::LookupDone(void *data)
+ACLProxyAuth::LookupDone(void *data)
 {
     ACLFilledChecklist *checklist = Filled(static_cast<ACLChecklist*>(data));
 
@@ -139,7 +129,7 @@ ProxyAuthLookup::LookupDone(void *data)
         }
     }
 
-    checklist->resumeNonBlockingCheck(ProxyAuthLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
 int
index 7febb609242e7d5d511271e5b17a535c1db7c20b..66c7d3dd366f4e6896f259b1d6c3026f38e3f629 100644 (file)
 #include "acl/Checklist.h"
 #include "acl/Data.h"
 
-class ProxyAuthLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static ProxyAuthLookup *Instance();
-    void checkForAsync(ACLChecklist *) const override;
-
-private:
-    static ProxyAuthLookup instance_;
-    static void LookupDone(void *data);
-};
-
 class ACLProxyAuth : public ACL
 {
     MEMPROXY_CLASS(ACLProxyAuth);
 
 public:
+    static void StartLookup(ACLFilledChecklist &, const ACL &);
+
     ~ACLProxyAuth() override;
     ACLProxyAuth(ACLData<char const *> *, char const *);
 
@@ -47,6 +37,8 @@ public:
     int matchForCache(ACLChecklist *checklist) override;
 
 private:
+    static void LookupDone(void *data);
+
     /* ACL API */
     const Acl::Options &lineOptions() override;
 
index 87e3bf154723efe1c21ecd38e7aab7a61d742627..ed7025b3e2991cc8ac2f1d012c6fc6e8022c7fe5 100644 (file)
@@ -55,7 +55,6 @@
 #define DEFAULT_EXTERNAL_ACL_CHILDREN 5
 #endif
 
-static char *makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data);
 static void external_acl_cache_delete(external_acl * def, const ExternalACLEntryPointer &entry);
 static int external_acl_entry_expired(external_acl * def, const ExternalACLEntryPointer &entry);
 static int external_acl_grace_expired(external_acl * def, const ExternalACLEntryPointer &entry);
@@ -593,8 +592,9 @@ copyResultsFromEntry(const HttpRequest::Pointer &req, const ExternalACLEntryPoin
     }
 }
 
-static Acl::Answer
-aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
+// TODO: Diff reduction. Rename this helper method to match_() or similar.
+Acl::Answer
+ACLExternal::aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch) const
 {
     debugs(82, 9, "acl=\"" << acl->def->name << "\"");
     ExternalACLEntryPointer entry = ch->extacl_entry;
@@ -630,7 +630,7 @@ aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
         if (acl->def->require_auth) {
             /* Make sure the user is authenticated */
             debugs(82, 3, acl->def->name << " check user authenticated.");
-            const auto ti = AuthenticateAcl(ch);
+            const auto ti = AuthenticateAcl(ch, *this);
             if (!ti.allowed()) {
                 debugs(82, 2, acl->def->name << " user not authenticated (" << ti << ")");
                 return ti;
@@ -653,7 +653,7 @@ aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
 
         if (entry != nullptr && external_acl_grace_expired(acl->def, entry)) {
             // refresh in the background
-            ExternalACLLookup::Start(ch, acl, true);
+            startLookup(ch, acl, true);
             debugs(82, 4, "no need to wait for the refresh of '" <<
                    key << "' in '" << acl->def->name << "' (ch=" << ch << ").");
         }
@@ -664,7 +664,7 @@ aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
             // TODO: All other helpers allow temporary overload. Should not we?
             if (!acl->def->theHelper->willOverload()) {
                 debugs(82, 2, "\"" << key << "\": queueing a call.");
-                if (!ch->goAsync(ExternalACLLookup::Instance()))
+                if (!ch->goAsync(StartLookup, *this))
                     debugs(82, 2, "\"" << key << "\": no async support!");
                 debugs(82, 2, "\"" << key << "\": return -1.");
                 return ACCESS_DUNNO; // expired cached or simply absent entry
@@ -757,8 +757,8 @@ external_acl_cache_touch(external_acl * def, const ExternalACLEntryPointer &entr
     dlinkAdd(e, &entry->lru, &def->lru_list);
 }
 
-static char *
-makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data)
+char *
+ACLExternal::makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data) const
 {
     static MemBuf mb;
     mb.reset();
@@ -799,7 +799,7 @@ makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data)
             if (!*ch->rfc931) {
                 // if we fail to go async, we still return NULL and the caller
                 // will detect the failure in ACLExternal::match().
-                (void)ch->goAsync(IdentLookup::Instance());
+                (void)ch->goAsync(ACLIdent::StartLookup, *this);
                 return nullptr;
             }
         }
@@ -1011,18 +1011,22 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
     } while (state);
 }
 
+/// Asks the helper (if needed) or returns the [cached] result (otherwise).
+/// Does not support "background" lookups. See also: ACLExternal::Start().
 void
-ACLExternal::ExternalAclLookup(ACLChecklist *checklist, ACLExternal * me)
+ACLExternal::StartLookup(ACLFilledChecklist &checklist, const ACL &acl)
 {
-    ExternalACLLookup::Start(checklist, me->data, false);
+    const auto &me = dynamic_cast<const ACLExternal&>(acl);
+    me.startLookup(&checklist, me.data, false);
 }
 
+// If possible, starts an asynchronous lookup of an external ACL.
+// Otherwise, asserts (or bails if background refresh is requested).
 void
-ExternalACLLookup::Start(ACLChecklist *checklist, external_acl_data *acl, bool inBackground)
+ACLExternal::startLookup(ACLFilledChecklist *ch, external_acl_data *acl, bool inBackground) const
 {
     external_acl *def = acl->def;
 
-    ACLFilledChecklist *ch = Filled(checklist);
     const char *key = makeExternalAclKey(ch, acl);
     assert(key); // XXX: will fail if EXT_ACL_IDENT case needs an async lookup
 
@@ -1053,8 +1057,8 @@ ExternalACLLookup::Start(ACLChecklist *checklist, external_acl_data *acl, bool i
     externalAclState *state = new externalAclState(def, key);
 
     if (!inBackground) {
-        state->callback = &ExternalACLLookup::LookupDone;
-        state->callback_data = cbdataReference(checklist);
+        state->callback = &LookupDone;
+        state->callback_data = cbdataReference(ch);
     }
 
     if (oldstate) {
@@ -1139,32 +1143,13 @@ externalAclShutdown(void)
     }
 }
 
-ExternalACLLookup ExternalACLLookup::instance_;
-ExternalACLLookup *
-ExternalACLLookup::Instance()
-{
-    return &instance_;
-}
-
-void
-ExternalACLLookup::checkForAsync(ACLChecklist *checklist)const
-{
-    /* TODO: optimise this - we probably have a pointer to this
-     * around somewhere */
-    ACL *acl = ACL::FindByName(AclMatchedName);
-    assert(acl);
-    ACLExternal *me = dynamic_cast<ACLExternal *> (acl);
-    assert (me);
-    ACLExternal::ExternalAclLookup(checklist, me);
-}
-
 /// Called when an async lookup returns
 void
-ExternalACLLookup::LookupDone(void *data, const ExternalACLEntryPointer &result)
+ACLExternal::LookupDone(void *data, const ExternalACLEntryPointer &result)
 {
     ACLFilledChecklist *checklist = Filled(static_cast<ACLChecklist*>(data));
     checklist->extacl_entry = result;
-    checklist->resumeNonBlockingCheck(ExternalACLLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
 ACLExternal::ACLExternal(char const *theClass) : data(nullptr), class_(xstrdup(theClass))
index a3f68860ea385375be89a787ed0cd17223799950..d2c43dc18a9b757cceaf3e84c53c7c446cc315b8 100644 (file)
@@ -55,13 +55,13 @@ ACLIdent::parse()
 int
 ACLIdent::match(ACLChecklist *cl)
 {
-    ACLFilledChecklist *checklist = Filled(cl);
+    const auto checklist = Filled(cl);
     if (checklist->rfc931[0]) {
         return data->match(checklist->rfc931);
     } else if (checklist->conn() != nullptr && checklist->conn()->clientConnection != nullptr && checklist->conn()->clientConnection->rfc931[0]) {
         return data->match(checklist->conn()->clientConnection->rfc931);
     } else if (checklist->conn() != nullptr && Comm::IsConnOpen(checklist->conn()->clientConnection)) {
-        if (checklist->goAsync(IdentLookup::Instance())) {
+        if (checklist->goAsync(StartLookup, *this)) {
             debugs(28, 3, "switching to ident lookup state");
             return -1;
         }
@@ -87,27 +87,18 @@ ACLIdent::empty () const
     return data->empty();
 }
 
-IdentLookup IdentLookup::instance_;
-
-IdentLookup *
-IdentLookup::Instance()
-{
-    return &instance_;
-}
-
 void
-IdentLookup::checkForAsync(ACLChecklist *cl)const
+ACLIdent::StartLookup(ACLFilledChecklist &cl, const ACL &)
 {
-    ACLFilledChecklist *checklist = Filled(cl);
-    const ConnStateData *conn = checklist->conn();
+    const ConnStateData *conn = cl.conn();
     // check that ACLIdent::match() tested this lookup precondition
     assert(conn && Comm::IsConnOpen(conn->clientConnection));
     debugs(28, 3, "Doing ident lookup" );
-    Ident::Start(checklist->conn()->clientConnection, LookupDone, checklist);
+    Ident::Start(cl.conn()->clientConnection, LookupDone, &cl);
 }
 
 void
-IdentLookup::LookupDone(const char *ident, void *data)
+ACLIdent::LookupDone(const char *ident, void *data)
 {
     ACLFilledChecklist *checklist = Filled(static_cast<ACLChecklist*>(data));
 
@@ -124,7 +115,7 @@ IdentLookup::LookupDone(const char *ident, void *data)
     if (checklist->conn() != nullptr && checklist->conn()->clientConnection != nullptr && !checklist->conn()->clientConnection->rfc931[0])
         xstrncpy(checklist->conn()->clientConnection->rfc931, checklist->rfc931, USER_IDENT_SZ);
 
-    checklist->resumeNonBlockingCheck(IdentLookup::Instance());
+    checklist->resumeNonBlockingCheck();
 }
 
 #endif /* USE_IDENT */
index f898aae870e7f13a16472842e9555977c27be6c8..82cedb417ca422cfb635637aa544e46016d20559 100644 (file)
 
 #if USE_IDENT
 
-#include "acl/Checklist.h"
-
-/// \ingroup ACLAPI
-class IdentLookup : public ACLChecklist::AsyncState
-{
-
-public:
-    static IdentLookup *Instance();
-    void checkForAsync(ACLChecklist *)const override;
-
-private:
-    static IdentLookup instance_;
-    static void LookupDone(const char *ident, void *data);
-};
-
 #include "acl/Acl.h"
+#include "acl/Checklist.h"
 #include "acl/Data.h"
 
 /// \ingroup ACLAPI
@@ -35,6 +21,8 @@ class ACLIdent : public ACL
     MEMPROXY_CLASS(ACLIdent);
 
 public:
+    static void StartLookup(ACLFilledChecklist &, const ACL &);
+
     ACLIdent(ACLData<char const *> *newData, char const *);
     ~ACLIdent() override;
 
@@ -47,6 +35,8 @@ public:
     bool empty () const override;
 
 private:
+    static void LookupDone(const char *ident, void *data);
+
     /* ACL API */
     const Acl::Options &lineOptions() override;
 
index 13601e9f30155312243e493fc5c615f1e8ed442c..8bbe78a0f6315b5d87d9b1251e88bdec9ab4c137 100644 (file)
@@ -22,11 +22,6 @@ bool ACLExternal::valid () const STUB_RETVAL(false)
 bool ACLExternal::empty () const STUB_RETVAL(false)
 int ACLExternal::match(ACLChecklist *) STUB_RETVAL(0)
 SBufList ACLExternal::dump() const STUB_RETVAL(SBufList())
-void ACLExternal::ExternalAclLookup(ACLChecklist *, ACLExternal *) STUB
-void ExternalACLLookup::Start(ACLChecklist *, external_acl_data *, bool) STUB
 void externalAclInit(void) STUB_NOP
 void externalAclShutdown(void) STUB_NOP
-ExternalACLLookup * ExternalACLLookup::Instance() STUB_RETVAL(nullptr)
-void ExternalACLLookup::checkForAsync(ACLChecklist *) const STUB
-void ExternalACLLookup::LookupDone(void *, const ExternalACLEntryPointer &) STUB
 
index 3185bbe542433f9457376cf5086f5e16c5dfa3f5..67262f0b4112bf739fb2afe4861759f977bafddc 100644 (file)
@@ -15,7 +15,7 @@
 #include "acl/Acl.h" /* for Acl::Answer */
 
 #include "auth/Acl.h"
-Acl::Answer AuthenticateAcl(ACLChecklist *) STUB_RETVAL(ACCESS_DENIED)
+Acl::Answer AuthenticateAcl(ACLChecklist *, const ACL &) STUB_RETVAL(ACCESS_DENIED)
 
 #include "auth/AclMaxUserIp.h"
 ACLMaxUserIP::ACLMaxUserIP (char const *) STUB
@@ -37,9 +37,6 @@ int ACLProxyAuth::match(ACLChecklist *) STUB_RETVAL(0)
 SBufList ACLProxyAuth::dump() const STUB_RETVAL(SBufList())
 bool ACLProxyAuth::empty () const STUB_RETVAL(false)
 bool ACLProxyAuth::valid () const STUB_RETVAL(false)
-ProxyAuthLookup * ProxyAuthLookup::Instance() STUB_RETVAL(nullptr)
-void ProxyAuthLookup::checkForAsync(ACLChecklist *) const STUB
-void ProxyAuthLookup::LookupDone(void *) STUB
 int ACLProxyAuth::matchForCache(ACLChecklist *) STUB_RETVAL(0)
 int ACLProxyAuth::matchProxyAuth(ACLChecklist *) STUB_RETVAL(0)
 const Acl::Options &ACLProxyAuth::lineOptions() STUB_RETVAL(Acl::NoOptions())