Protect ACLFilledChecklist heap allocations from leaking #1870

eduard-bagdasaryan · 2024-07-20T16:39:16Z

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.

The common approach when dealing with ACLFilledChecklist non-blocking checks consists of three steps: 1. create an ACLFilledChecklist object on heap 2. configure the object 3. invoke ACLFilledChecklist::nonBlockingCheck() Memory leak occurs if an exception is thrown between (1) and (3). To avoid leaks, whe should rely on RAII mechanism instead of raw pointers.

After introducing ACLFilledChecklist::Make() API the calling code got unique_ptr<> instead of ACLFilledChecklist*, and its semantics remained the same. However, since nonBlockingCheck() still expects a raw pointer (which it takes ownership of), the unique_ptr<> should be released before the call. The new nonBlockingCheck() now encapsulates this requirement.

Also polished new macro description.

There is no difference for the CBDATA_CLASS_WITH_MAKE()-calling class. However, if some future code (incorrectly) creates a child of that class, then we want build to fail. Creating a child requires using something like CBDATA_INTERMEDIATE()/CBDATA_CHILD() for reasons documented in CBDATA_INTERMEDIATE() description. We cannot _guarantee_ that such a build would fail, but making new() private will help in cases where child code calls new(). Besides, tighter restrictions should raise fewer questions, and it is always easier to relax a restriction than to add one.

This change arguably makes this macro-based code slightly easier to grok and clarifies CBDATA_CLASS_WITH_MAKE() intent.

The pointer returned by ACLFilledChecklist::Make() cannot be used for those purposes (because there can be only one checklist/cbdata owner) and, to prevent memory leaks, a different kind of pointer should be used during object configuration.

Also updated stale references to aclCheckFast() and aclNBCheck().

rousskov

Thank you.

rousskov · 2024-07-23T15:16:52Z

src/client_side.cc

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


This explicit std::move() call (and similar calls added in this PR) is one of those exceptional cases where forcing developer to write (and code reader to see) this explicit std::move call is desirable: We want to emphasize that acl_checklist may be gone after this NonBlockingCheck() call returns.

BTW, GCC and clang do not do that for us automatically, but we plan to enhance our CI tests to automatically check Squid code for use-after-move errors.

This comment does not request any PR changes.

rousskov · 2024-07-23T15:21:00Z

src/client_side.cc

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


If others prefer, we can simplify this to remove ACLFilledChecklist prefix by making NonBlockingCheck() into a global function:

Suggested change

ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

or

Suggested change

ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

Acl::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

Due to fairly unique parameter types, such a global function is unlikely to clash with other non-blocking check functions. Please add a comment if you want to see this kind of change (at the cost of adding a corresponding friend declaration to checklist class type).

This comment, byt itself, does not request any PR changes.

rousskov · 2024-07-23T15:25:06Z

src/acl/FilledChecklist.h

+    /// 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>;


Another possible type name that still addresses the documented "this is not our regular Pointer" concern is UniquePointer:

Suggested change

using MakingPointer = std::unique_ptr<ACLFilledChecklist>;

using UniquePointer = std::unique_ptr<ACLFilledChecklist>;

I prefer MakingPointer because it is more specific to this smart pointer primary purpose, but I acknowledge that "making pointer" is ambiguous.

This comment does not request any PR changes.

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.

…#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.

eduard-bagdasaryan and others added 13 commits July 4, 2024 17:54

Prohibit ACLFilledChecklist heap allocations bypassing the Make(...) API

ff6fb73

Temporarily applied SQUID-997-remove-peerselector-checklist changes

769e66b

Merge from master

ed12451

Documented a method

08aa0ee

Refactored CBDATA_CLASS_2() into simpler CBDATA_CLASS_WITH_MAKE()

f8d024e

Also polished new macro description.

fixup: Move newSpecifier into CBDATA_DECL_() callers

faead84

This change arguably makes this macro-based code slightly easier to grok and clarifies CBDATA_CLASS_WITH_MAKE() intent.

fixup: Minor comment polishing

f95d411

Do not leak checklist if NonBlockingCheck() throws

664008b

fixup: Found one more stale operator new() reference

43419b9

Also updated stale references to aclCheckFast() and aclNBCheck().

rousskov approved these changes Jul 23, 2024

View reviewed changes

rousskov mentioned this pull request Jul 23, 2024

Remove Ident protocol support #1827

Closed

Merged from master

948fb09

rousskov added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Aug 9, 2024

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Aug 9, 2024

squid-anubis closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect ACLFilledChecklist heap allocations from leaking #1870

Protect ACLFilledChecklist heap allocations from leaking #1870

eduard-bagdasaryan commented Jul 20, 2024

rousskov left a comment

rousskov Jul 23, 2024

rousskov Jul 23, 2024

rousskov Jul 23, 2024

	ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);
	NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

	ACLFilledChecklist::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);
	Acl::NonBlockingCheck(std::move(acl_checklist), httpsSslBumpStep2AccessCheckDone, this);

	using MakingPointer = std::unique_ptr<ACLFilledChecklist>;
	using UniquePointer = std::unique_ptr<ACLFilledChecklist>;

Protect ACLFilledChecklist heap allocations from leaking #1870

Protect ACLFilledChecklist heap allocations from leaking #1870

Conversation

eduard-bagdasaryan commented Jul 20, 2024

rousskov left a comment

Choose a reason for hiding this comment

rousskov Jul 23, 2024

Choose a reason for hiding this comment

rousskov Jul 23, 2024

Choose a reason for hiding this comment

rousskov Jul 23, 2024

Choose a reason for hiding this comment