Harden bvar sampling against use-after-free - #3487
Open
chenBright wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens bvar sampling against silent use-after-free by making sampling independent of host lifetime where possible and by detecting/leaking borrowed samplers when Window/PerSecond outlive their referenced bvar.
Changes:
- Teach
ReducerSamplerto sample via a shared combiner (when available) instead of dereferencing the host after construction. - Add borrower tracking + diagnostics to
Samplerto detectWindow/PerSecondoutliving a referenced bvar and intentionally leak the sampler to avoid UAF. - Update
WindowBaseseries sampling to copy the operator instead of touching the underlying var from the sampling thread; add a regression test for the lifetime violation case.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/bvar_window_unittest.cpp | Adds regression test for Window/PerSecond outliving referenced bvar (skipped under ASan due to intentional leak). |
| test/bvar_sampler_unittest.cpp | Removes unused includes related to sampler tests. |
| src/bvar/window.h | Copies underlying var operator, adds sampler borrower tracking calls, and adjusts series sampler to avoid dereferencing destroyed vars. |
| src/bvar/reducer.h | Adds share_combiner() + sampler debug name wiring for better lifetime safety and diagnostics. |
| src/bvar/recorder.h | Adds share_combiner(), improves sampler debug naming, and updates expose/debug-name propagation. |
| src/bvar/passive_status.h | Sets sampler debug name and updates expose path to refresh it when available. |
| src/bvar/latency_recorder.cpp | Adds compile-time checks validating share-combiner detection behavior. |
| src/bvar/detail/sampler.h | Adds borrower tracking + debug name, and introduces share-combiner-based sampling sources for ReducerSampler. |
| src/bvar/detail/sampler.cpp | Implements borrower tracking/leak behavior, adds a gflag, and respects _leaked in collector deletion. |
| src/bvar/detail/percentile.h | Adds share_combiner() and propagates debug names to sampler instances. |
| src/brpc/input_messenger.cpp | Fixes indentation for an existing error message call. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _leaked = true; | ||
| std::string owner = | ||
| _debug_name.empty() ? "An unnamed bvar" : "bvar "; | ||
| if (_debug_name.empty()) { |
Comment on lines
+226
to
+251
| BAIDU_SCOPED_LOCK(_mutex); | ||
| _used = false; | ||
| _mutex.unlock(); | ||
| if (_nborrow > 0) { | ||
| // The owning bvar is being destructed while Window/PerSecond objects | ||
| // still borrow this sampler. Leak the sampler so that the borrowers | ||
| // keep pointing at valid memory (they just stop getting new samples), | ||
| // which turns a use-after-free into a bounded leak. | ||
| _leaked = true; | ||
| std::string owner = | ||
| _debug_name.empty() ? "An unnamed bvar" : "bvar "; | ||
| if (_debug_name.empty()) { | ||
| owner.append("'").append(_debug_name).append("'"); | ||
| } | ||
| if (FLAGS_bvar_abort_on_sampler_still_borrowed) { | ||
| LOG(FATAL) << "Abort because " << owner << " is destructed while " | ||
| << _nborrow << " Window/PerSecond still reference its" | ||
| " sampler"; | ||
| } else { | ||
| LOG(ERROR) << owner << " is destructed while " << _nborrow | ||
| << " Window/PerSecond still reference its sampler. The" | ||
| " bvar referenced by a Window MUST be destructed" | ||
| " AFTER that Window, see comments of Window in" | ||
| " bvar/window.h. The sampler is leaked to avoid a" | ||
| " dangling pointer."; | ||
| } | ||
| } |
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.
What problem does this PR solve?
Issue Number: resolve
Problem Summary:
bvar sampling has two lifetime hazards, both of which end up as a silent
use-after-free:
detail::ReducerSamplerholds a rawR* _reducerand dereferences it intake_sample()andget_value(). Safety therefore relies entirely on everyhost correctly calling
Sampler::destroy()from its dtor, which is a convention,not a structural guarantee.
Window/PerSecondborrow the sampler of the bvar they reference(
var->get_sampler()), andWindowBase::SeriesSampler::Opholds a rawR* _var. If a Window outlives that bvar (violating the contract documentedin
bvar/window.h), the sampling thread has alreadydeleted the sampler, sothe Window is left with a permanently dangling pointer: both
get_value()andthe series sampler become a use-after-free, and nothing reports the misuse.
What is changed and the side effects?
Changed:
Sample through the shared data carrier where possible.
ReducerSamplernow selects its data source with a trait: hosts exposingshare_combiner()are sampled through theirshared_ptr<AgentCombiner>plusby-value copies of
Op/InvOp, so the sampler never dereferences the host afterconstruction. Since the sampler keeps a reference to the combiner, sampling reads
valid memory even if the host is destructed before the sampler is recycled.
share_combiner()is added toReducer(Adder/Maxer/Miner),IntRecorderandPercentile(the one insideLatencyRecorder). Hosts without such a carrier --PassiveStatus(data lives in a user callback) and the babylon variants (valuetypes) -- keep the previous host-pointer mode, so their behaviour is unchanged.
Detect a Window outliving its bvar, and degrade UAF to a bounded leak.
Samplergains a borrower counter (guarded by its existing_mutex) withadd_borrower()/remove_borrower(), called byWindowBase's ctor/dtor. Ifdestroy()finds the sampler still borrowed, it reports the misuse (includingthe bvar name) and marks the sampler, and the sampling thread then skips the
delete, leaking it on purpose. The borrowers keep pointing at valid memoryand merely stop receiving new samples. Note the counter and the "don't delete"
part are inseparable: otherwise
remove_borrower()itself would be ause-after-free. A new gflag
bvar_abort_on_sampler_still_borrowed(defaultfalse, i.e.LOG(ERROR)) can escalate this to an abort, mirroring the existingbvar_abort_on_same_name.3. Stop touching the var from the series sampler.
WindowBase::SeriesSampler::Opused to holdR* _varand call_var->op()fromthe sampling thread. It now holds a copy of the operator (bvar operators such as
AddTo/MaxTo/AddStatare stateless functors), copied once intoWindowBase::_var_opat construction. As a result
_varis only dereferenced in the ctor and never afterwards.Side effects:
Performance effects:
Breaking backward compatibility:
Check List: