Protect ACLFilledChecklist heap allocations from leaking (squid-cache…

…#1870)

Non-blocking ACL checks follow this code pattern:

    const auto ch = new ACLFilledChecklist(...);
    fillWithInformation(ch); // may throw
    ch->nonBlockingCheck(&throwingCallback); // may delete ch or throw!
    // ch may be a dangling raw pointer here

The checklist object is leaked if an exception is thrown by
fillWithInformation() (and the code it calls) or by nonBlockingCheck()
(and the code it calls, including, in some cases, throwingCallback()).
Use std::unique_ptr to avoid such leaks.

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 *);
 

src/peer_select.cc

@@ -613,18 +613,18 @@ PeerSelector::selectMore()
         if (always_direct == ACCESS_DUNNO) {
             debugs(44, 3, "direct = " << DirectStr[direct] << " (always_direct to be checked)");
             /** check always_direct; */
-            const auto ch = new ACLFilledChecklist(Config.accessList.AlwaysDirect, request);
+            auto ch = ACLFilledChecklist::Make(Config.accessList.AlwaysDirect, request);
             ch->al = al;
             ch->syncAle(request, nullptr);
-            ch->nonBlockingCheck(CheckAlwaysDirectDone, this);
+            ACLFilledChecklist::NonBlockingCheck(std::move(ch), CheckAlwaysDirectDone, this);
             return;
         } else if (never_direct == ACCESS_DUNNO) {
             debugs(44, 3, "direct = " << DirectStr[direct] << " (never_direct to be checked)");
             /** check never_direct; */
-            const auto ch = new ACLFilledChecklist(Config.accessList.NeverDirect, request);
+            auto ch = ACLFilledChecklist::Make(Config.accessList.NeverDirect, request);
             ch->al = al;
             ch->syncAle(request, nullptr);
-            ch->nonBlockingCheck(CheckNeverDirectDone, this);
+            ACLFilledChecklist::NonBlockingCheck(std::move(ch), CheckNeverDirectDone, this);
             return;
         } else if (request->flags.noDirect) {
             /** if we are accelerating, direct is not an option. */

src/security/PeerConnector.cc

@@ -169,9 +169,9 @@ Security::PeerConnector::initialize(Security::SessionPointer &serverSession)
         // TODO: Remove ACLFilledChecklist::sslErrors and other pre-computed
         // state in favor of the ACLs accessing current/fresh info directly.
         if (acl_access *acl = ::Config.ssl_client.cert_error) {
-            const auto check = new ACLFilledChecklist(acl, request.getRaw());
+            auto check = ACLFilledChecklist::Make(acl, request.getRaw());
             fillChecklist(*check);
-            SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check);
+            SSL_set_ex_data(serverSession.get(), ssl_ex_index_cert_error_check, check.release());
         }
     }