Fix aral race condition by stelfrag · Pull Request #22367 · netdata/netdata

stelfrag · 2026-05-01T16:01:44Z

Summary

Introduce a third trailer state (page | UNMARKING) that aral_unmark_allocation publishes before taking the page lock to decrement marked_elements, and clears after. While the bit is visible, aral_freez_internal observes it in its claim and spins until the final UNMARKED state is published, so:

freez never sees an unmarked trailer with a stale marked_elements counter (no spurious "marked > used" assertion under NETDATA_INTERNAL_CHECKS)
the slot's refcount contribution stays in place across the trailer transition, so the page cannot be destroyed under unmark's page_lock (closes the UAF that surfaced in production as gorilla_writer_aral_unmark SEGVs and aral_set_page_pointer dereferences of stale page pointers)
The freez hot path keeps its single atomic-exchange; the cold path (UNMARKING observed) restores the bit and retries. Bit availability is guarded with a _Static_assert against SYSTEM_REQUIRED_ALIGNMENT.

Add aral_unittest_concurrency() (8 scenarios) under NETDATA_INTERNAL_CHECKS and reachable via -W aralconcurrency:

Cases:

1-3 clean unmark / unmark-on-unmarked / clean freez (with marked guard on the same page)
4-6 forced same with no grace period - each verifies counter consistency
7 last marked on page triggers list move
8 coordinated stress: 256 pointers, deterministic UNMARKING for every slot, asserts the cold path was entered via a counter incremented in aral_claim_page_pointer_after_element___wait_for_unmark

Summary by cubic

Serialize ARAL unmark/freez with a new UNMARKING trailer state to close the race that caused counter skew and page‑lifetime UAFs. Free path stays fast; adds deterministic concurrency tests and a CLI flag to run them.

Bug Fixes
- Introduced ARAL_TRAILER_UNMARKING and ARAL_TRAILER_TAG_MASK; decode masks both; guarded by _Static_assert for alignment.
- aral_unmark_allocation() CASes (page|MARKED) -> (page|UNMARKING) before taking page_lock, decrements counters, then stores (page) under lock; early‑exit if freed, already unmarked, or CAS loses.
- aral_freez_internal() keeps the hot path as a single atomic exchange; if UNMARKING is seen, restores the tag (CAS if still zero), yields briefly, and retries; counts cold‑path hits via aral_freez_unmarking_observed_count under NETDATA_INTERNAL_CHECKS.
New Features
- Added aral_unittest_concurrency() (8 scenarios) with race hooks at UNMARK_BEFORE_CAS, UNMARK_AFTER_CAS, and FREEZ_BEFORE_CLAIM; includes a multi‑page stress that verifies the cold path; skipped under FSANITIZE_ADDRESS.
- Exposed via -W aralconcurrency; gated by NETDATA_INTERNAL_CHECKS with a clear stderr message when disabled; declarations updated in aral.h.

^{Written for commit 0e7d02e. Summary will update on new commits.}

…nter race and page-lifetime UAF Introduce a third trailer state (page | UNMARKING) that aral_unmark_allocation publishes before taking the page lock to decrement marked_elements, and clears after. While the bit is visible, aral_freez_internal observes it in its claim and spins until the final UNMARKED state is published, so: - freez never sees an unmarked trailer with a stale marked_elements counter (no spurious "marked > used" assertion under NETDATA_INTERNAL_CHECKS) - the slot's refcount contribution stays in place across the trailer transition, so the page cannot be destroyed under unmark's page_lock (closes the UAF that surfaced in production as gorilla_writer_aral_unmark SEGVs and aral_set_page_pointer dereferences of stale page pointers) The freez hot path keeps its single atomic-exchange; the cold path (UNMARKING observed) restores the bit and retries. Bit availability is guarded with a _Static_assert against SYSTEM_REQUIRED_ALIGNMENT. Add aral_unittest_concurrency() (8 scenarios) wired into aral_unittest under NETDATA_INTERNAL_CHECKS and reachable via -W aralconcurrency: 1-3 clean unmark / unmark-on-unmarked / clean freez (with marked guard on the same page) 4-6 forced races: freez wins claim, unmark wins trailer transition, and the same with no grace period - each verifies counter consistency 7 last marked on page triggers list move 8 coordinated stress: 256 pointers, deterministic UNMARKING for every slot, asserts the cold path was entered via a counter incremented in aral_claim_page_pointer_after_element___wait_for_unmark

cubic-dev-ai

No issues found across 3 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

Architecture diagram

