From: Alex Rousskov Date: Mon, 8 Apr 2024 22:06:02 +0000 (+0000) Subject: Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766) X-Git-Tag: SQUID_7_0_1~144 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=25aa6c9a505f0b0f8a2c68b10d7a0933680f3772;p=thirdparty%2Fsquid.git Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766) AclMatchedName global has been localized into a regular Acl::Answer data member (Acl::Answer maintains the result of ACLChecklist evaluations). This long overdue change resolves an old TODO and XXXs, paving the way for Acl::Node reference counting. No significant functionality changes are expected, but it is possible that some deny_info configurations will now be handled better in reconfiguration corner cases (because Squid no longer forgets the name of the last checked ACL when a slow ACL check crosses reconfiguration barrier). Most of these changes are performance-neutral or -positive because they eliminate or reduce memory allocations and associated name copying (and more reduction will become possible after upgrading squid.conf parsers to use SBuf). This change adds SBuf object copies when Acl::Answer is propagated to ACLCB callbacks, but those read-only copies are cheap. Also renamed and polished aclGetDenyInfoPage() because we had to update its parameter type (to supply the last evaluated ACL). All callers were also supplying the same first argument (that is unlikely to change in the foreseeable future); that argument is now gone. We did not fix the redirect_allowed name and debugs(): Fixing that behavior deserves a dedicated change. Also polished legacy aclIsProxyAuth() profile and description because we have to change the parameter type (to supply the last evaluated ACL). Also removed 63-character aclname parameter limit for acl directives. --- diff --git a/src/AccessLogEntry.cc b/src/AccessLogEntry.cc index dfd03d6d3b..8481b37c80 100644 --- a/src/AccessLogEntry.cc +++ b/src/AccessLogEntry.cc @@ -133,8 +133,6 @@ AccessLogEntry::~AccessLogEntry() safe_free(headers.adapted_request); HTTPMSGUNLOCK(adapted_request); - safe_free(lastAclName); - HTTPMSGUNLOCK(request); #if ICAP_CLIENT HTTPMSGUNLOCK(icap.reply); diff --git a/src/AccessLogEntry.h b/src/AccessLogEntry.h index eb51087398..cab6ae8b00 100644 --- a/src/AccessLogEntry.h +++ b/src/AccessLogEntry.h @@ -188,7 +188,7 @@ public: } adapt; #endif - const char *lastAclName = nullptr; ///< string for external_acl_type %ACL format code + SBuf lastAclName; ///< string for external_acl_type %ACL format code SBuf lastAclData; ///< string for external_acl_type %DATA format code HierarchyLogEntry hier; diff --git a/src/FwdState.cc b/src/FwdState.cc index 34cbd873ca..94d9610437 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -353,9 +353,7 @@ FwdState::Start(const Comm::ConnectionPointer &clientConn, StoreEntry *entry, Ht ch.src_addr = request->client_addr; ch.syncAle(request, nullptr); if (ch.fastCheck().denied()) { - err_type page_id; - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); - + auto page_id = FindDenyInfoPage(ch.currentAnswer(), true); if (page_id == ERR_NONE) page_id = ERR_FORWARDING_DENIED; diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index d2e9c5faec..b08ab442e5 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -27,8 +27,6 @@ #include #include -const char *AclMatchedName = nullptr; - namespace Acl { /// Acl::Node type name comparison functor @@ -115,6 +113,14 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey) Here()); } +const SBuf & +Acl::Answer::lastCheckDescription() const +{ + static const auto none = new SBuf("[no-ACL]"); + // no value_or() because it would create a new SBuf object here + return lastCheckedName ? *lastCheckedName : *none; +} + /* Acl::ParsingContext */ ScopedId @@ -143,27 +149,39 @@ void Acl::Node::operator delete(void *) fatal ("unusable Acl::Node::delete"); } -Acl::Node * -Acl::Node::FindByName(const char *name) +/// implements both Acl::Node::FindByName() variations +template +static Acl::Node * +FindByNameT(const SBufOrCString name) { - debugs(28, 9, "name=" << name); - - for (auto *a = Config.aclList; a; a = a->next) - if (!strcasecmp(a->name, name)) + for (auto a = Config.aclList; a; a = a->next) { + if (a->name.caseCmp(name) == 0) { + debugs(28, 8, "found " << a->name); return a; + } + } - debugs(28, 9, "found no match"); - + debugs(28, 8, "cannot find " << name); return nullptr; } +Acl::Node * +Acl::Node::FindByName(const SBuf &name) +{ + return FindByNameT(name); +} + +Acl::Node * +Acl::Node::FindByName(const char *name) +{ + return FindByNameT(name); +} + Acl::Node::Node() : cfgline(nullptr), next(nullptr), registered(false) -{ - *name = 0; -} +{} bool Acl::Node::valid() const @@ -176,10 +194,7 @@ Acl::Node::matches(ACLChecklist *checklist) const { debugs(28, 5, "checking " << name); - // XXX: AclMatchedName does not contain a matched ACL name when the acl - // does not match. It contains the last (usually leaf) ACL name checked - // (or is NULL if no ACLs were checked). - AclMatchedName = name; + checklist->setLastCheckedName(name); int result = 0; if (!checklist->hasAle() && requiresAle()) { @@ -206,11 +221,9 @@ Acl::Node::matches(ACLChecklist *checklist) const } void -Acl::Node::context(const char *aName, const char *aCfgLine) +Acl::Node::context(const SBuf &aName, const char *aCfgLine) { - name[0] = '\0'; - if (aName) - xstrncpy(name, aName, ACL_NAME_SZ-1); + name = aName; safe_free(cfgline); if (aCfgLine) cfgline = xstrdup(aCfgLine); @@ -230,22 +243,15 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head) return; } - if (strlen(t) >= ACL_NAME_SZ) { - debugs(28, DBG_CRITICAL, "aclParseAclLine: aclParseAclLine: ACL name '" << t << - "' too long, max " << ACL_NAME_SZ - 1 << " characters supported"); - parser.destruct(); - return; - } - SBuf aclname(t); CallParser(ParsingContext::Pointer::Make(aclname), [&] { - ParseNamed(parser, head, aclname.c_str()); // TODO: Convert Node::name to SBuf + ParseNamed(parser, head, aclname); }); } /// parses acl directive parts that follow aclname void -Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * const aclname) +Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &aclname) { /* snarf the ACL type */ const char *theType; @@ -278,7 +284,7 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * con } theType = "localport"; debugs(28, DBG_IMPORTANT, "WARNING: UPGRADE: ACL 'myport' type has been renamed to 'localport' and matches the port the client connected to."); - } else if (strcmp(theType, "proto") == 0 && strcmp(aclname, "manager") == 0) { + } else if (strcmp(theType, "proto") == 0 && aclname.cmp("manager") == 0) { // ACL manager is now a built-in and has a different type. debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: UPGRADE: ACL 'manager' is now a built-in ACL. Remove it from your config file."); return; // ignore the line @@ -446,7 +452,6 @@ Acl::Node::~Node() { debugs(28, 3, "freeing ACL " << name); safe_free(cfgline); - AclMatchedName = nullptr; // in case it was pointing to our name } void diff --git a/src/acl/Acl.h b/src/acl/Acl.h index f8345efe28..130682b2c4 100644 --- a/src/acl/Acl.h +++ b/src/acl/Acl.h @@ -12,9 +12,10 @@ #include "acl/forward.h" #include "defines.h" #include "dlink.h" -#include "sbuf/forward.h" +#include "sbuf/SBuf.h" #include +#include #include namespace Acl { @@ -66,7 +67,7 @@ public: return !(*this == aCode); } - bool operator ==(const Answer allow) const { + bool operator ==(const Answer &allow) const { return code == allow.code && kind == allow.kind; } @@ -89,6 +90,9 @@ public: /// whether Squid is uncertain about the allowed() or denied() answer bool conflicted() const { return !allowed() && !denied(); } + /// describes the ACL that was evaluated last while obtaining this answer (for debugging) + const SBuf &lastCheckDescription() const; + aclMatchCode code = ACCESS_DUNNO; ///< ACCESS_* code /// the matched custom access list verb (or zero) @@ -96,10 +100,13 @@ public: /// whether we were computed by the "negate the last explicit action" rule bool implicit = false; + + /// the name of the ACL (if any) that was evaluated last while obtaining this answer + std::optional lastCheckedName; }; inline std::ostream & -operator <<(std::ostream &o, const Answer a) +operator <<(std::ostream &o, const Answer &a) { switch (a) { case ACCESS_DENIED: @@ -136,9 +143,5 @@ public: void *acl_data; }; -/// \ingroup ACLAPI -/// XXX: find a way to remove or at least use a refcounted Acl::Node pointer -extern const char *AclMatchedName; /* NULL */ - #endif /* SQUID_SRC_ACL_ACL_H */ diff --git a/src/acl/AllOf.cc b/src/acl/AllOf.cc index b9e92b50e2..731f56781b 100644 --- a/src/acl/AllOf.cc +++ b/src/acl/AllOf.cc @@ -14,6 +14,7 @@ #include "cache_cf.h" #include "MemBuf.h" #include "sbuf/SBuf.h" +#include "sbuf/Stream.h" char const * Acl::AllOf::typeString() const @@ -56,13 +57,8 @@ Acl::AllOf::parse() } else if (oldNode) { // this acl saw a single line before; create a new OR inner node - MemBuf wholeCtx; - wholeCtx.init(); - wholeCtx.appendf("(%s lines)", name); - wholeCtx.terminate(); - Acl::OrNode *newWhole = new Acl::OrNode; - newWhole->context(wholeCtx.content(), oldNode->cfgline); + newWhole->context(ToSBuf('(', name, " lines)"), oldNode->cfgline); newWhole->add(oldNode); // old (i.e. first) line nodes.front() = whole = newWhole; aclRegister(newWhole); @@ -74,13 +70,8 @@ Acl::AllOf::parse() assert(whole); const int lineId = whole->childrenCount() + 1; - MemBuf lineCtx; - lineCtx.init(); - lineCtx.appendf("(%s line #%d)", name, lineId); - lineCtx.terminate(); - Acl::AndNode *line = new AndNode; - line->context(lineCtx.content(), config_input_line); + line->context(ToSBuf('(', name, " line #", lineId, ')'), config_input_line); line->lineParse(); whole->add(line); diff --git a/src/acl/BoolOps.cc b/src/acl/BoolOps.cc index e117965422..e418c567b2 100644 --- a/src/acl/BoolOps.cc +++ b/src/acl/BoolOps.cc @@ -17,10 +17,9 @@ Acl::NotNode::NotNode(Acl::Node *acl) { assert(acl); - Must(strlen(acl->name) <= sizeof(name)-2); - name[0] = '!'; - name[1] = '\0'; - xstrncpy(&name[1], acl->name, sizeof(name)-1); // -1 for '!' + name.reserveCapacity(1 + acl->name.length()); + name.append('!'); + name.append(acl->name); add(acl); } @@ -56,7 +55,7 @@ SBufList Acl::NotNode::dump() const { SBufList text; - text.push_back(SBuf(name)); + text.push_back(name); return text; } diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc index 3e697881f6..c04014500c 100644 --- a/src/acl/Checklist.cc +++ b/src/acl/Checklist.cc @@ -60,6 +60,7 @@ ACLChecklist::markFinished(const Acl::Answer &finalAnswer, const char *reason) assert (!finished() && !asyncInProgress()); finished_ = true; answer_ = finalAnswer; + answer_.lastCheckedName = lastCheckedName_; debugs(28, 3, this << " answer " << answer_ << " for " << reason); } @@ -74,7 +75,7 @@ ACLChecklist::preCheck(const char *what) occupied_ = true; asyncLoopDepth_ = 0; - AclMatchedName = nullptr; + lastCheckedName_.reset(); finished_ = false; } @@ -154,7 +155,7 @@ ACLChecklist::goAsync(AsyncStarter starter, const Acl::Node &acl) // ACLFilledChecklist overwrites this to unclock something before we // "delete this" void -ACLChecklist::checkCallback(Acl::Answer answer) +ACLChecklist::checkCallback(const Acl::Answer &answer) { ACLCB *callback_; void *cbdata_; diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h index 0ee5844114..3f7fcd96bf 100644 --- a/src/acl/Checklist.h +++ b/src/acl/Checklist.h @@ -12,6 +12,8 @@ #include "acl/Acl.h" #include "acl/InnerNode.h" #include "cbdata.h" + +#include #include #include @@ -152,9 +154,12 @@ public: return old; } + /// remember the name of the last ACL being evaluated + void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; } + private: /// Calls non-blocking check callback with the answer and destroys self. - void checkCallback(Acl::Answer answer); + void checkCallback(const Acl::Answer &answer); void matchAndFinish(); @@ -209,6 +214,9 @@ private: /* internal methods */ std::stack matchPath; /// the list of actions which must ignored during acl checks std::vector bannedActions_; + + /// the name of the last evaluated ACL (if any ACLs were evaluated) + std::optional lastCheckedName_; }; #endif /* SQUID_SRC_ACL_CHECKLIST_H */ diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc index fa886af756..fb0492cf41 100644 --- a/src/acl/Gadgets.cc +++ b/src/acl/Gadgets.cc @@ -25,6 +25,7 @@ #include "errorpage.h" #include "globals.h" #include "HttpRequest.h" +#include "SquidConfig.h" #include "src/sbuf/Stream.h" #include @@ -34,20 +35,17 @@ using AclSet = std::set; /// Accumulates all ACLs to facilitate their clean deletion despite reuse. static AclSet *RegisteredAcls; // TODO: Remove when ACLs are refcounted -/* does name lookup, returns page_id */ err_type -aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allowed) +FindDenyInfoPage(const Acl::Answer &answer, const bool redirect_allowed) { - if (!name) { - debugs(28, 3, "ERR_NONE due to a NULL name"); + if (!answer.lastCheckedName) { + debugs(28, 3, "ERR_NONE because access was denied without evaluating ACLs"); return ERR_NONE; } - AclDenyInfoList *A = nullptr; - - debugs(28, 8, "got called for " << name); + const auto &name = *answer.lastCheckedName; - for (A = *head; A; A = A->next) { + for (auto A = Config.denyInfoList; A; A = A->next) { if (!redirect_allowed && strchr(A->err_page_name, ':') ) { debugs(28, 8, "Skip '" << A->err_page_name << "' 30x redirects not allowed as response here."); continue; @@ -55,33 +53,30 @@ aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allow for (const auto &aclName: A->acl_list) { if (aclName.cmp(name) == 0) { - debugs(28, 8, "match on " << name); + debugs(28, 8, "matched " << name << "; returning " << A->err_page_id << ' ' << A->err_page_name); return A->err_page_id; } } } - debugs(28, 8, "aclGetDenyInfoPage: no match"); + debugs(28, 8, "no match for " << name << (Config.denyInfoList ? "" : "; no deny_info rules")); return ERR_NONE; } -/* does name lookup, returns if it is a proxy_auth acl */ -int -aclIsProxyAuth(const char *name) +bool +aclIsProxyAuth(const std::optional &name) { if (!name) { - debugs(28, 3, "false due to a NULL name"); + debugs(28, 3, "no; caller did not supply an ACL name"); return false; } - debugs(28, 5, "aclIsProxyAuth: called for " << name); - - if (const auto *a = Acl::Node::FindByName(name)) { - debugs(28, 5, "aclIsProxyAuth: returning " << a->isProxyAuth()); + if (const auto a = Acl::Node::FindByName(*name)) { + debugs(28, 5, "returning " << a->isProxyAuth() << " for ACL " << *name); return a->isProxyAuth(); } - debugs(28, 3, "aclIsProxyAuth: WARNING, called for nonexistent ACL"); + debugs(28, 3, "WARNING: Called for nonexistent ACL " << *name); return false; } @@ -153,13 +148,9 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) } const int ruleId = ((treep && *treep) ? (*treep)->childrenCount() : 0) + 1; - MemBuf ctxBuf; - ctxBuf.init(); - ctxBuf.appendf("%s#%d", directive, ruleId); - ctxBuf.terminate(); Acl::AndNode *rule = new Acl::AndNode; - rule->context(ctxBuf.content(), config_input_line); + rule->context(ToSBuf(directive, '#', ruleId), config_input_line); rule->lineParse(); if (rule->empty()) { debugs(28, DBG_CRITICAL, "aclParseAccessLine: " << cfg_filename << " line " << config_lineno << ": " << config_input_line); @@ -173,7 +164,7 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep) assert(treep); if (!*treep) { *treep = new Acl::Tree; - (*treep)->context(directive, config_input_line); + (*treep)->context(SBuf(directive), config_input_line); } (*treep)->add(rule, action); @@ -189,24 +180,14 @@ aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label) if (!label) label = "..."; - MemBuf ctxLine; - ctxLine.init(); - ctxLine.appendf("(%s %s line)", cfg_directive, label); - ctxLine.terminate(); - Acl::AndNode *rule = new Acl::AndNode; - rule->context(ctxLine.content(), config_input_line); + rule->context(ToSBuf('(', cfg_directive, ' ', label, " line)"), config_input_line); const auto aclCount = rule->lineParse(); - MemBuf ctxTree; - ctxTree.init(); - ctxTree.appendf("%s %s", cfg_directive, label); - ctxTree.terminate(); - // We want a cbdata-protected Tree (despite giving it only one child node). Acl::Tree *tree = new Acl::Tree; tree->add(rule); - tree->context(ctxTree.content(), config_input_line); + tree->context(ToSBuf(cfg_directive, ' ', label), config_input_line); assert(treep); assert(!*treep); diff --git a/src/acl/Gadgets.h b/src/acl/Gadgets.h index 0f41899e5b..1e09be11b1 100644 --- a/src/acl/Gadgets.h +++ b/src/acl/Gadgets.h @@ -11,7 +11,9 @@ #include "acl/forward.h" #include "error/forward.h" +#include "sbuf/forward.h" +#include #include class ConfigParser; @@ -45,10 +47,14 @@ aclParseAclList(ConfigParser &parser, Acl::Tree **tree, const Any any) return aclParseAclList(parser, tree, buf.str().c_str()); } -/// \ingroup ACLAPI -int aclIsProxyAuth(const char *name); -/// \ingroup ACLAPI -err_type aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allowed); +/// Whether the given name names an Acl::Node object with true isProxyAuth() result. +/// This is a safe variation of Acl::Node::FindByName(*name)->isProxyAuth(). +bool aclIsProxyAuth(const std::optional &name); + +/// The first configured deny_info error page ID matching the given access check outcome (or ERR_NONE). +/// \param allowCustomStatus whether to consider deny_info rules containing custom HTTP response status code +err_type FindDenyInfoPage(const Acl::Answer &, bool allowCustomStatus); + /// \ingroup ACLAPI void aclParseDenyInfoLine(AclDenyInfoList **); /// \ingroup ACLAPI diff --git a/src/acl/InnerNode.cc b/src/acl/InnerNode.cc index 585c9e6247..761b56814c 100644 --- a/src/acl/InnerNode.cc +++ b/src/acl/InnerNode.cc @@ -81,8 +81,8 @@ SBufList Acl::InnerNode::dump() const { SBufList rv; - for (Nodes::const_iterator i = nodes.begin(); i != nodes.end(); ++i) - rv.push_back(SBuf((*i)->name)); + for (const auto &node: nodes) + rv.push_back(node->name); return rv; } diff --git a/src/acl/Node.h b/src/acl/Node.h index 747f76a5cf..cd264f5224 100644 --- a/src/acl/Node.h +++ b/src/acl/Node.h @@ -12,6 +12,7 @@ #include "acl/forward.h" #include "acl/Options.h" #include "dlink.h" +#include "sbuf/SBuf.h" class ConfigParser; @@ -30,6 +31,11 @@ public: static void ParseAclLine(ConfigParser &parser, Acl::Node **head); static void Initialize(); + + /// A configured ACL with a given name or nil. + static Acl::Node *FindByName(const SBuf &); + /// \copydoc FindByName() + /// \deprecated Use to avoid performance regressions; remove with the last caller. static Acl::Node *FindByName(const char *name); Node(); @@ -37,7 +43,7 @@ public: virtual ~Node(); /// sets user-specified ACL name and squid.conf context - void context(const char *name, const char *configuration); + void context(const SBuf &aName, const char *configuration); /// Orchestrates matching checklist against the Acl::Node using match(), /// after checking preconditions and while providing debugging. @@ -67,7 +73,12 @@ public: /// printed parameters are collected from all same-name "acl" directives. void dumpWhole(const char *directiveName, std::ostream &); - char name[ACL_NAME_SZ]; + /// Either aclname parameter from the explicitly configured acl directive or + /// a label generated for an internal ACL tree node. All Node objects + /// corresponding to one Squid configuration have unique names. + /// See also: context() and FindByName(). + SBuf name; + char *cfgline; Acl::Node *next; // XXX: remove or at least use refcounting bool registered; ///< added to the global list of ACLs via aclRegister() @@ -91,7 +102,7 @@ private: /// \see Acl::Node::options() virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); } - static void ParseNamed(ConfigParser &, Node **head, const char *name); + static void ParseNamed(ConfigParser &, Node **head, const SBuf &name); }; } // namespace Acl diff --git a/src/acl/forward.h b/src/acl/forward.h index 2dcbd9e7a9..d11747ff72 100644 --- a/src/acl/forward.h +++ b/src/acl/forward.h @@ -38,8 +38,6 @@ void Init(void); typedef void ACLCB(Acl::Answer, void *); -#define ACL_NAME_SZ 64 - // TODO: Consider renaming all users and removing. Cons: hides the difference // between ACLList tree without actions and acl_access Tree with actions. #define acl_access Acl::Tree diff --git a/src/adaptation/Answer.cc b/src/adaptation/Answer.cc index 8ca8c9d14a..0f5c8c567f 100644 --- a/src/adaptation/Answer.cc +++ b/src/adaptation/Answer.cc @@ -32,7 +32,7 @@ Adaptation::Answer::Forward(Http::Message *aMsg) } Adaptation::Answer -Adaptation::Answer::Block(const String &aRule) +Adaptation::Answer::Block(const SBuf &aRule) { Answer answer(akBlock); answer.ruleId = aRule; @@ -40,6 +40,15 @@ Adaptation::Answer::Block(const String &aRule) return answer; } +Acl::Answer +Adaptation::Answer::blockedToChecklistAnswer() const +{ + assert(kind == akBlock); + Acl::Answer answer(ACCESS_DENIED); + answer.lastCheckedName = ruleId; + return answer; +} + std::ostream & Adaptation::Answer::print(std::ostream &os) const { diff --git a/src/adaptation/Answer.h b/src/adaptation/Answer.h index 2fcb51459e..5b06c46bfd 100644 --- a/src/adaptation/Answer.h +++ b/src/adaptation/Answer.h @@ -9,11 +9,13 @@ #ifndef SQUID_SRC_ADAPTATION_ANSWER_H #define SQUID_SRC_ADAPTATION_ANSWER_H +#include "acl/Acl.h" #include "adaptation/forward.h" #include "http/forward.h" -#include "SquidString.h" +#include "sbuf/SBuf.h" #include +#include namespace Adaptation { @@ -31,13 +33,16 @@ public: static Answer Error(bool final); ///< create an akError answer static Answer Forward(Http::Message *aMsg); ///< create an akForward answer - static Answer Block(const String &aRule); ///< create an akBlock answer + static Answer Block(const SBuf &aRule); ///< create an akBlock answer + + /// creates an Acl::Answer from akBlock answer + Acl::Answer blockedToChecklistAnswer() const; std::ostream &print(std::ostream &os) const; public: Http::MessagePointer message; ///< HTTP request or response to forward - String ruleId; ///< ACL (or similar rule) name that blocked forwarding + std::optional ruleId; ///< ACL (or similar rule) name that blocked forwarding bool final; ///< whether the error, if any, cannot be bypassed Kind kind; ///< the type of the answer diff --git a/src/adaptation/ecap/XactionRep.cc b/src/adaptation/ecap/XactionRep.cc index c167cd6acf..b7cfe0d845 100644 --- a/src/adaptation/ecap/XactionRep.cc +++ b/src/adaptation/ecap/XactionRep.cc @@ -18,6 +18,7 @@ #include "format/Format.h" #include "HttpReply.h" #include "MasterXaction.h" +#include "sbuf/StringConvert.h" #if HAVE_LIBECAP_COMMON_AREA_H #include @@ -456,7 +457,7 @@ Adaptation::Ecap::XactionRep::blockVirgin() sinkVb("blockVirgin"); updateHistory(nullptr); - sendAnswer(Answer::Block(service().cfg().key)); + sendAnswer(Answer::Block(StringToSBuf(service().cfg().key))); Must(done()); } diff --git a/src/cache_cf.cc b/src/cache_cf.cc index f8f8757ccc..fafb9867b0 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -2029,15 +2029,12 @@ static void ParseAclWithAction(acl_access **access, const Acl::Answer &action, const char *desc, Acl::Node *acl) { assert(access); - SBuf name; if (!*access) { *access = new Acl::Tree; - name.Printf("(%s rules)", desc); - (*access)->context(name.c_str(), config_input_line); + (*access)->context(ToSBuf('(', desc, " rules)"), config_input_line); } Acl::AndNode *rule = new Acl::AndNode; - name.Printf("(%s rule)", desc); - rule->context(name.c_str(), config_input_line); + rule->context(ToSBuf('(', desc, " rule)"), config_input_line); if (acl) rule->add(acl); else @@ -4726,7 +4723,8 @@ static void parse_ftp_epsv(acl_access **ftp_epsv) *ftp_epsv = nullptr; if (ftpEpsvDeprecatedAction == Acl::Answer(ACCESS_DENIED)) { - if (auto *a = Acl::Node::FindByName("all")) + static const auto all = new SBuf("all"); + if (const auto a = Acl::Node::FindByName(*all)) ParseAclWithAction(ftp_epsv, ftpEpsvDeprecatedAction, "ftp_epsv", a); else { self_destruct(); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index ef9e05a0f5..5803bbadeb 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1855,12 +1855,11 @@ clientReplyContext::processReplyAccessResult(const Acl::Answer &accessAllowed) { debugs(88, 2, "The reply for " << http->request->method << ' ' << http->uri << " is " << accessAllowed << ", because it matched " - << (AclMatchedName ? AclMatchedName : "NO ACL's")); + << accessAllowed.lastCheckDescription()); if (!accessAllowed.allowed()) { ErrorState *err; - err_type page_id; - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, 1); + auto page_id = FindDenyInfoPage(accessAllowed, true); http->updateLoggingTags(LOG_TCP_DENIED_REPLY); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 9ed54f2c72..2133db3790 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -713,11 +713,10 @@ void ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) { acl_checklist = nullptr; - err_type page_id; Http::StatusCode status; debugs(85, 2, "The request " << http->request->method << ' ' << http->uri << " is " << answer << - "; last ACL checked: " << (AclMatchedName ? AclMatchedName : "[none]")); + "; last ACL checked: " << answer.lastCheckDescription()); #if USE_AUTH char const *proxy_auth_msg = ""; @@ -732,21 +731,14 @@ ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) /* Send an auth challenge or error */ // XXX: do we still need aclIsProxyAuth() ? - bool auth_challenge = (answer == ACCESS_AUTH_REQUIRED || aclIsProxyAuth(AclMatchedName)); + const auto auth_challenge = (answer == ACCESS_AUTH_REQUIRED || aclIsProxyAuth(answer.lastCheckedName)); debugs(85, 5, "Access Denied: " << http->uri); - debugs(85, 5, "AclMatchedName = " << (AclMatchedName ? AclMatchedName : "")); #if USE_AUTH if (auth_challenge) debugs(33, 5, "Proxy Auth Message = " << (proxy_auth_msg ? proxy_auth_msg : "")); #endif - /* - * NOTE: get page_id here, based on AclMatchedName because if - * USE_DELAY_POOLS is enabled, then AclMatchedName gets clobbered in - * the clientCreateStoreEntry() call just below. Pedro Ribeiro - * - */ - page_id = aclGetDenyInfoPage(&Config.denyInfoList, AclMatchedName, answer != ACCESS_AUTH_REQUIRED); + auto page_id = FindDenyInfoPage(answer, answer != ACCESS_AUTH_REQUIRED); http->updateLoggingTags(LOG_TCP_DENIED); @@ -2050,10 +2042,8 @@ ClientHttpRequest::handleAdaptationBlock(const Adaptation::Answer &answer) { static const auto d = MakeNamedErrorDetail("REQMOD_BLOCK"); request->detailError(ERR_ACCESS_DENIED, d); - AclMatchedName = answer.ruleId.termedBuf(); assert(calloutContext); - calloutContext->clientAccessCheckDone(ACCESS_DENIED); - AclMatchedName = nullptr; + calloutContext->clientAccessCheckDone(answer.blockedToChecklistAnswer()); } void diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 60ab7d0878..5d62fa0c67 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -923,7 +923,9 @@ Client::handledEarlyAdaptationAbort() void Client::handleAdaptationBlocked(const Adaptation::Answer &answer) { - debugs(11,5, answer.ruleId); + const auto blockedAnswer = answer.blockedToChecklistAnswer(); + + debugs(11,5, blockedAnswer.lastCheckDescription()); if (abortOnBadEntry("entry went bad while ICAP aborted")) return; @@ -939,8 +941,7 @@ Client::handleAdaptationBlocked(const Adaptation::Answer &answer) debugs(11,7, "creating adaptation block response"); - err_type page_id = - aclGetDenyInfoPage(&Config.denyInfoList, answer.ruleId.termedBuf(), 1); + auto page_id = FindDenyInfoPage(blockedAnswer, true); if (page_id == ERR_NONE) page_id = ERR_ACCESS_DENIED; diff --git a/src/external_acl.cc b/src/external_acl.cc index a64155baf4..84ddc3e549 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -486,11 +486,11 @@ class external_acl_data CBDATA_CLASS(external_acl_data); public: - explicit external_acl_data(external_acl *aDef) : def(cbdataReference(aDef)), name(nullptr), arguments(nullptr) {} + explicit external_acl_data(external_acl * const aDef): def(cbdataReference(aDef)), arguments(nullptr) {} ~external_acl_data(); external_acl *def; - const char *name; + SBuf name; wordlist *arguments; }; @@ -498,7 +498,6 @@ CBDATA_CLASS_INIT(external_acl_data); external_acl_data::~external_acl_data() { - xfree(name); wordlistDestroy(&arguments); cbdataReferenceDone(def); } @@ -528,7 +527,7 @@ ACLExternal::parse() // def->name is the name of the external_acl_type. // this is the name of the 'acl' directive being tested - data->name = xstrdup(name); + data->name = name; while ((token = ConfigParser::strtokFile())) { wordlistAdd(&data->arguments, token); @@ -768,8 +767,7 @@ ACLExternal::makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl if (t->type == Format::LFT_EXT_ACL_NAME) { // setup for %ACL - safe_free(ch->al->lastAclName); - ch->al->lastAclName = xstrdup(acl_data->name); + ch->al->lastAclName = acl_data->name; } if (t->type == Format::LFT_EXT_ACL_DATA) { diff --git a/src/format/Format.cc b/src/format/Format.cc index abd919c5aa..cdce6a75b4 100644 --- a/src/format/Format.cc +++ b/src/format/Format.cc @@ -1438,7 +1438,8 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS break; case LFT_EXT_ACL_NAME: - out = al->lastAclName; + if (!al->lastAclName.isEmpty()) + out = al->lastAclName.c_str(); break; case LFT_EXT_ACL_DATA: diff --git a/src/icp_v2.cc b/src/icp_v2.cc index 7c0f571aea..6171358fa1 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -427,8 +427,6 @@ icpCreateAndSend(icp_opcode opcode, int flags, char const *url, int reqnum, int void icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) { - debugs(12, 2, "icpDenyAccess: Access Denied for " << from << " by " << AclMatchedName << "."); - if (clientdbCutoffDenied(from)) { /* * count this DENIED query in the clientdb, even though @@ -443,14 +441,20 @@ icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd) bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request) { - /* absent any explicit rules, we deny all */ - if (!Config.accessList.icp) + if (!Config.accessList.icp) { + debugs(12, 2, "Access Denied due to lack of ICP access rules."); return false; + } ACLFilledChecklist checklist(Config.accessList.icp, icp_request, nullptr); checklist.src_addr = from; checklist.my_addr.setNoAddr(); - return checklist.fastCheck().allowed(); + const auto &answer = checklist.fastCheck(); + if (answer.allowed()) + return true; + + debugs(12, 2, "Access Denied for " << from << " by " << answer.lastCheckDescription() << "."); + return false; } HttpRequest * diff --git a/src/tests/stub_acl.cc b/src/tests/stub_acl.cc index 1583504c27..29c1027bc6 100644 --- a/src/tests/stub_acl.cc +++ b/src/tests/stub_acl.cc @@ -13,8 +13,7 @@ #define STUB_API "acl/" #include "tests/STUB.h" -#include "acl/Acl.h" -const char *AclMatchedName = nullptr; +#include "acl/forward.h" #include "acl/Gadgets.h" size_t aclParseAclList(ConfigParser &, Acl::Tree **, const char *) STUB_RETVAL(0)