This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 656ffa21522 [fix](be) Fix SIGSEGV in bvar::take_sample caused by
~AgentCombiner walking freed TLS agents (#67276)
656ffa21522 is described below
commit 656ffa21522dfa9c0ca40039fbd8c72422979800
Author: stinger1206 <[email protected]>
AuthorDate: Wed Sep 16 05:51:27 2026 +0300
[fix](be) Fix SIGSEGV in bvar::take_sample caused by ~AgentCombiner walking
freed TLS agents (#67276)
### What problem does this PR solve?
Issue Number: close #66895
Related PR: apache/brpc#3291
Problem Summary:
Follow-up to #64040 (backport of apache/brpc#2949). That fix made
`Agent::~Agent()`
safe by using a `weak_ptr` for `Agent::combiner`, but `~AgentCombiner()`
still calls
`clear_all_agents()`. When the last `shared_ptr` to the combiner is
released while
another thread is exiting, the TLS agents' `weak_ptr`s are already
expired: `~Agent`
skips `commit_and_erase()` and leaves its `LinkNode` in `_agents`, then
the
`ThreadBlock` is freed - and `clear_all_agents()` walks that freed TLS
storage,
causing a heap-use-after-free. In production this shows up as SIGSEGV in
`bvar::SeriesSampler::take_sample` (#66895).
Fix: port apache/brpc#3291 - `~AgentCombiner()` no longer traverses
`_agents`.
This is safe: `butil::LinkNode` has a trivial destructor and is never
dereferenced
when the list is torn down, and surviving agents observe
`combiner.expired() == true`
in `~Agent` and skip `commit_and_erase()`. There is no memory leak:
agents are freed
together with their `ThreadBlock` at thread exit. This closes the gap
that was
flagged during review of #64040.
### Release note
None
### Check List (For Author)
- Test
- [ ] Regression test
- [ ] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- Behavior changed:
- [x] No.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
Manual test:
1. Applied the patch on top of the existing
`brpc-1.4.0-fix-agent-combiner-thread-safety.patch` and rebuilt brpc
1.4.0
thirdparty + BE (4.1.3-based image).
2. Ran the same workload that previously triggered the crashes in
#66895:
sustained high-EPS stream load while rewriting a ~40B-row table from
storage format V2 to V3 on the affected production cluster
(compute-storage coupled mode, 6 BE nodes, Kubernetes).
3. Before the fix: BEs crashed with SIGSEGV in
`bvar::SeriesSampler::take_sample` (two BEs independently, see issue).
After the fix: the production cluster has been running stable for 2 days
with the patch applied - no recurrence of the SIGSEGV.
### Check List (For Reviewer who merge this PR)
- Confirm the release note
- Confirm test cases
- Confirm document
- Add branch pick label
Co-authored-by: stinger1206 <[email protected]>
---
...c-1.4.0-fix-agent-combiner-use-after-free.patch | 86 ++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git
a/thirdparty/patches/brpc-1.4.0-fix-agent-combiner-use-after-free.patch
b/thirdparty/patches/brpc-1.4.0-fix-agent-combiner-use-after-free.patch
new file mode 100644
index 00000000000..c88455d27a1
--- /dev/null
+++ b/thirdparty/patches/brpc-1.4.0-fix-agent-combiner-use-after-free.patch
@@ -0,0 +1,86 @@
+diff --git a/src/bvar/detail/combiner.h b/src/bvar/detail/combiner.h
+index 869e2b5..2e64e31 100644
+--- a/src/bvar/detail/combiner.h
++++ b/src/bvar/detail/combiner.h
+@@ -235,7 +235,37 @@ friend class GlobalValue<self_type>;
+
+ ~AgentCombiner() {
+ if (_id >= 0) {
+- clear_all_agents();
++ // NOTE: We intentionally do NOT walk `_agents` here (e.g. via the
++ // previously existed `clear_all_agents()`).
++ //
++ // `Agent` instances live inside per-thread `ThreadBlock`s owned
by
++ // `AgentGroup` and are destroyed when their owning thread exits
++ // (via `_destroy_tls_blocks`). At that point `~Agent` calls
++ // `combiner.lock()`; if the combiner has already started its
++ // destruction the `weak_ptr` is expired and the agent will skip
++ // `commit_and_erase`, leaving its `LinkNode` linked to this
++ // combiner's `_agents`. If we tried to traverse `_agents` here we
++ // could touch agent nodes whose `ThreadBlock` was just freed by
++ // a concurrent thread-exit, causing heap-use-after-free.
++ //
++ // It is safe to leave the list "dirty" because:
++ // * `butil::LinkedList` / `butil::LinkNode` have trivial
++ // destructors and never traverse on destruction, so tearing
++ // down `_agents` here does not dereference any agent node.
++ // * After this combiner is gone, every still-alive `Agent` will
++ // observe `combiner.expired() == true` in `~Agent` and skip
++ // `commit_and_erase`, so the dangling `prev_/next_` pointers
++ // in those agents are never read.
++ // * If the freed `_id` is later reused by a new combiner and
the
++ // same TLS slot is taken, `get_or_create_tls_agent` will call
++ // `Agent::reset` and `Append` the agent into the new
++ // combiner's `_agents`. `LinkNode::InsertBefore` only writes
++ // `prev_/next_` (never reads their stale values), so the
++ // dangling pointers are safely overwritten.
++ // * `Agent::element` is destroyed together with the
`ThreadBlock`,
++ // so any non-POD resource it holds is still released; if the
++ // agent slot is reused, `Agent::reset` will overwrite the
++ // element value before it is observed again.
+ AgentGroup::destroy_agent(_id);
+ _id = -1;
+ }
+@@ -321,18 +351,30 @@ friend class GlobalValue<self_type>;
+ return agent;
+ }
+
+- void clear_all_agents() {
+- butil::AutoLock guard(_lock);
+- // reseting agents is must because the agent object may be reused.
+- // Set element to be default-constructed so that if it's non-pod,
+- // internal allocations should be released.
+- for (butil::LinkNode<Agent>* node = _agents.head(); node !=
_agents.end();) {
+- node->value()->reset(ElementTp(), NULL);
+- butil::LinkNode<Agent>* const saved_next = node->next();
+- node->RemoveFromList();
+- node = saved_next;
+- }
+- }
++ // NOTE: `clear_all_agents()` is intentionally kept but no longer called
++ // from `~AgentCombiner` (see the long comment in `~AgentCombiner`).
++ //
++ // Calling it from the destructor is unsafe: by the time the destructor
++ // runs, agent weak_ptrs have already expired and `~Agent` will skip
++ // `commit_and_erase`; a concurrent thread-exit can therefore free the
++ // `ThreadBlock` (and the agents inside it) while we are still walking
++ // `_agents` here, which is a heap-use-after-free.
++ //
++ // The body is left around (commented out) for reference / future use --
++ // do NOT re-enable it from `~AgentCombiner`.
++ //
++ // void clear_all_agents() {
++ // butil::AutoLock guard(_lock);
++ // // reseting agents is must because the agent object may be reused.
++ // // Set element to be default-constructed so that if it's non-pod,
++ // // internal allocations should be released.
++ // for (butil::LinkNode<Agent>* node = _agents.head(); node !=
_agents.end();) {
++ // node->value()->reset(ElementTp(), NULL);
++ // butil::LinkNode<Agent>* const saved_next = node->next();
++ // node->RemoveFromList();
++ // node = saved_next;
++ // }
++ // }
+
+ const BinaryOp& op() const { return _op; }
+
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
