sched: skip waking a null or invalid thread pointer - #1484
Open
gburd wants to merge 1 commit into
Open
Conversation
waiter::wake() unconditionally dereferences the thread pointer stored in a wait_record, and thread::wake_impl() dereferences the detached_state it is handed. Both run on the wake path with preemption disabled, so if either pointer is null a page fault trips assert(sched::preemptable()) in page_fault and aborts the whole instance instead of faulting recoverably. A wait_record left linked in a condvar/mutex queue can be stale: it may already have been woken (wake() stores null), or its backing thread may not be resolvable in the waker's address space, in which case the stored pointer reads as null or a tiny-integer remnant. A live sched::thread never lives in the first page of the address space, so such a value is unambiguously not a thread. Guard both paths: in waiter::wake() skip a thread pointer below 0x1000 (null or an obviously invalid low value), and in thread::wake_impl() return early on a null detached_state. The guard is deliberately narrow -- only clearly-bogus pointers are dropped -- so it never discards a legitimate wake. Behavior is unchanged for every valid waiter; the only effect is turning a fatal abort on a stale/torn record into a no-op wake.
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
1 participant
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.
The wake path dereferences two pointers without checking them, and it runs
with preemption disabled, so a null dereference there does not fault
recoverably -- it trips
assert(sched::preemptable())inpage_faultandaborts the whole instance.
waiter::wake()loads thesched::thread *out of await_recordandimmediately calls
wake_with_from_mutex()on it.thread::wake_impl()dereferences thedetached_state *it is handed(
trace_sched_wake(st->t)and the CAS loop onst->st).A
wait_recordleft linked in a condvar/mutex queue can be stale: it mayalready have been woken (
wake()stores null), or its backing thread may notbe resolvable in the waker's context, in which case the stored pointer reads
as null or a small-integer remnant. A live
sched::threadnever lives in thefirst page of the address space, so such a value is unambiguously not a thread.
This change guards both paths:
waiter::wake(), skip a thread pointer below0x1000(null or anobviously invalid low value);
thread::wake_impl(), return early on a nulldetached_state.The guard is deliberately narrow -- only clearly-bogus pointers are dropped --
so it never discards a legitimate wake. Behavior is unchanged for every valid
waiter; the only effect is turning a fatal abort on a stale/torn record into a
no-op wake, which is the correct outcome (there is nothing live to wake).
Two small files, no functional change on the happy path.