Protect ACLFilledChecklist heap allocations from leaking #257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
rousskov
requested changes
Jul 4, 2024
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.
rousskov
changed the base branch from
master
to
SQUID-993-testHeaders-same-basename
rousskov
changed the base branch from
SQUID-993-testHeaders-same-basename
to
master
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
approved these changes
Jul 18, 2024
|
@eduard-bagdasaryan, FYI: Our repository had no branch protections, so all tests were essentially optional (AFAICT). To test my working theory better, I now enabled branch protections and marked two Ubuntu tests as "required". The outcome is still the same -- GitHub Actions cancel all test jobs, including required ones, when an optional test fails: |
…id-cache#1869) These wrappers were left in recent commit f036532 to reduce noise. Now we always call finishAndDeleteFetch() directly (instead of sometimes going through those wrappers). The new function name helps reduce the number of use-after-free bugs in this code.
|
This became official PR 1870. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Non-blocking ACL checks follow this code pattern:
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.