Fix ARAL double-free race - #22294
Merged
Merged
Conversation
aral_freez_internal() used a non-atomic load+store on the trailer. Two concurrent double-frees of the same pointer could both see the same page and enqueue the slot onto the free list twice, leading to later trailer corruption and crashes (e.g. in aral_unmark_allocation following a stale page pointer). Replace with __atomic_exchange_n(.., 0, ACQ_REL); the loser sees NULL and fatal()s with a clear diagnostic before touching the free list. aral_unmark_allocation() has the same load-then-modify shape but is not exercised concurrently by any current caller; left for follow-up.
Contributor
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant T1 as Thread 1 (First Free)
participant T2 as Thread 2 (Concurrent Free)
participant Slot as Memory Slot Trailer
participant Stats as ARAL Global Stats
participant FL as Free List
Note over T1, T2: Concurrent aral_freez_internal(ptr) calls
T1->>Slot: NEW: aral_claim_page_pointer_after_element()
Note right of Slot: Atomic Exchange (swap with 0)
Slot-->>T1: Returns valid ARAL_PAGE pointer
T2->>Slot: NEW: aral_claim_page_pointer_after_element()
Note right of Slot: Atomic Exchange (swap with 0)
Slot-->>T2: Returns NULL (Already claimed)
rect rgb(200, 255, 200)
Note over T1, FL: Winner Path
T1->>Stats: Increment deallocators count
T1->>FL: Enqueue slot to free list
end
rect rgb(255, 200, 200)
Note over T2: Loser Path (Double Free)
alt page is NULL
T2->>T2: fatal() "ARAL: double free or stale free"
end
end
stelfrag
marked this pull request as ready for review
April 27, 2026 13:53
stelfrag
marked this pull request as draft
April 27, 2026 13:53
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Have aral_claim_page_pointer_after_element_*() return NULL directly when the atomic exchange yields 0, so the freez path's explicit fatal() runs in both debug and release builds (was previously preempted by aral_decode_*'s internal_fatal in NETDATA_INTERNAL_CHECKS builds). The load path keeps its debug-only NULL assertion unchanged. Reword the fatal to also cover memory corruption / invalid pointers that happen to have 0 in the trailer.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stelfrag
marked this pull request as ready for review
April 27, 2026 14:46
thiagoftsm
approved these changes
Apr 27, 2026
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
PR is working as expected and no issue was found during runtime. LGTM!
Merged
Ferroin
pushed a commit
that referenced
this pull request
Jul 15, 2026
* ARAL: close double-free race by atomically claiming the slot trailer aral_freez_internal() used a non-atomic load+store on the trailer. Two concurrent double-frees of the same pointer could both see the same page and enqueue the slot onto the free list twice, leading to later trailer corruption and crashes (e.g. in aral_unmark_allocation following a stale page pointer). Replace with __atomic_exchange_n(.., 0, ACQ_REL); the loser sees NULL and fatal()s with a clear diagnostic before touching the free list. aral_unmark_allocation() has the same load-then-modify shape but is not exercised concurrently by any current caller; left for follow-up. * ARAL: short-circuit claim on NULL trailer; broaden free fatal message Have aral_claim_page_pointer_after_element_*() return NULL directly when the atomic exchange yields 0, so the freez path's explicit fatal() runs in both debug and release builds (was previously preempted by aral_decode_*'s internal_fatal in NETDATA_INTERNAL_CHECKS builds). The load path keeps its debug-only NULL assertion unchanged. Reword the fatal to also cover memory corruption / invalid pointers that happen to have 0 in the trailer. (cherry picked from commit 82fcb4f)
This file contains hidden or 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
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.
Summary
aral_freez_internal() used a non-atomic load+store on the trailer. Two concurrent double-frees of the same pointer could both see the same page and enqueue the slot onto the free list twice, leading to later trailer corruption and crashes (e.g. in aral_unmark_allocation following a stale page pointer).
Replace with __atomic_exchange_n(.., 0, ACQ_REL); the loser sees NULL and fatal()s with a clear diagnostic before touching the free list.
aral_unmark_allocation() has the same load-then-modify shape but is not exercised concurrently by any current caller; left for follow-up.
Summary by cubic
Fixes a double-free race in ARAL by atomically claiming the slot trailer during free. Only one thread can re-enqueue; stale/double frees now fatal consistently (debug and release) with a clearer message that also covers corrupted pointers.
Written for commit d580d34. Summary will update on new commits. Review in cubic