Repository navigation
Restore 4.10.0 ABI of Sleep/Delay/Timeout nodes; ignore stale timer handlers - #1283
Open
facontidavide wants to merge 3 commits into
Open
facontidavide wants to merge 3 commits into
facontidavide wants to merge 3 commits into
Conversation
…fix #1180 fixed a use-after-free of the node mutex on destruction by moving TimerQueue<> timer_ from the first to the last data member of SleepNode, DelayNode and TimeoutNode, so that ~TimerQueue() (which joins the worker thread running the handlers that lock delay_mutex_ / timeout_mutex_) runs before those mutexes are destroyed. The reorder changed every member offset vs 4.10.0 while constructors/destructors are inline, breaking the ABI for plugins built against 4.10.0. Restore the 4.10.0 member order and keep the fix by joining the worker explicitly: TimerQueue gets an idempotent stop() (the former destructor body: cancel all, drain the handlers, join), and the three node destructors call it before any member is destroyed. ~TimeoutNode still resets timeout_started_ under timeout_mutex_ (#1241) but releases the lock before stop(), since the handler takes it. sizeof and all member offsets of the three nodes and TimerQueue are again identical to 4.10.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TimerQueue::cancel() neither prevents nor waits for a handler that the worker thread already popped: it returns 0 and the handler then runs with aborted == false. If the node was halted (or its child completed) and restarted in that window, the stale handler acted on the new run: a restarted Sleep returned SUCCESS early, a restarted Delay ticked its child without delay, a restarted Timeout halted its new child. Each handler now checks that it belongs to the current timer, reusing the existing timer_id_ member (no layout change). The node mutex is held across add() and the assignment of timer_id_ (and of the handler's own id), so the handler, which takes the same mutex before comparing, never sees a half-set state. halt() and Timeout's completion path reset timer_id_ to 0 under the mutex. No code waits for a handler while holding its mutex. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores the 4.10.0 ABI of
SleepNode,DelayNodeandTimeoutNode, keeps the use-after-free fix from #1180, and fixes a remaining stale-timer-handler race.1. ABI (aec7ab5)
#1180 (9b63b50) fixed a use-after-free on destruction by moving
TimerQueue<> timer_to the last member, so the timer is destroyed, and its worker joined, before the mutex the handler locks. That changed every member offset against 4.10.0 (sizeofstayed the same). These classes have inline constructors and destructors in the headers, whiletick()/halt()live in the library, and the soname is unversioned. So a plugin built against 4.10.0 that derives from or directly constructs one of them would crash with a newer library.TimerQueue::stop()holds the former destructor body (cancel all, finish, notify, join). It is idempotent, and~TimerQueue()calls it. TimerQueue's layout is unchanged.timer_.stop()first, so every pending or aborted handler finishes while the node's mutexes are still alive. Destruction order now comes from code instead of member order.A sizeof/offsetof probe, compiled against the 4.10.0 headers and this branch, shows identical layouts for all three nodes and for
TimerQueue.Note: on glibc,
std::mutexhas a trivial destructor, so #1180's regression test can't catch the bug on Linux. With libc++ and a real~mutex(as on macOS), the test aborts on every run with "mutex lock failed" whenstop()is removed, and passes 5/5 with it.2. Stale timer handlers (5743c18)
TimerQueue::cancel()neither prevents nor waits for a handler the worker has already popped. That handler then runs withaborted=falseand acts on the restarted node:Each handler now captures the id it was created for and, under the node mutex, returns unless that id still equals
timer_id_.halt()and the Timeout completion path resettimer_id_to 0. No header changes. New testDecorator.TimeoutRestartedIgnoresStaleTimerHandlerfailed 13/13 on the old code. Sleep and Delay are fixed the same way, but can't be tested deterministically without hooks in the headers.Tests
Full suite passes. TSan ×10 and ASan+UBSan ×5 on the Sleep/Delay/Timeout/Timer tests are clean.
🤖 Generated with Claude Code