From bd5937648cd7e525ea8643dfb5ff154d30671211 Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Fri, 28 Oct 2022 18:04:03 -0700 Subject: [PATCH 1/2] Fix query.to_readonly().get_component_mut() soundness bug --- crates/bevy_ecs/src/system/mod.rs | 19 ++++++++++++++++--- crates/bevy_ecs/src/system/query.rs | 29 +++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 36837802c8ea8..e92a0e7b35574 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -116,8 +116,8 @@ mod tests { query::{Added, Changed, Or, With, Without}, schedule::{Schedule, Stage, SystemStage}, system::{ - Commands, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, RemovedComponents, - Res, ResMut, Resource, System, SystemState, + Commands, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, QueryComponentError, + RemovedComponents, Res, ResMut, Resource, System, SystemState, }, world::{FromWorld, World}, }; @@ -141,7 +141,7 @@ mod tests { #[derive(Component, Resource)] struct F; - #[derive(Component)] + #[derive(Component, Debug)] struct W(T); #[derive(StageLabel)] @@ -1163,4 +1163,17 @@ mod tests { } }); } + + #[test] + fn readonly_query_get_component_fails() { + let mut world = World::new(); + let entity = world.spawn(W(42u32)).id(); + run_system(&mut world, move |q: Query<&mut W>| { + let mut rq = q.to_readonly(); + assert_eq!( + QueryComponentError::MissingWriteAccess, + rq.get_component_mut::>(entity).unwrap_err(), + ); + }); + } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index fcd4039a7983d..9b5f56bbff19c 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -278,6 +278,12 @@ pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { pub(crate) state: &'state QueryState, pub(crate) last_change_tick: u32, pub(crate) change_tick: u32, + // SAFETY: This is used to ensure that `get_component_mut::` properly fails when a Query writes C + // and gets converted to a read-only query using `to_readonly`. Without checking this, `get_component_mut` relies on + // QueryState's archetype_component_access, which will continue allowing write access to C after being cast to + // the read-only variant. This whole situation is confusing and error prone. Ideally this is a temporary hack + // until we sort out a cleaner alternative. + pub(crate) force_read_only_component_access: bool, } impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> std::fmt::Debug for Query<'w, 's, Q, F> { @@ -301,6 +307,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { change_tick: u32, ) -> Self { Self { + force_read_only_component_access: false, world, state, last_change_tick, @@ -316,13 +323,14 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn to_readonly(&self) -> Query<'_, 's, Q::ReadOnly, F::ReadOnly> { let new_state = self.state.as_readonly(); // SAFETY: This is memory safe because it turns the query immutable. - unsafe { - Query::new( - self.world, - new_state, - self.last_change_tick, - self.change_tick, - ) + Query { + // SAFETY: this must be set to true or `get_component_mut` will be unsound. See the comments + // on this field for more details + force_read_only_component_access: true, + world: self.world, + state: new_state, + last_change_tick: self.last_change_tick, + change_tick: self.change_tick, } } @@ -1161,6 +1169,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { &self, entity: Entity, ) -> Result, QueryComponentError> { + // SAFETY: this check is required to ensure soundness in the case of `to_readonly().get_component_mut()` + // See the comments on the `force_read_only_component_access` field for more info. + if self.force_read_only_component_access { + return Err(QueryComponentError::MissingWriteAccess); + } let world = self.world; let entity_ref = world .get_entity(entity) @@ -1411,7 +1424,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> IntoIterator for &'w mut Quer } /// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`] -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub enum QueryComponentError { MissingReadAccess, MissingWriteAccess, From 8ac5789f7bade33fa62e0610bad7a374adc5311b Mon Sep 17 00:00:00 2001 From: Carter Anderson Date: Fri, 28 Oct 2022 18:16:15 -0700 Subject: [PATCH 2/2] fix test name --- crates/bevy_ecs/src/system/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index e92a0e7b35574..0dcc806c6d44a 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1165,7 +1165,7 @@ mod tests { } #[test] - fn readonly_query_get_component_fails() { + fn readonly_query_get_mut_component_fails() { let mut world = World::new(); let entity = world.spawn(W(42u32)).id(); run_system(&mut world, move |q: Query<&mut W>| {