From 5fb6afeceee81096e03b43766f8e7fdb63626592 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Wed, 15 Nov 2023 09:16:02 +0000 Subject: [PATCH] Reduce ACLChecklist::AsyncState to a function pointer (#1576) 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)! --- src/ExternalACL.h | 29 +++++------------ src/acl/Asn.cc | 2 +- src/acl/Checklist.cc | 47 +++------------------------- src/acl/Checklist.h | 44 +++----------------------- src/acl/DestinationDomain.cc | 23 +++++--------- src/acl/DestinationDomain.h | 13 -------- src/acl/DestinationIp.cc | 19 +++--------- src/acl/DestinationIp.h | 16 +++------- src/acl/SourceDomain.cc | 22 +++++-------- src/acl/SourceDomain.h | 12 ------- src/auth/Acl.cc | 4 +-- src/auth/Acl.h | 2 +- src/auth/AclMaxUserIp.cc | 2 +- src/auth/AclProxyAuth.cc | 24 +++++--------- src/auth/AclProxyAuth.h | 16 +++------- src/external_acl.cc | 57 +++++++++++++--------------------- src/ident/AclIdent.cc | 23 +++++--------- src/ident/AclIdent.h | 20 +++--------- src/tests/stub_external_acl.cc | 5 --- src/tests/stub_libauth_acls.cc | 5 +-- 20 files changed, 92 insertions(+), 293 deletions(-) diff --git a/src/ExternalACL.h b/src/ExternalACL.h index d3b7689d66..0ce7461562 100644 --- a/src/ExternalACL.h +++ b/src/ExternalACL.h @@ -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_; }; diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc index f1e318b1c7..663e74d38c 100644 --- a/src/acl/Asn.cc +++ b/src/acl/Asn.cc @@ -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) } diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index d159dc2d60..813e863c01 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -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 diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 57800c819e..ae0096cc0b 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -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 diff --git a/src/acl/DestinationDomain.cc b/src/acl/DestinationDomain.cc index 156cee0574..410cab7073 100644 --- a/src/acl/DestinationDomain.cc +++ b/src/acl/DestinationDomain.cc @@ -16,28 +16,21 @@ #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) } diff --git a/src/acl/DestinationDomain.h b/src/acl/DestinationDomain.h index 8910bf4ec0..9143bc7a6c 100644 --- a/src/acl/DestinationDomain.h +++ b/src/acl/DestinationDomain.h @@ -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 */ diff --git a/src/acl/DestinationIp.cc b/src/acl/DestinationIp.cc index 60b67da6af..fa4bf9d700 100644 --- a/src/acl/DestinationIp.cc +++ b/src/acl/DestinationIp.cc @@ -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(); } diff --git a/src/acl/DestinationIp.h b/src/acl/DestinationIp.h index 9d3453e0ff..4282852e47 100644 --- a/src/acl/DestinationIp.h +++ b/src/acl/DestinationIp.h @@ -13,28 +13,20 @@ #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? }; diff --git a/src/acl/SourceDomain.cc b/src/acl/SourceDomain.cc index ea892b3eac..5f39e489f0 100644 --- a/src/acl/SourceDomain.cc +++ b/src/acl/SourceDomain.cc @@ -17,27 +17,21 @@ #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) } diff --git a/src/acl/SourceDomain.h b/src/acl/SourceDomain.h index 854d1a2eeb..9e7acc4572 100644 --- a/src/acl/SourceDomain.h +++ b/src/acl/SourceDomain.h @@ -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 */ diff --git a/src/auth/Acl.cc b/src/auth/Acl.cc index 9f202030a6..19847aae89 100644 --- a/src/auth/Acl.cc +++ b/src/auth/Acl.cc @@ -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); diff --git a/src/auth/Acl.h b/src/auth/Acl.h index 7486a198d6..dd8c4a71ac 100644 --- a/src/auth/Acl.h +++ b/src/auth/Acl.h @@ -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 */ diff --git a/src/auth/AclMaxUserIp.cc b/src/auth/AclMaxUserIp.cc index b252a830e0..ee09cf9801 100644 --- a/src/auth/AclMaxUserIp.cc +++ b/src/auth/AclMaxUserIp.cc @@ -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 diff --git a/src/auth/AclProxyAuth.cc b/src/auth/AclProxyAuth.cc index b61ee766ba..e346852772 100644 --- a/src/auth/AclProxyAuth.cc +++ b/src/auth/AclProxyAuth.cc @@ -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(data)); @@ -139,7 +129,7 @@ ProxyAuthLookup::LookupDone(void *data) } } - checklist->resumeNonBlockingCheck(ProxyAuthLookup::Instance()); + checklist->resumeNonBlockingCheck(); } int diff --git a/src/auth/AclProxyAuth.h b/src/auth/AclProxyAuth.h index 7febb60924..66c7d3dd36 100644 --- a/src/auth/AclProxyAuth.h +++ b/src/auth/AclProxyAuth.h @@ -15,23 +15,13 @@ #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 *); @@ -47,6 +37,8 @@ public: int matchForCache(ACLChecklist *checklist) override; private: + static void LookupDone(void *data); + /* ACL API */ const Acl::Options &lineOptions() override; diff --git a/src/external_acl.cc b/src/external_acl.cc index 87e3bf1547..ed7025b3e2 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -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(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 (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(data)); checklist->extacl_entry = result; - checklist->resumeNonBlockingCheck(ExternalACLLookup::Instance()); + checklist->resumeNonBlockingCheck(); } ACLExternal::ACLExternal(char const *theClass) : data(nullptr), class_(xstrdup(theClass)) diff --git a/src/ident/AclIdent.cc b/src/ident/AclIdent.cc index a3f68860ea..d2c43dc18a 100644 --- a/src/ident/AclIdent.cc +++ b/src/ident/AclIdent.cc @@ -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(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 */ diff --git a/src/ident/AclIdent.h b/src/ident/AclIdent.h index f898aae870..82cedb417c 100644 --- a/src/ident/AclIdent.h +++ b/src/ident/AclIdent.h @@ -11,22 +11,8 @@ #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 *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; diff --git a/src/tests/stub_external_acl.cc b/src/tests/stub_external_acl.cc index 13601e9f30..8bbe78a0f6 100644 --- a/src/tests/stub_external_acl.cc +++ b/src/tests/stub_external_acl.cc @@ -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 diff --git a/src/tests/stub_libauth_acls.cc b/src/tests/stub_libauth_acls.cc index 3185bbe542..67262f0b41 100644 --- a/src/tests/stub_libauth_acls.cc +++ b/src/tests/stub_libauth_acls.cc @@ -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()) -- 2.47.2