Fix use-after-free race on future->monitor in error paths#381
Merged
Conversation
When a task threw (normal exception) or crashed (IllegalInstruction), the worker signalled the future twice: first PHP_PARALLEL_ERROR (or READY|ERROR), then an unconditional trailing PHP_PARALLEL_READY at the end of php_parallel_scheduler_run(). Future::value() waits on READY|FAILURE|ERROR, so the first signal already releases the consumer. The consumer can then tear the future down (freeing future->monitor) before the worker runs the trailing php_parallel_monitor_set(future->monitor, PHP_PARALLEL_READY), which then dereferences a dangling monitor inside pthread_cond_signal -> access violation / use-after-free. This was observed as a hard crash on Windows (0xC0000005) in tests/functional/007.phpt, masked until now by the catch-all VEH; on Linux the freed memory happened to stay readable. Fix: the error paths now signal READY|ERROR exactly once and skip the trailing READY (via a 'ready' flag). The success and kill paths are unchanged - they have no early wake and still rely on the trailing READY. Setting READY in the error path also ensures the future destructor (which waits for READY) never blocks when value() is not called.
realFlowControl
marked this pull request as ready for review
June 16, 2026 11:01
2 tasks
future->monitor in error paths
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.
Root cause of the Windows worker-thread teardown access violation found via the diagnostic in #380.
The bug
When a task threw a normal exception (or crashed into the
IllegalInstructionpath), the worker signalled the future twice inphp_parallel_scheduler_run():PHP_PARALLEL_ERROR(resp.READY|ERROR) when saving the task error, thenphp_parallel_monitor_set(future->monitor, PHP_PARALLEL_READY)at the end of the function.Future::value()waits onREADY|FAILURE|ERROR, so the first signal already releases the consumer. The consumer can then tear the future down — freeingfuture->monitor— before the worker reaches the second signal, which then dereferences a dangling monitor insidepthread_cond_signal→ use-after-free.On Windows this is a hard
0xC0000005(caught here via a diagnostic backtrace):It surfaced in
tests/functional/007.phpt(bootstrap set inside a task). On Linux the freed memory stayed readable, so it went unnoticed; on Windows it was masked by the catch-all VEH from #355.The fix
The error paths now signal
READY|ERRORexactly once and skip the trailingREADY(guarded by areadyflag). Once that singlemonitor_setreturns, the worker never touches the future again, closing the window. SettingREADYin the error path also keeps the future destructor (which waits forREADY) from blocking whenvalue()is never called.The success and kill paths are unchanged: they have no early wake and still rely on the trailing
READY.Testing
functional/007passing for the right reason (no AV).Related
007will then pass instead of being masked.