Skip to content

Commit 106db47

Browse files
authored
Fix subtle/weird UB in the multi threaded executor (#15309)
# Objective - The multithreaded executor has some weird UB related to stacked borrows and async blocks - See my explanation on discord https://discord.com/channels/691052431525675048/749335865876021248/1286359267921887232 - Closes #15296 (can this be used to close PRs?) ## Solution - Don't create a `&mut World` reference outside `async` blocks and then capture it, but instead directly create it inside the `async` blocks. This avoids it being captured, which has some weird requirement on its validity. ## Testing - Added a regression test
1 parent 55c84cc commit 106db47

File tree

1 file changed

+18
-3
lines changed

1 file changed

+18
-3
lines changed

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -613,9 +613,6 @@ impl ExecutorState {
613613
/// # Safety
614614
/// Caller must ensure no systems are currently borrowed.
615615
unsafe fn spawn_exclusive_system_task(&mut self, context: &Context, system_index: usize) {
616-
// SAFETY: `can_run` returned true for this system, which means
617-
// that no other systems currently have access to the world.
618-
let world = unsafe { context.environment.world_cell.world_mut() };
619616
// SAFETY: this system is not running, no other reference exists
620617
let system = unsafe { &mut *context.environment.systems[system_index].get() };
621618
// Move the full context object into the new future.
@@ -626,13 +623,19 @@ impl ExecutorState {
626623
let unapplied_systems = self.unapplied_systems.clone();
627624
self.unapplied_systems.clear();
628625
let task = async move {
626+
// SAFETY: `can_run` returned true for this system, which means
627+
// that no other systems currently have access to the world.
628+
let world = unsafe { context.environment.world_cell.world_mut() };
629629
let res = apply_deferred(&unapplied_systems, context.environment.systems, world);
630630
context.system_completed(system_index, res, system);
631631
};
632632

633633
context.scope.spawn_on_scope(task);
634634
} else {
635635
let task = async move {
636+
// SAFETY: `can_run` returned true for this system, which means
637+
// that no other systems currently have access to the world.
638+
let world = unsafe { context.environment.world_cell.world_mut() };
636639
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
637640
__rust_begin_short_backtrace::run(&mut **system, world);
638641
}));
@@ -783,4 +786,16 @@ mod tests {
783786
schedule.run(&mut world);
784787
assert!(world.get_resource::<R>().is_some());
785788
}
789+
790+
/// Regression test for a weird bug flagged by MIRI in
791+
/// `spawn_exclusive_system_task`, related to a `&mut World` being captured
792+
/// inside an `async` block and somehow remaining alive even after its last use.
793+
#[test]
794+
fn check_spawn_exclusive_system_task_miri() {
795+
let mut world = World::new();
796+
let mut schedule = Schedule::default();
797+
schedule.set_executor_kind(ExecutorKind::MultiThreaded);
798+
schedule.add_systems(((|_: Commands| {}), |_: Commands| {}).chain());
799+
schedule.run(&mut world);
800+
}
786801
}

0 commit comments

Comments
 (0)