Mirrors/bevy
2
0
Fork 0
mirror of https://github.com/bevyengine/bevy synced 2024-11-10 07:04:33 +00:00
Code Issues Projects Releases Packages Wiki Activity

Fix subtle/weird UB in the multi threaded executor (#15309)

Browse source 
# 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
This commit is contained in:
Giacomo Stevanato 2024-09-19 20:15:58 +02:00 committed by GitHub
parent 55c84cc722
commit 106db47f69
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 18 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
View file

@ -613,9 +613,6 @@ impl ExecutorState {
/// # Safety
/// Caller must ensure no systems are currently borrowed.
unsafe fn spawn_exclusive_system_task(&mut self, context: &Context, system_index: usize) {
// SAFETY: `can_run` returned true for this system, which means
// that no other systems currently have access to the world.
let world = unsafe { context.environment.world_cell.world_mut() };
// SAFETY: this system is not running, no other reference exists
let system = unsafe { &mut *context.environment.systems[system_index].get() };
// Move the full context object into the new future.
@ -626,6 +623,9 @@ impl ExecutorState {
let unapplied_systems = self.unapplied_systems.clone();
self.unapplied_systems.clear();
let task = async move {
// SAFETY: `can_run` returned true for this system, which means
// that no other systems currently have access to the world.
let world = unsafe { context.environment.world_cell.world_mut() };
let res = apply_deferred(&unapplied_systems, context.environment.systems, world);
context.system_completed(system_index, res, system);
};
@ -633,6 +633,9 @@ impl ExecutorState {
context.scope.spawn_on_scope(task);
} else {
let task = async move {
// SAFETY: `can_run` returned true for this system, which means
// that no other systems currently have access to the world.
let world = unsafe { context.environment.world_cell.world_mut() };
let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
__rust_begin_short_backtrace::run(&mut **system, world);
}));
@ -783,4 +786,16 @@ mod tests {
schedule.run(&mut world);
assert!(world.get_resource::<R>().is_some());
}
/// Regression test for a weird bug flagged by MIRI in
/// `spawn_exclusive_system_task`, related to a `&mut World` being captured
/// inside an `async` block and somehow remaining alive even after its last use.
#[test]
fn check_spawn_exclusive_system_task_miri() {
let mut world = World::new();
let mut schedule = Schedule::default();
schedule.set_executor_kind(ExecutorKind::MultiThreaded);
schedule.add_systems(((|_: Commands| {}), |_: Commands| {}).chain());
schedule.run(&mut world);
}
}