Fix potential bvar deadlock by running describe()/dump() outside the global VarMap lock - #3470
Open
chenBright wants to merge 1 commit into
Open
Fix potential bvar deadlock by running describe()/dump() outside the global VarMap lock#3470chenBright wants to merge 1 commit into
chenBright wants to merge 1 commit into
Conversation
…global VarMap lock
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a potential deadlock in bvar caused by invoking user-defined describe() / dump() callbacks while holding the global VarMap (pthread) mutex. It introduces an indirection handle so readers can safely call user callbacks outside the map lock, while hide() waits for in-flight readers to finish to prevent use-after-free.
Changes:
- Add
bvar/detail/exposed_ref.himplementingdetail::ExposedRef<T>(ref-count + hide-and-wait) and wire it into bothVariableandMVariableBaseexposure paths. - Update
Variable::{describe_exposed, describe_series_exposed, get_exposed}andMVariableBase::{describe_exposed, dump_exposed}to acquire a safe handle under the map lock, then call user code after releasing the lock. - Add regression/unit tests covering (1) bthread-yield deadlock reproduction and (2) destructor waiting for in-flight
describe_exposed()to complete.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/bvar_variable_unittest.cpp | Adds a test ensuring hide()/destruction waits for an in-flight describe_exposed() callback to finish. |
| test/bthread_unittest.cpp | Adds a regression test reproducing issue #2888 (many bthreads + yielding callback) and ensuring no deadlock. |
| src/bvar/variable.h | Adds an exposed-handle member (SharedExposedRef) to Variable for safe describe/get outside the global map lock. |
| src/bvar/variable.cpp | Reworks VarMap entries to store the handle, moves describe()/get_value()/describe_series() out of the map lock, and makes hide() wait for in-flight readers. |
| src/bvar/mvariable.h | Adds the same exposed-handle mechanism to MVariableBase. |
| src/bvar/mvariable.cpp | Updates describe_exposed()/dump_exposed() to acquire under lock and invoke user callbacks outside the lock; hide() waits for readers. |
| src/bvar/detail/exposed_ref.h | New shared indirection/ref-count + hide-and-wait primitive used to prevent deadlocks and UAF. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 #2888
Problem Summary:
bvar's global
VarMapis guarded by a pthread mutex.Variable::describe_exposed()(and
describe_series_exposed(),dump_exposed(), plus the multi-dimensionMVariableBasecounterparts) used to invokevar->describe()while holding that lock.For
PassiveStatus,describe()runs a user-provided callback; if the callback yieldsthe bthread (e.g. by acquiring a
bthread::Mutex), the pthread mutex is never releasedand the process deadlocks.
What is changed and the side effects?
Changed:
Run user callbacks OUTSIDE the global map lock via a small indirection handle:
bvar/detail/exposed_ref.h:ExposedRef<T>(a reference-counted handleguarding an exposed object). It uses
butil::Mutex+butil::ConditionVariable.describe_exposed()/describe_series_exposed()/get_exposed(): under themap lock they now only
seek+acquire()(ref-count +1, serialized withhide()'serase); the lock is released,
describe()is called outside the lock, thenrelease().hide()now also invalidates the handle and blocks (hide_and_wait()) untilall in-flight readers finish, so a Variable cannot be destroyed while a concurrent
describe()is still using it. Eachexpose()rebuilds a fresh handle (the old oneis single-use once hidden).
MVariableBase::describe_exposed()/dump_exposed()get the same treatment;dump()in particular is moved outside the lock becauseDumperis auser-overridable interface that may yield.
VarMapmutex is nolonger needed and is reverted to a plain mutex (now consistent with
MVarMap).Side effects:
Performance effects:
Breaking backward compatibility:
Check List: