Protect ACLFilledChecklist heap allocations from leaking

src/acl/Checklist.h

@@ -39,28 +39,6 @@ class ACLChecklist
     ACLChecklist();
     virtual ~ACLChecklist();
 
-    /**
-     * Start a non-blocking (async) check for a list of allow/deny rules.
-     * Each rule comes with a list of ACLs.
-     *
-     * The callback specified will be called with the result of the check.
-     *
-     * The first rule where all ACLs match wins. If there is such a rule,
-     * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED).
-     *
-     * If there are rules but all ACL lists mismatch, an implicit rule is used.
-     * Its result is the negation of the keyword of the last seen rule.
-     *
-     * Some ACLs may stop the check prematurely by setting an exceptional
-     * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a
-     * match or mismatch.
-     *
-     * If there are no rules to check at all, the result becomes ACCESS_DUNNO.
-     * Calling this method with no rules to check wastes a lot of CPU cycles
-     * and will result in a DBG_CRITICAL debugging message.
-     */
-    void nonBlockingCheck(ACLCB * callback, void *callback_data);
-
     /**
      * Perform a blocking (immediate) check for a list of allow/deny rules.
      * Each rule comes with a list of ACLs.
@@ -149,6 +127,29 @@ class ACLChecklist
     /// remember the name of the last ACL being evaluated
     void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; }
 
+protected:
+    /**
+     * Start a non-blocking (async) check for a list of allow/deny rules.
+     * Each rule comes with a list of ACLs.
+     *
+     * The callback specified will be called with the result of the check.
+     *
+     * The first rule where all ACLs match wins. If there is such a rule,
+     * the result becomes that rule keyword (ACCESS_ALLOWED or ACCESS_DENIED).
+     *
+     * If there are rules but all ACL lists mismatch, an implicit rule is used.
+     * Its result is the negation of the keyword of the last seen rule.
+     *
+     * Some ACLs may stop the check prematurely by setting an exceptional
+     * check result (e.g., ACCESS_AUTH_REQUIRED) instead of declaring a
+     * match or mismatch.
+     *
+     * If there are no rules to check at all, the result becomes ACCESS_DUNNO.
+     * Calling this method with no rules to check wastes a lot of CPU cycles
+     * and will result in a DBG_CRITICAL debugging message.
+     */
+    void nonBlockingCheck(ACLCB * callback, void *callback_data);
+
 private:
     /// Calls non-blocking check callback with the answer and destroys self.
     /// If abortReason is provided, sets the final answer to ACCESS_DUNNO.

src/acl/FilledChecklist.cc

@@ -186,16 +186,17 @@ ACLFilledChecklist::markSourceDomainChecked()
 /*
  * There are two common ACLFilledChecklist lifecycles paths:
  *
- * A) Using aclCheckFast(): The caller creates an ACLFilledChecklist object
- *    on stack and calls aclCheckFast().
+ * "Fast" (always synchronous or "blocking"): The user constructs an
+ *    ACLFilledChecklist object on stack, configures it as needed, and calls one
+ *    or both of its fastCheck() methods.
  *
- * B) Using aclNBCheck() and callbacks: The caller allocates an
- *    ACLFilledChecklist object (via operator new) and passes it to
- *    aclNBCheck(). Control eventually passes to ACLChecklist::checkCallback(),
- *    which will invoke the callback function as requested by the
- *    original caller of aclNBCheck().  This callback function must
- *    *not* delete the list.  After the callback function returns,
- *    checkCallback() will delete the list (i.e., self).
+ * "Slow" (usually asynchronous or "non-blocking"): The user allocates an
+ *    ACLFilledChecklist object on heap (via Make()), configures it as needed,
+ *    and passes it to NonBlockingCheck() while specifying the callback function
+ *    to call with check results. NonBlockingCheck() calls the callback function
+ *    (if the corresponding cbdata is still valid), either immediately/directly
+ *    (XXX) or eventually/asynchronously. After this callback obligations are
+ *    fulfilled, checkCallback() deletes the checklist object (i.e. "this").
  */
 ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_request):
     dst_rdns(nullptr),

src/acl/FilledChecklist.h

@@ -32,13 +32,27 @@ class ConnStateData;
  */
 class ACLFilledChecklist: public ACLChecklist
 {
-    CBDATA_CLASS(ACLFilledChecklist);
+    CBDATA_CLASS_WITH_MAKE(ACLFilledChecklist);
 
 public:
+    /// Unlike regular Foo::Pointer types, this smart pointer is meant for use
+    /// during checklist configuration only, when it provides exception safety.
+    /// Any other/long-term checklist storage requires CbcPointer or equivalent.
+    using MakingPointer = std::unique_ptr<ACLFilledChecklist>;
+
     ACLFilledChecklist();
     ACLFilledChecklist(const acl_access *, HttpRequest *);
     ~ACLFilledChecklist() override;
 
+    /// Creates an ACLFilledChecklist object with given constructor arguments.
+    /// Callers are expected to eventually proceed with NonBlockingCheck().
+    static MakingPointer Make(const acl_access *a, HttpRequest *r) { return MakingPointer(new ACLFilledChecklist(a, r)); }
+
+    /// \copydoc ACLChecklist::nonBlockingCheck()
+    /// This public nonBlockingCheck() wrapper should be paired with Make(). The
+    /// pair prevents exception-caused Checklist memory leaks in caller code.
+    static void NonBlockingCheck(MakingPointer &&p, ACLCB *cb, void *data) { p->nonBlockingCheck(cb, data); (void)p.release(); }
+
     /// configure client request-related fields for the first time
     void setRequest(HttpRequest *);
 

src/adaptation/AccessCheck.cc

@@ -128,11 +128,11 @@ Adaptation::AccessCheck::checkCandidates()
     while (!candidates.empty()) {
         if (AccessRule *r = FindRule(topCandidate())) {
             /* BUG 2526: what to do when r->acl is empty?? */
-            const auto acl_checklist = new ACLFilledChecklist(r->acl, filter.request);
+            auto acl_checklist = ACLFilledChecklist::Make(r->acl, filter.request);
             acl_checklist->updateAle(filter.al);
             acl_checklist->updateReply(filter.reply);
             acl_checklist->syncAle(filter.request, nullptr);
-            acl_checklist->nonBlockingCheck(AccessCheckCallbackWrapper, this);
+            ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), AccessCheckCallbackWrapper, this);
             return;
         }
 

src/cbdata.h

@@ -255,12 +255,12 @@ cbdata_type cbdataInternalAddType(cbdata_type type, const char *label, int size)
 
 /// declaration-generator used internally by CBDATA_CLASS() and CBDATA_CHILD()
 #define CBDATA_DECL_(type, methodSpecifiers) \
-    public: \
         void *operator new(size_t size) { \
           assert(size == sizeof(type)); \
           if (!CBDATA_##type) CBDATA_##type = cbdataInternalAddType(CBDATA_##type, #type, sizeof(type)); \
           return (type *)cbdataInternalAlloc(CBDATA_##type); \
         } \
+    public: \
         void operator delete (void *address) { \
           if (address) cbdataInternalFree(address); \
         } \
@@ -286,12 +286,17 @@ class CbdataParent
 /// cbdata-enables a stand-alone class that is not a CbdataParent child
 /// sets the class declaration section to "private"
 /// use this at the start of your class declaration for consistency sake
-#define CBDATA_CLASS(type) CBDATA_DECL_(type, noexcept)
+#define CBDATA_CLASS(type) public: CBDATA_DECL_(type, noexcept)
+
+/// A CBDATA_CLASS() variant for classes that want to prevent accidental
+/// operator new() calls by making that operator private and forcing external
+/// users to call a Make() function instead.
+#define CBDATA_CLASS_WITH_MAKE(type) private: CBDATA_DECL_(type, noexcept)
 
 /// cbdata-enables a final CbdataParent-derived class in a hierarchy
 /// sets the class declaration section to "private"
 /// use this at the start of your class declaration for consistency sake
-#define CBDATA_CHILD(type) CBDATA_DECL_(type, final) \
+#define CBDATA_CHILD(type) public: CBDATA_DECL_(type, final) \
       void finalizedInCbdataChild() final {}
 
 /// cbdata-enables a non-final CbdataParent-derived class T in a hierarchy.

src/client_side.cc

@@ -2484,7 +2484,7 @@ ConnStateData::postHttpsAccept()
         CodeContext::Reset(connectAle);
         // TODO: Use these request/ALE when waiting for new bumped transactions.
 
-        const auto acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, request);
+        auto acl_checklist = ACLFilledChecklist::Make(Config.accessList.ssl_bump, request);
         fillChecklist(*acl_checklist);
         // Build a local AccessLogEntry to allow requiresAle() acls work
         acl_checklist->al = connectAle;
@@ -2501,7 +2501,7 @@ ConnStateData::postHttpsAccept()
         ClientHttpRequest *http = context ? context->http : nullptr;
         const char *log_uri = http ? http->log_uri : nullptr;
         acl_checklist->syncAle(request, log_uri);
-        acl_checklist->nonBlockingCheck(httpsSslBumpAccessCheckDone, this);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpAccessCheckDone, this);
 #else
         fatal("FATAL: SSL-Bump requires --with-openssl");
 #endif
@@ -2967,12 +2967,12 @@ ConnStateData::startPeekAndSplice()
         sslServerBump->step = XactionStep::tlsBump2;
         // Run a accessList check to check if want to splice or continue bumping
 
-        const auto acl_checklist = new ACLFilledChecklist(Config.accessList.ssl_bump, sslServerBump->request.getRaw());
+        auto acl_checklist = ACLFilledChecklist::Make(Config.accessList.ssl_bump, sslServerBump->request.getRaw());
         acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpNone));
         acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpClientFirst));
         acl_checklist->banAction(Acl::Answer(ACCESS_ALLOWED, Ssl::bumpServerFirst));
         fillChecklist(*acl_checklist);
-        acl_checklist->nonBlockingCheck(httpsSslBumpStep2AccessCheckDone, this);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);
         return;
     }
 
@@ -3111,7 +3111,7 @@ ConnStateData::initiateTunneledRequest(HttpRequest::Pointer const &cause, const
         // TLS handshakes on non-bumping https_port. TODO: Discover these
         // problems earlier so that they can be classified/detailed better.
         debugs(33, 2, "Not able to compute URL, abort request tunneling for " << reason);
-        // TODO: throw when nonBlockingCheck() callbacks gain job protections
+        // TODO: throw when NonBlockingCheck() callbacks gain job protections
         static const auto d = MakeNamedErrorDetail("TUNNEL_TARGET");
         updateError(ERR_INVALID_REQ, d);
         return false;
@@ -3448,10 +3448,10 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
     }
 }
 
-ACLFilledChecklist *
+ACLFilledChecklist::MakingPointer
 clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http)
 {
-    const auto checklist = new ACLFilledChecklist(acl, nullptr);
+    auto checklist = ACLFilledChecklist::Make(acl, nullptr);
     clientAclChecklistFill(*checklist, http);
     return checklist;
 }

src/client_side_reply.cc

@@ -1835,10 +1835,10 @@ clientReplyContext::processReplyAccess ()
     }
 
     /** Process http_reply_access lists */
-    ACLFilledChecklist *replyChecklist =
+    auto replyChecklist =
         clientAclChecklistCreate(Config.accessList.reply, http);
     replyChecklist->updateReply(reply);
-    replyChecklist->nonBlockingCheck(ProcessReplyAccessResult, this);
+    ACLFilledChecklist::NonBlockingCheck(std::move(replyChecklist), ProcessReplyAccessResult, this);
 }
 
 void

src/client_side_request.cc

@@ -438,13 +438,13 @@ clientFollowXForwardedForCheck(Acl::Answer answer, void *data)
         if ((addr = asciiaddr)) {
             request->indirect_client_addr = addr;
             request->x_forwarded_for_iterator.cut(l);
-            const auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http);
+            auto ch = clientAclChecklistCreate(Config.accessList.followXFF, http);
             if (!Config.onoff.acl_uses_indirect_client) {
                 /* override the default src_addr tested if we have to go deeper than one level into XFF */
                 ch->src_addr = request->indirect_client_addr;
             }
             if (++calloutContext->currentXffHopNumber < SQUID_X_FORWARDED_FOR_HOP_MAX) {
-                ch->nonBlockingCheck(clientFollowXForwardedForCheck, data);
+                ACLFilledChecklist::NonBlockingCheck(std::move(ch), clientFollowXForwardedForCheck, data);
                 return;
             }
             const auto headerName = Http::HeaderLookupTable.lookup(Http::HdrType::X_FORWARDED_FOR).name;
