Skip to content

fix(mutex): hold lock during notify_all in Barrier::wait to prevent teardown race (fixes #1672) - #2325

Merged
dmah42 merged 3 commits into
google:mainfrom
jdymitarai:fix/barrier-wait-teardown-1672
Oct 9, 2026
Merged

dmah42 merged 3 commits into
google:mainfrom
jdymitarai:fix/barrier-wait-teardown-1672

Conversation

@jdymitarai

Copy link
Copy Markdown
Contributor

Root Cause

In Barrier::wait() (src/mutex.h), MutexLock ml(lock_) was enclosed in an inner block that released the mutex before calling phase_condition_.notify_all(). When other threads awakened by createBarrier() finish their work and immediately destroy or free the Barrier instance, the last thread proceeds to call phase_condition_.notify_all() on a deallocated or destructed condition variable, resulting in a segmentation fault / null pointer dereference (#1672).

Fix Approach

Extend MutexLock ml(lock_) to cover phase_condition_.notify_all(). Holding the lock until after notify_all() completes ensures the Barrier object and its condition variable remain valid and protected from premature destruction by waking peer threads.

Verification

Added regression tests to test/benchmark_gtest.cc:

  • BarrierTest.SingleThreadedWaitAndTeardown: 100 iterations of single-threaded wait and immediate destruction.
  • BarrierTest.MultiThreadedWaitAndTeardown: 100 iterations of 8-threaded barrier wait and immediate join/destruction.
  • BarrierTest.MultiThreadedRemoveThreadAndTeardown: 100 iterations verifying removeThread() and concurrent teardown safety.
    All tests pass cleanly with zero sanitizer warnings.

dmah42
dmah42 previously approved these changes Oct 9, 2026
@dmah42

dmah42 commented Oct 9, 2026

Copy link
Copy Markdown
Member

nice catch. thanks. can you fix the formatting?

@jdymitarai

Copy link
Copy Markdown
Contributor Author

Updated the formatting in test/benchmark_gtest.cc to match clang-format. Thanks @dmah42!

@dmah42
dmah42 merged commit f2f7808 into google:main Oct 9, 2026
104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants