Skip to content

Fix Collector use-after-free during process exit - #3721

Merged
chenBright merged 2 commits into
apache:masterfrom
chenBright:fix-collector
Nov 10, 2026
Merged

chenBright merged 2 commits into
apache:masterfrom
chenBright:fix-collector

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Collector uses a leaky singleton, so its destructor never stops its worker threads during normal process exit. A pending RPC dump can therefore read rpc_dump_dir after its string storage has been destroyed, causing a heap-use-after-free.

What is changed and the side effects?

Changed:

  • Add an idempotent shutdown hook that stops and joins both Collector threads while retaining the singleton.
  • Ensure Collector shutdown completes before RPC dump flags are destroyed, including when Collector was initialized before main().
  • Serialize first-time initialization with shutdown, use an atomic stop flag, and reject submissions after shutdown.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An active concurrency defect can allow a post-shutdown submission to escape final cleanup, and the regression tests do not cover the pre-main initialization teardown ordering.

2 open issues
What changed in this PR

This pull request adds coordinated shutdown for the process-wide bvar collector, ensuring worker threads finish before RPC dump configuration is destroyed during process exit.

Changes:

  • Adds synchronized collector initialization and shutdown with an atomic stop state.
  • Stops and joins collector workers, cleans up pending samples, and rejects post-shutdown submissions.
  • Adds RPC dump exit-ordering safeguards.
  • Adds shutdown, concurrency, and process-exit regression tests.

This pull request requires changes.

File Description
test/​brpc_collector_shutdown_unittest.cpp Adds process-exit and concurrency tests for collector shutdown behavior.
src/​bvar/​collector.h Declares the internal collector shutdown hook and documents post-shutdown submission behavior.
src/​bvar/​collector.cpp Implements serialized initialization, shutdown coordination, worker joining, pending-sample cleanup, and shutdown-aware submissions.
src/​brpc/​rpc_dump.cpp Registers RPC dump shutdown before its flags are destroyed.

Review effort: Lite

Comment thread src/bvar/collector.cpp Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Runtime sanitizer/build validation across pthread and TLS shutdown interleavings, atexit and static teardown ordering, and supported link modes was unavailable, leaving correctness and duplicate-source test-linking unconfirmed.

No open issues 🎉

2 resolved since last review

Review effort: Lite

@wasphin wasphin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chenBright
chenBright merged commit f729504 into apache:master Nov 10, 2026
45 of 47 checks passed
@chenBright
chenBright deleted the fix-collector branch November 10, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants