apache / apache/brpc

[bvar] AgentGroup may use a stale _s_tls_blocks address after bthread migration under Clang

Open
#3,520 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
17.6k
Forks
4.1k
Avg merge
2d 12h
Merged PRs (30d)
69

Description

**Describe the bug**

We observed a `SIGSEGV` in `bvar::detail::AgentGroup<...>::get_tls_agent()` while updating a `bvar::Adder` from a bthread. The captured core and generated code indicate that the bthread suspended in a synchronous RPC, migrated from one pthread to another, and then continued to use the `_s_tls_blocks` TLS address resolved for the previous pthread.

The crash was observed in a downstream Apache Doris BE process built with Clang 16 on Linux x86_64 and using brpc 1.4.0. This report is based on source, core, register, and disassembly analysis. We have not attempted a standalone reproduction.

### Relevant implementation

`AgentGroup` declares its per-pthread block vector as a raw TLS static member:

```cpp
static __thread std::vector* _s_tls_blocks;
```

The inline methods `get_tls_agent()` and `get_or_create_tls_agent()` read and write `_s_tls_blocks` directly. The same direct-access pattern is still present in [`agent_group.h` in brpc 1.17.0](https://github.com/apache/brpc/blob/1.17.0/src/bvar/detail/agent_group.h).

Clang kept the address of this TLS slot alive across a call that suspended the current bthread. After the bthread resumed on another pthread, the inlined bvar path still used the address belonging to the original pthread.

### Core evidence

The relevant call path was:

```text
application code after a synchronous RPC
-> bvar::Reducer::operator<<
-> bvar::detail::AgentCombiner::get_or_create_tls_agent
-> bvar::detail::AgentGroup<...>::get_tls_agent(id=124)
-> SIGSEGV
```

The faulting instruction in the inlined `get_tls_agent()` path was:

```asm
mov (%rcx,%rax,8), %rbx
```

At the crash:

```text
id = 124
block_id = 0
rcx = 0x0
current pthread fs_base = 0x7fa6bd997700 (pthread B, LWP 2432)
cached TLS slot address = 0x7fa90bd1eaf8
other pthread fs_base = 0x7fa90bd25700 (pthread A, LWP 1244)
```

The cached slot address is exactly `pthread A fs_base - 0x6c08`, where `-0x6c08` is the `_s_tls_blocks` TLS offset in this binary. It does not correspond to the currently executing pthread B. This shows that execution resumed on pthread B while retaining the `_s_tls_blocks` address from pthread A.

The size check immediately before the indexed load had passed, but the vector data pointer used by the indexed load was null. This is consistent with pthread B reading pthread A's stale TLS vector while pthread A concurrently initialized or resized that vector. Even without that concurrent resize, accessing another pthread's bvar agents is incorrect.

The resulting sequence is:

```text
bthread runs on pthread A
-> Clang resolves/caches the address of AgentGroup::_s_tls_blocks
-> synchronous RPC suspends the bthread
-> bthread resumes on pthread B
-> inlined bvar code reuses pthread A's TLS address
-> pthread A concurrently mutates its own AgentGroup vector
-> inconsistent vector state is observed and get_tls_agent() crashes
```

This is not specific to the application RPC or metric. Any inlined bvar access whose TLS address is kept across a bthread suspend/migration point may be affected.

**To Reproduce**

We have not run a standalone reproducer. The observed trigger had the following shape:

```cpp
void run_in_bthread() {
// Work in this function causes Clang to resolve/retain the
// AgentGroup::_s_tls_blocks TLS address.

synchronous_rpc_that_suspends_the_bthread();
// The bthread may now be running on a different pthread.

adder << 1; // inlined AgentGroup access
}
```

A deterministic regression test could use explicit handshakes to:

1. Start the bthread on pthread A and ensure the relevant TLS address has been used.
2. Suspend it at a controlled point and resume it on pthread B.
3. Concurrently initialize enough agents on pthread A to initialize or resize that `AgentGroup` specialization's TLS vector.
4. Verify that the post-migration bvar operation resolves `_s_tls_blocks` from pthread B and neither accesses pthread A's vector nor crashes.

The test should use synchronization/handshakes rather than timing-based sleeps.

**Expected behavior**

Every `AgentGroup` operation executed after a bthread resumes should resolve `_s_tls_blocks` for the currently executing pthread. A bthread migration must not cause bvar to access another pthread's agent vector or crash.

**Versions**

```text
OS: Linux x86_64 (exact distribution unavailable)
Compiler: Clang 16, optimized build
brpc: 1.4.0 plus the downstream Apache Doris patch set
protobuf: N/A to this failure
```

The affected `AgentGroup` implementation matches upstream brpc 1.4.0; the downstream patch set does not modify this code. The crash has not been reproduced against brpc 1.17.0. However, static inspection shows that 1.17.0 still directly accesses raw `_s_tls_blocks`, so the specific `AgentGroup` gap remains in the latest release source.

**Additional context/screenshots**

### Existing related issues and fixes

- [`#1776`](https://github.com/apache/brpc/issues/1776) describes the same class of failure: a compiler-cached `tls_bls` address remains associated with pthread A after a bthread migrates to pthread B.
- [`#1860`](https://github.com/apache/brpc/issues/1860) discusses Clang/LTO retaining a TLS `errno` address across bthread context switches.
- [`#845`](https://github.com/apache/brpc/issues/845) and [`#1407`](https://github.com/apache/brpc/issues/1407) are earlier `TaskGroup` failures related to compiler-optimized TLS access.
- [`PR #2156`](https://github.com/apache/brpc/pull/2156) introduced noinline/asm-based volatile TLS accessors for `tls_task_group`.
- [`PR #2248`](https://github.com/apache/brpc/pull/2248) enabled that protection for Clang on x86_64 as well.
- [`PR #2934`](https://github.com/apache/brpc/pull/2934) applied the same mechanism to `tls_bls`.
- [`LLVM issue #98479`](https://github.com/llvm/llvm-project/issues/98479) documents TLS addresses being kept alive across stackful-fiber suspension and migration to another OS thread.

These fixes establish the required access pattern, but none of them changes `bvar::detail::AgentGroup::_s_tls_blocks`. An exact search for `_s_tls_blocks` in brpc issues only found unrelated memory-layout and compilation reports; no existing issue appears to cover this `AgentGroup` migration case.

### Possible fix direction

brpc already provides `STATIC_MEMBER_BAIDU_VOLATILE_THREAD_LOCAL`, `BAIDU_GET_VOLATILE_THREAD_LOCAL`, and `BAIDU_SET_VOLATILE_THREAD_LOCAL` in [`butil/thread_local.h`](https://github.com/apache/brpc/blob/1.17.0/src/butil/thread_local.h), and uses this pattern for static-member TLS in [`ObjectPool`](https://github.com/apache/brpc/blob/1.17.0/src/butil/object_pool_inl.h).

A possible fix is to apply the same mechanism to `AgentGroup::_s_tls_blocks` and route all reads and writes in `get_tls_agent()`, `get_or_create_tls_agent()`, and `_destroy_tls_blocks()` through the accessor functions. Each operation should first obtain the current pthread's vector through the noinline accessor and then consistently use that local pointer.

An ordinary null check, value-level `volatile`, atomic operation, or memory fence is not sufficient because the problematic optimization retains the TLS address itself.

### Evidence limitations

- The crash and TLS-address relationship come from the captured incident's core analysis; the original core is not publicly available.
- No standalone reproduction has been attempted.
- brpc 1.17.0 has only been inspected statically for this report; it has not been runtime-tested against this trigger.

Contributor guide

Open the contributing guide

Research direction

Start with bvar/detail/agent_group.h, focusing on get_tls_agent(), get_or_create_tls_agent(), and _destroy_tls_blocks(). Compare the TLS access pattern with butil/thread_local.h and the static-member usage in butil/object_pool_inl.h, then inspect related TLS fixes in issues #1776 and #2934. Done means a deterministic migration test confirms post-resume operations use the current pthread's agent vector without crashing.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.