chenBright opened a new pull request, #3551:
URL: https://github.com/apache/brpc/pull/3551
### What problem does this PR solve?
Issue Number: resolve
Problem Summary:
There are two related bugs between `butex_wait()` and
`TaskGroup::interrupt()`,
both exposed by `ButexTest.join_cant_be_wakeup` under ASAN in CI.
1. Uninitialized `ButexPthreadWaiter::container` crash.
In `butex_wait_from_pthread()`, the on-stack `ButexPthreadWaiter pw` is
published to `TaskMeta::current_waiter` before its `container` field is
initialized. A concurrent `TaskGroup::interrupt()` consumes
`current_waiter`
and calls `erase_from_butex()` on it, which loads `container` and, if
it is
non-null, dereferences it as a `Butex*` and locks it. Reading
uninitialized
stack memory there makes `erase_from_butex()` lock garbage and crash.
The
ASAN report from
[CI](https://github.com/apache/brpc/actions/runs/34238048209/job/102100771652):
```text
==41009==ERROR: AddressSanitizer: SEGV on unknown address 0x7fceda8e0440 (pc
0x7fced9c1ef7a bp 0x7fff5f9a44c0 sp 0x7fff5f9a4420 T0)
==41009==The signal is caused by a WRITE memory access.
#0 0x7fced9c1ef7a in std::__atomic_base<unsigned
char>::exchange(unsigned char, std::memory_order)
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/atomic_base.h:506:9
#1 0x7fced9c1ef7a in butil::static_atomic<unsigned
char>::exchange(unsigned char, std::memory_order)
/home/runner/work/brpc/brpc/./src/butil/atomicops.h:295:52
#2 0x7fced9c1ef7a in bthread::internal::FastPthreadMutex::try_lock()
/home/runner/work/brpc/brpc/src/bthread/mutex.cpp:1150:32
#3 0x7fced9c1ef7a in bthread::internal::FastPthreadMutex::lock()
/home/runner/work/brpc/brpc/src/bthread/mutex.cpp:1140:9
#4 0x7fced9c1ef7a in
bthread::internal::pthread_mutex_lock_internal(bthread::internal::FastPthreadMutex*,
timespec const*) /home/runner/work/brpc/brpc/src/bthread/mutex.cpp:861:16
#5 0x7fced9c1ef7a in int
bthread::internal::pthread_mutex_lock_impl<bthread::internal::FastPthreadMutex>(bthread::internal::FastPthreadMutex*,
timespec const*) /home/runner/work/brpc/brpc/src/bthread/mutex.cpp:884:16
#6 0x7fced9c1ef7a in bthread::FastPthreadMutex::lock()
/home/runner/work/brpc/brpc/src/bthread/mutex.cpp:1180:5
#7 0x7fced9bf3826 in
std::lock_guard<bthread::FastPthreadMutex>::lock_guard(bthread::FastPthreadMutex&)
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_mutex.h:229:19
#8 0x7fced9bf3826 in bthread::erase_from_butex(bthread::ButexWaiter*,
bool, bthread::WaiterState)
/home/runner/work/brpc/brpc/src/bthread/butex.cpp:524:9
#9 0x7fced9c979f3 in bthread::TaskGroup::interrupt(unsigned long,
bthread::TaskControl*)
/home/runner/work/brpc/brpc/src/bthread/task_group.cpp:1176:9
#10 0x4e8134 in (anonymous
namespace)::ButexTest_join_cant_be_wakeup_Test::TestBody()
/home/runner/work/brpc/brpc/test/bthread_butex_unittest.cpp:303:9
#11 0x528bfe in void
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
void>(testing::Test*, void (testing::Test::*)(), char const*)
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x528bfe)
#12 0x51cce5 in testing::Test::Run()
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x51cce5)
#13 0x51ce64 in testing::TestInfo::Run()
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x51ce64)
#14 0x51d418 in testing::TestSuite::Run()
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x51d418)
#15 0x51db1e in testing::internal::UnitTestImpl::RunAllTests()
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x51db1e)
#16 0x5291c6 in bool
testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool>(testing::internal::UnitTestImpl*, bool
(testing::internal::UnitTestImpl::*)(), char const*)
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x5291c6)
#17 0x51cf2b in testing::UnitTest::Run()
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x51cf2b)
#18 0x42cab3 in main
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x42cab3)
#19 0x7fced8829d8f in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#20 0x7fced8829e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#21 0x42cb14 in _start
(/home/runner/work/brpc/brpc/test/bthread_butex_unittest+0x42cb14)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
/usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/atomic_base.h:506:9
in std::__atomic_base<unsigned char>::exchange(unsigned char,
std::memory_order)
```
2. Lost interruption.
Even with `container` initialized to nullptr, checking
`TaskMeta::interrupted`
and publishing `container` were not atomic with respect to `interrupt()`.
`interrupt()` sets `interrupted` and consumes `current_waiter` under
`version_lock`. If it slips in between the check and the publish, its
`erase_from_butex()` observes `container` still nullptr (a no-op) and no
one
wakes `pw` up, so the waiter blocks forever (or until timeout) and the
interruption is lost.
### What is changed and the side effects?
Changed:
* Initialize `pw.container` to nullptr before `pw` is published to
`current_waiter`, so a racing `interrupt()` observes either nullptr (its
`erase_from_butex()` is a no-op) or a valid `Butex*`, never stack
garbage.
* Keep `container` nullptr until `pw` is queued, then check `interrupted`
and
publish `container` together under `version_lock`. This makes the check
and
the publish atomic with respect to `interrupt()`, so `interrupt()` can
only
land on either side of them, never in between. This mirrors the existing
logic in `wait_for_butex()` for bthread waiters.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
