From 328347f44c4088f72039128c0be5e61ed6c36001 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 17 Apr 2023 11:20:42 -0400 Subject: [PATCH] Add a missing safety invariant to `System::run_unsafe` (#7778) # Objective The implementation of `System::run_unsafe` for `FunctionSystem` requires that the world is the same one used to initialize the system. However, the `System` trait has no requirements that the world actually matches, which makes this implementation unsound. This was previously mentioned in https://github.com/bevyengine/bevy/pull/7605#issuecomment-1426491871 Fixes part of #7833. ## Solution Add the safety invariant that `System::update_archetype_component_access` must be called prior to `System::run_unsafe`. Since `FunctionSystem::update_archetype_component_access` properly validates the world, this ensures that `run_unsafe` is not called with a mismatched world. Most exclusive systems are not required to be run on the same world that they are initialized with, so this is not a concern for them. Systems formed by combining an exclusive system with a regular system *do* require the world to match, however the validation is done inside of `System::run` when needed. --- .../src/schedule/executor/multi_threaded.rs | 32 +++++++++++++++---- crates/bevy_ecs/src/system/combinator.rs | 2 ++ .../src/system/exclusive_function_system.rs | 5 +-- crates/bevy_ecs/src/system/function_system.rs | 9 +++--- crates/bevy_ecs/src/system/system.rs | 8 ++++- 5 files changed, 40 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index a00d328c843aa..dbf8386a8145c 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -320,6 +320,8 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); + // SAFETY: Since `self.can_run` returned true earlier, it must have called + // `update_archetype_component_access` for each run condition. if !self.should_run(system_index, system, conditions, world) { self.skip_system_and_signal_dependents(system_index); continue; @@ -338,7 +340,9 @@ impl MultiThreadedExecutor { break; } - // SAFETY: No other reference to this system exists. + // SAFETY: + // - No other reference to this system exists. + // - `self.can_run` has been called, which calls `update_archetype_component_access` with this system. unsafe { self.spawn_system_task(scope, system_index, systems, world); } @@ -408,7 +412,11 @@ impl MultiThreadedExecutor { true } - fn should_run( + /// # Safety + /// + /// `update_archetype_component` must have been called with `world` + /// for each run condition in `conditions`. + unsafe fn should_run( &mut self, system_index: usize, _system: &BoxedSystem, @@ -421,7 +429,8 @@ impl MultiThreadedExecutor { continue; } - // evaluate system set's conditions + // Evaluate the system set's conditions. + // SAFETY: `update_archetype_component_access` has been called for each run condition. let set_conditions_met = evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world); @@ -434,7 +443,8 @@ impl MultiThreadedExecutor { self.evaluated_sets.insert(set_idx); } - // evaluate system's conditions + // Evaluate the system's conditions. + // SAFETY: `update_archetype_component_access` has been called for each run condition. let system_conditions_met = evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world); @@ -448,7 +458,9 @@ impl MultiThreadedExecutor { } /// # Safety - /// Caller must not alias systems that are running. + /// - Caller must not alias systems that are running. + /// - `update_archetype_component_access` must have been called with `world` + /// on the system assocaited with `system_index`. unsafe fn spawn_system_task<'scope>( &mut self, scope: &Scope<'_, 'scope, ()>, @@ -470,7 +482,9 @@ impl MultiThreadedExecutor { #[cfg(feature = "trace")] let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - // SAFETY: access is compatible + // SAFETY: + // - Access: TODO. + // - `update_archetype_component_access` has been called. unsafe { system.run_unsafe((), world) }; })); #[cfg(feature = "trace")] @@ -673,7 +687,11 @@ fn apply_system_buffers( Ok(()) } -fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { +/// # Safety +/// +/// `update_archetype_component_access` must have been called +/// with `world` for each condition in `conditions`. +unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { // not short-circuiting is intentional #[allow(clippy::unnecessary_fold)] conditions diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 671a29431c883..e9b002e4175dc 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -164,6 +164,8 @@ where // so the caller will guarantee that no other systems will conflict with `a` or `b`. // Since these closures are `!Send + !Sync + !'static`, they can never be called // in parallel, so their world accesses will not conflict with each other. + // Additionally, `update_archetype_component_access` has been called, + // which forwards to the implementations for `self.a` and `self.b`. |input| self.a.run_unsafe(input, world), |input| self.b.run_unsafe(input, world), ) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 1469a903e90dd..c1c4bff3a2a86 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -6,7 +6,7 @@ use crate::{ check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, IntoSystem, System, SystemMeta, }, - world::{World, WorldId}, + world::World, }; use bevy_utils::all_tuples; @@ -25,7 +25,6 @@ where func: F, param_state: Option<::State>, system_meta: SystemMeta, - world_id: Option, // NOTE: PhantomData T> gives this safe Send/Sync impls marker: PhantomData Marker>, } @@ -43,7 +42,6 @@ where func, param_state: None, system_meta: SystemMeta::new::(), - world_id: None, marker: PhantomData, } } @@ -132,7 +130,6 @@ where #[inline] fn initialize(&mut self, world: &mut World) { - self.world_id = Some(world.id()); self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); self.param_state = Some(F::Param::init(world, &mut self.system_meta)); } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index a56e5d85fe119..e0401d4b72f22 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -479,10 +479,11 @@ where unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { let change_tick = world.increment_change_tick(); - // Safety: - // We update the archetype component access correctly based on `Param`'s requirements - // in `update_archetype_component_access`. - // Our caller upholds the requirements. + // SAFETY: + // - The caller has invoked `update_archetype_component_access`, which will panic + // if the world does not match. + // - All world accesses used by `F::Param` have been registered, so the caller + // will ensure that there are no data access conflicts. let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index b8c11b6262677..ae4a9fc074033 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -49,11 +49,17 @@ pub trait System: Send + Sync + 'static { /// 1. This system is the only system running on the given world across all threads. /// 2. This system only runs in parallel with other systems that do not conflict with the /// [`System::archetype_component_access()`]. + /// + /// Additionally, the method [`Self::update_archetype_component_access`] must be called at some + /// point before this one, with the same exact [`World`]. If `update_archetype_component_access` + /// panics (or otherwise does not return for any reason), this method must not be called. unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { self.update_archetype_component_access(world); - // SAFETY: world and resources are exclusively borrowed + // SAFETY: + // - World and resources are exclusively borrowed, which ensures no data access conflicts. + // - `update_archetype_component_access` has been called. unsafe { self.run_unsafe(input, world) } } fn apply_buffers(&mut self, world: &mut World);