@@ -666,15 +666,15 @@ ClientRequestContext::clientAccessCheck()
         http->request->x_forwarded_for_iterator = http->request->header.getList(Http::HdrType::X_FORWARDED_FOR);
 
         /* begin by checking to see if we trust direct client enough to walk XFF */
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http);
-        acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientFollowXForwardedForCheck, this);
         return;
     }
 #endif
 
     if (Config.accessList.http) {
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http);
-        acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.http, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientAccessCheckDoneWrapper, this);
     } else {
         debugs(0, DBG_CRITICAL, "No http_access configuration found. This will block ALL traffic");
         clientAccessCheckDone(ACCESS_DENIED);
@@ -690,8 +690,8 @@ void
 ClientRequestContext::clientAccessCheck2()
 {
     if (Config.accessList.adapted_http) {
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http);
-        acl_checklist->nonBlockingCheck(clientAccessCheckDoneWrapper, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.adapted_http, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientAccessCheckDoneWrapper, this);
     } else {
         debugs(85, 2, "No adapted_http_access configuration. default: ALLOW");
         clientAccessCheckDone(ACCESS_ALLOWED);
@@ -835,8 +835,8 @@ ClientRequestContext::clientRedirectStart()
     debugs(33, 5, "'" << http->uri << "'");
     http->al->syncNotes(http->request);
     if (Config.accessList.redirector) {
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http);
-        acl_checklist->nonBlockingCheck(clientRedirectAccessCheckDone, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.redirector, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientRedirectAccessCheckDone, this);
     } else
         redirectStart(http, clientRedirectDoneWrapper, this);
 }
@@ -871,8 +871,8 @@ ClientRequestContext::clientStoreIdStart()
     debugs(33, 5,"'" << http->uri << "'");
 
     if (Config.accessList.store_id) {
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http);
-        acl_checklist->nonBlockingCheck(clientStoreIdAccessCheckDone, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.store_id, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), clientStoreIdAccessCheckDone, this);
     } else
         storeIdStart(http, clientStoreIdDoneWrapper, this);
 }
@@ -1310,8 +1310,8 @@ void
 ClientRequestContext::checkNoCache()
 {
     if (Config.accessList.noCache) {
-        const auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http);
-        acl_checklist->nonBlockingCheck(checkNoCacheDoneWrapper, this);
+        auto acl_checklist = clientAclChecklistCreate(Config.accessList.noCache, http);
+        ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), checkNoCacheDoneWrapper, this);
     } else {
         /* unless otherwise specified, we try to cache. */
         checkNoCacheDone(ACCESS_ALLOWED);
@@ -1405,8 +1405,8 @@ ClientRequestContext::sslBumpAccessCheck()
 
     debugs(85, 5, "SslBump possible, checking ACL");
 
-    ACLFilledChecklist *aclChecklist = clientAclChecklistCreate(Config.accessList.ssl_bump, http);
-    aclChecklist->nonBlockingCheck(sslBumpAccessCheckDoneWrapper, this);
+    auto aclChecklist = clientAclChecklistCreate(Config.accessList.ssl_bump, http);
+    ACLFilledChecklist::NonBlockingCheck(std::move(aclChecklist), sslBumpAccessCheckDoneWrapper, this);
     return true;
 }
 

src/client_side_request.h

@@ -10,6 +10,7 @@
 #define SQUID_SRC_CLIENT_SIDE_REQUEST_H
 
 #include "AccessLogEntry.h"
+#include "acl/FilledChecklist.h"
 #include "client_side.h"
 #include "clientStream.h"
 #include "http/forward.h"
@@ -257,7 +258,7 @@ class ClientHttpRequest
 /* client http based routines */
 char *clientConstructTraceEcho(ClientHttpRequest *);
 
-ACLFilledChecklist *clientAclChecklistCreate(const acl_access *, ClientHttpRequest *);
+ACLFilledChecklist::MakingPointer clientAclChecklistCreate(const acl_access *, ClientHttpRequest *);
 void clientAclChecklistFill(ACLFilledChecklist &, ClientHttpRequest *);
 void clientAccessCheck(ClientHttpRequest *);