chenBright commented on code in PR #3551:
URL: https://github.com/apache/brpc/pull/3551#discussion_r4060835131
##########
src/bthread/butex.cpp:
##########
@@ -573,26 +573,37 @@ void wait_for_butex(void* arg) {
BAIDU_SCOPED_LOCK(b->waiter_lock);
if (b->value.load(butil::memory_order_relaxed) != bw->expected_value) {
bw->waiter_state = WAITER_STATE_UNMATCHEDVALUE;
- } else if (bw->waiter_state == WAITER_STATE_READY/*1*/ &&
- !bw->task_meta->interrupted) {
- if (args->prepend) {
- b->waiters.Prepend(bw);
- } else {
- b->waiters.Append(bw);
- }
- bw->container.store(b, butil::memory_order_relaxed);
+ } else {
+ // Checking `interrupted` and publishing `bw->container` must be
+ // atomic with respect to TaskGroup::interrupt(), which sets
+ // `interrupted` and consumes `current_waiter' under the same
+ // `version_lock`. Otherwise interrupt() may consume `bw` in
between
+ // and its erase_from_butex() does nothing because `container` is
+ // still nullptr, leaving this bthread queued but never woken up.
+ // `container` cannot be published upfront: it must stay nullptr
until
+ // the bthread is off its stack, see the comment after this block.
+ BAIDU_SCOPED_LOCK(bw->task_meta->version_lock);
+ if (bw->waiter_state == WAITER_STATE_READY/*1*/ &&
+ !bw->task_meta->interrupted) {
+ if (args->prepend) {
+ b->waiters.Prepend(bw);
+ } else {
+ b->waiters.Append(bw);
+ }
+ bw->container.store(b, butil::memory_order_relaxed);
#ifdef BRPC_BTHREAD_TRACER
- bw->control->_task_tracer.set_status(TASK_STATUS_SUSPENDED,
bw->task_meta);
+ TaskTracer::set_status_unsafe(TASK_STATUS_SUSPENDED,
bw->task_meta);
#endif // BRPC_BTHREAD_TRACER
- if (bw->abstime != nullptr) {
- bw->sleep_id = get_global_timer_thread()->schedule(
- erase_from_butex_and_wakeup, bw, *bw->abstime);
- if (!bw->sleep_id) { // TimerThread stopped.
- errno = ESTOP;
- erase_from_butex_and_wakeup(bw);
+ if (bw->abstime != nullptr) {
+ bw->sleep_id = get_global_timer_thread()->schedule(
+ erase_from_butex_and_wakeup, bw, *bw->abstime);
+ if (!bw->sleep_id) { // TimerThread stopped.
+ errno = ESTOP;
+ erase_from_butex_and_wakeup(bw);
Review Comment:
Confirmed and fixed in 62f8e5dd8eb823250198f838b6e6b70602e6b5b9. It was
actually two levels of self-deadlock on that path, since Butex::waiter_lock
(FastPthreadMutex) is non-recursive as well: erase_from_butex_and_wakeup() →
erase_from_butex() re-takes waiter_lock before ever reaching the tracer.
Fixed by narrowing the version_lock scope to the interrupted check + enqueue
+ container store, as suggested. Two deliberate deviations from the suggestion:
- The tracer update stays inside version_lock. It uses the new lock-free
set_status_unsafe(), so it cannot deadlock, and keeping it there makes the
transition to SUSPENDED atomic w.r.t. TraceImpl(). Moving it out would let a
concurrent butex_wake() set READY first, which we would then overwrite back to
SUSPENDED — TraceImpl() would then ContextTrace() a running bthread and return
a bogus stack.
- TimerThread::schedule() stays inside waiter_lock (but outside
version_lock). bw lives on butex_wait()'s stack: if scheduling happened after
releasing waiter_lock, a concurrent butex_wake() could remove bw and resume the
bthread, whose stack frame is then gone while the timer task still refers to it
(UAF), and unsleep_if_necessary() would miss sleep_id too. schedule() only
takes the timer's own locks, so it cannot deadlock on waiter_lock.
On schedule() failure, bw is now unlinked directly under the locks already
held (RemoveFromList() + container = nullptr + waiter_state = TIMEDOUT, errno =
ESTOP) and control falls through to the existing epilogue, which re-schedules
the bthread via ready_to_run(). That is safe.
--
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]
