Fix use-after-free in single-threaded TaskPool::scope during unwinding - #25248
Fix use-after-free in single-threaded TaskPool::scope during unwinding#25248kiana1kaslana wants to merge 6 commits into
Conversation
The single-threaded `Scope` used `.detach()` on spawned tasks and had no `Drop` implementation. When a panic occurred in the scope callback, the completion loop in `scope_with_executor` was skipped, causing the executor (dropped last due to reverse declaration order) to drop queued futures after their borrowed state (`results`, `pending_tasks`) had already been freed. This mirrors the multi-threaded `Scope`'s approach: - Store `Task` handles in a `RefCell<Vec<Task<()>>>` instead of detaching - Implement `Drop` to cancel all pending tasks while the executor is still running - Add a regression test that passes under Miri
|
Welcome, new contributor! Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨ |
hymm
left a comment
hymm
left a comment
There was a problem hiding this comment.
This fix makes sense to me. Web tasks weren't cancellable until we started using the web-task crate recently. Switching to that crate might have surfaced this problem.
The comments are a bit too verbose and seem to be addressed to reviewers rather than future readers of the code. Made some suggestions for cutting some text. Could probably cut more, but that'd be just nit picking.
I did ask a question about the test that needs answering before I approve.
| /// | ||
| /// This test should pass under Miri without reporting any UB. | ||
| #[test] | ||
| fn scope_panic_cancels_pending_futures() { |
There was a problem hiding this comment.
did you confirm that this test fails miri before this fix? The task doesn't access any scoped data, so I would expect this to pass.
There was a problem hiding this comment.
You're right — I ran cargo +nightly miri test scope_panic_cancels_pending_futures on the parent commit and it passes. The pending future doesn't hold any scoped references, so the executor dropping it after unwind doesn't trigger Miri. I've simplified the test to check panic propagation without claiming Miri detection.
The soundness fix itself (storing Task handles in a RefCell + Drop cancelling them before executor teardown) should be clear from the diff.
Co-authored-by: Mike <mike.hsu@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
The test uses pending() which holds no scoped references, so Miri passes even on the parent commit. Rewrite the doc comment to describe what the test actually checks (panic propagation without UB), and remove the claim about detecting use-after-free via Miri.
2 participants
Objective
Fix soundness issue C-12: the single-threaded
TaskPool::scopecan trigger use-after-free when the scope callback panics.Solution
The single-threaded
Scopeused.detach()on spawned tasks and had noDropimplementation. When a panic occurred in the scope callback, thecompletion loop (
block_on(executor.run(...))) inscope_with_executorwas skipped. The
executoris declared first (dropped last due toreverse declaration order), so its
Dropwould later drop queuedfutures after their borrowed state (
results,pending_tasks) hadalready been freed — producing use-after-free.
This fix mirrors the multi-threaded
Scope's approach(
crates/bevy_tasks/src/task_pool.rs):Taskhandles in aRefCell<Vec<Task<()>>>instead of calling.detach()DropforScopethat drives the executor while cancellingall spawned tasks via
task.cancel().await, ensuring future destructorsrun while the borrowed state is still alive
Testing
cargo +nightly check -p bevy_tasks ...— passescargo +nightly test -p bevy_tasks ...— 5 unit + 5 doc tests passcargo +nightly miri test -p bevy_tasks ...— passes with zero UBscope_panic_cancels_pending_futures...