sequenceDiagram
    participant C as Caller
    participant AU as aral_unmark_allocation
    participant AF as aral_freez_internal
    participant T as Slot Trailer (Atomic Word)
    participant P as ARAL Page (Counters & Lock)

    Note over AU, AF: Concurrent access to the same allocation slot

    rect rgb(240, 245, 255)
        Note right of AU: UNMARKING Phase
        AU->>T: NEW: atomic_compare_exchange(MARKED -> UNMARKING)
        Note over T: State: UNMARKING (Stays in place to keep Page alive)
    end

    rect rgb(255, 245, 240)
        Note right of AF: FREEZ (Claim) Phase
        AF->>T: CHANGED: atomic_exchange(0)
        T-->>AF: Returns UNMARKING
        
        alt UNMARKING Observed (Cold Path)
            AF->>T: NEW: restore UNMARKING via CAS(0, UNMARKING)
            AF->>AF: tinysleep() / yield
        end
    end

    rect rgb(240, 245, 255)
        Note right of AU: Counter Update Phase
        AU->>P: aral_page_lock()
        AU->>P: CHANGED: decrement page->marked_elements
        
        alt marked_elements == 0
            AU->>P: Move page to appropriate ARAL list
        end
        
        AU->>T: NEW: atomic_store(UNMARKED)
        AU->>P: aral_page_unlock()
    end

    rect rgb(255, 245, 240)
        Note right of AF: Retry Phase
        AF->>T: atomic_exchange(0)
        T-->>AF: Returns UNMARKED
        AF->>P: aral_page_lock()
        AF->>P: decrement page->used_elements
        Note over P: Counter Consistency: marked_elements <= used_elements
        AF->>P: aral_page_unlock()
        AF-->>C: Slot freed successfully
    end

Copilot

Pull request overview

This PR hardens ARAL’s unmark/freez concurrency by introducing a third trailer state (UNMARKING) to prevent counter invariants from being observed in an inconsistent state and to avoid page lifetime/UAF hazards during the trailer transition. It also adds a dedicated concurrency-focused unit test suite and exposes it via a new daemon CLI test option.

Changes:

Add a new trailer tag bit/state (UNMARKING) and update trailer encode/decode + freez claim logic to wait/retry when UNMARKING is observed.
Update aral_unmark_allocation() to CAS to UNMARKING before taking page_lock, then publish final UNMARKED trailer state after counters are updated.
Add aral_unittest_concurrency() (8 scenarios) and wire it into aral_unittest() and -W aralconcurrency.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/libnetdata/aral/aral.h	Declares the new `aral_unittest_concurrency()` entry point under `NETDATA_INTERNAL_CHECKS`.
src/libnetdata/aral/aral.c	Implements the `UNMARKING` trailer state machine, updates unmark/freez logic, and adds the new concurrency unit tests + deterministic race hook.
src/daemon/main.c	Adds `-W aralconcurrency` to run the new ARAL concurrency unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Fixes a race between aral_unmark_allocation() and aral_freez_internal() by introducing an UNMARKING trailer state, ensuring page counters and page lifetime remain consistent during the unmark/freez transition. Adds deterministic concurrency tests for the new unmark/freez state machine and wires them into the daemon unittest CLI option.

Changes:

Add ARAL_TRAILER_UNMARKING trailer state and update trailer encode/decode and free/unmark logic to wait out the transition safely.
Add aral_unittest_concurrency() covering multiple deterministic unmark/freez race scenarios (including a stress scenario).
Add -W aralconcurrency daemon option to run the concurrency tests under NETDATA_INTERNAL_CHECKS.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`src/libnetdata/aral/aral.h`	Exposes the new concurrency unittest API (guarded by `NETDATA_INTERNAL_CHECKS`).
`src/libnetdata/aral/aral.c`	Implements the `UNMARKING` trailer protocol, updates free/unmark interactions, and adds deterministic concurrency tests.
`src/daemon/main.c`	Adds CLI hook (`-W aralconcurrency`) to run the new ARAL concurrency tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Fixes a concurrency bug in ARAL’s per-slot trailer protocol by introducing an UNMARKING intermediate trailer state to serialize aral_unmark_allocation() vs aral_freez_internal(), preventing counter inconsistencies and page lifetime UAFs; adds deterministic concurrency tests and a CLI entry point to run them.

Changes:

Introduces ARAL_TRAILER_UNMARKING and updates trailer encode/decode masking to support 2 tag bits with a compile-time alignment guard.
Updates aral_unmark_allocation() to CAS MARKED -> UNMARKING, then update counters under page_lock, then publish final UNMARKED.
Updates aral_freez_internal() to wait/retry if it observes UNMARKING, and adds aral_unittest_concurrency() plus -W aralconcurrency wiring.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/libnetdata/aral/aral.h`	Exposes `aral_unittest_concurrency()` under `NETDATA_INTERNAL_CHECKS` and adjusts preprocessor structure around unittest entry points.
`src/libnetdata/aral/aral.c`	Implements the `UNMARKING` trailer state machine, updates unmark/freez synchronization, and adds deterministic concurrency unit tests.
`src/daemon/main.c`	Adds `-W aralconcurrency` CLI option to run the new ARAL concurrency tests (guarded by `NETDATA_INTERNAL_CHECKS`).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

stelfrag · 2026-05-02T08:10:01Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-05-02T08:10:06Z

@cubic-dev-ai review this PR

@stelfrag I have started the AI code review. It will take a few minutes to complete.

sonarqubecloud · 2026-05-02T08:11:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

thiagoftsm

Tested during few hours on Debian 13, and everything worked as expected. LGTM!

* aral: serialize unmark/freez via UNMARKING trailer state, prevent counter race and page-lifetime UAF Introduce a third trailer state (page | UNMARKING) that aral_unmark_allocation publishes before taking the page lock to decrement marked_elements, and clears after. While the bit is visible, aral_freez_internal observes it in its claim and spins until the final UNMARKED state is published, so: - freez never sees an unmarked trailer with a stale marked_elements counter (no spurious "marked > used" assertion under NETDATA_INTERNAL_CHECKS) - the slot's refcount contribution stays in place across the trailer transition, so the page cannot be destroyed under unmark's page_lock (closes the UAF that surfaced in production as gorilla_writer_aral_unmark SEGVs and aral_set_page_pointer dereferences of stale page pointers) The freez hot path keeps its single atomic-exchange; the cold path (UNMARKING observed) restores the bit and retries. Bit availability is guarded with a _Static_assert against SYSTEM_REQUIRED_ALIGNMENT. Add aral_unittest_concurrency() (8 scenarios) wired into aral_unittest under NETDATA_INTERNAL_CHECKS and reachable via -W aralconcurrency: 1-3 clean unmark / unmark-on-unmarked / clean freez (with marked guard on the same page) 4-6 forced races: freez wins claim, unmark wins trailer transition, and the same with no grace period - each verifies counter consistency 7 last marked on page triggers list move 8 coordinated stress: 256 pointers, deterministic UNMARKING for every slot, asserts the cold path was entered via a counter incremented in aral_claim_page_pointer_after_element___wait_for_unmark * Address review comments * Address review comments (2) * Address review comments (3) * Address review comments (4) * Update src/libnetdata/aral/aral.c Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Address review comments (5) * Address review comments (6) * Address review comments (7) --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> (cherry picked from commit e8caf80)

github-actions Bot added the area/daemon label May 1, 2026

stelfrag marked this pull request as ready for review May 1, 2026 16:02

stelfrag requested review from thiagoftsm and vkalintiris as code owners May 1, 2026 16:02

Copilot AI review requested due to automatic review settings May 1, 2026 16:02

stelfrag marked this pull request as draft May 1, 2026 16:02

Copilot started reviewing on behalf of stelfrag May 1, 2026 16:02 View session

cubic-dev-ai Bot reviewed May 1, 2026

View reviewed changes

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.c Outdated

Address review comments

0abdf3c

stelfrag requested a review from Copilot May 1, 2026 18:27

Copilot started reviewing on behalf of stelfrag May 1, 2026 18:27 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.h

Comment thread src/libnetdata/aral/aral.c Outdated

Comment thread src/libnetdata/aral/aral.c Outdated

Comment thread src/libnetdata/aral/aral.c Outdated

Comment thread src/libnetdata/aral/aral.c Outdated

stelfrag added 2 commits May 1, 2026 21:57

Address review comments (2)

39035df

Address review comments (3)

2575070

stelfrag requested a review from Copilot May 1, 2026 19:18

Copilot started reviewing on behalf of stelfrag May 1, 2026 19:19 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.h Outdated

Comment thread src/libnetdata/aral/aral.c

Address review comments (4)

c21c3b6

stelfrag requested a review from Copilot May 1, 2026 19:46

Copilot started reviewing on behalf of stelfrag May 1, 2026 19:47 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.c Outdated

Update src/libnetdata/aral/aral.c

f00c67c

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

stelfrag requested a review from Copilot May 1, 2026 19:56

Copilot started reviewing on behalf of stelfrag May 1, 2026 19:56 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.c Outdated

Comment thread src/libnetdata/aral/aral.c Outdated

Address review comments (5)

a5d54d4

stelfrag requested a review from Copilot May 1, 2026 20:21

Copilot started reviewing on behalf of stelfrag May 1, 2026 20:22 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread src/daemon/main.c

Address review comments (6)

b9a8bdb

stelfrag requested a review from Copilot May 2, 2026 07:51

Copilot started reviewing on behalf of stelfrag May 2, 2026 07:52 View session

Copilot AI reviewed May 2, 2026

View reviewed changes

Comment thread src/libnetdata/aral/aral.c Outdated

Comment thread src/libnetdata/aral/aral.c Outdated

Address review comments (7)

0e7d02e

stelfrag requested a review from Copilot May 2, 2026 08:03

Copilot started reviewing on behalf of stelfrag May 2, 2026 08:04 View session

Copilot AI reviewed May 2, 2026

View reviewed changes

stelfrag marked this pull request as ready for review May 2, 2026 08:51

thiagoftsm approved these changes May 2, 2026

View reviewed changes

stelfrag merged commit e8caf80 into netdata:master May 3, 2026
162 checks passed

stelfrag deleted the fix_aral_unmark_v2 branch May 3, 2026 15:16

stelfrag mentioned this pull request Jun 22, 2026

Patch release 2.10.4 #22825

Merged

Uh oh!

Conversation

stelfrag commented May 1, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

stelfrag commented May 2, 2026

Uh oh!

cubic-dev-ai Bot commented May 2, 2026

Uh oh!

sonarqubecloud Bot commented May 2, 2026

Quality Gate passed

Uh oh!

thiagoftsm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stelfrag commented May 1, 2026 •

edited by cubic-dev-ai Bot

Loading