From 3487f0df62ebe17ad3d96769e22f4b52a7644325 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 16 Aug 2023 20:38:26 -0400 Subject: [PATCH 1/7] make `Query::is_empty` safe --- crates/bevy_ecs/src/query/state.rs | 17 +++++++++++++++-- crates/bevy_ecs/src/system/query.rs | 8 ++------ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index ef01805937ad8..8cb72553eb0cd 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -130,10 +130,23 @@ impl QueryState { /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + self.is_empty_unsafe_world_cell(world.as_unsafe_world_cell_readonly(), last_run, this_run) + } + + /// Checks if the query is empty for the given [`UnsafeWorldCell`]. + #[inline] + pub(crate) fn is_empty_unsafe_world_cell( + &self, + world: UnsafeWorldCell, + last_run: Tick, + this_run: Tick, + ) -> bool { + // SAFETY: + // - `as_nop()` ensures no world data is accessed. + // - `&self` ensures no one has exclusive access. unsafe { self.as_nop() - .iter_unchecked_manual(world.as_unsafe_world_cell_readonly(), last_run, this_run) + .iter_unchecked_manual(world, last_run, this_run) .next() .is_none() } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 2cf3273fa7ed9..8ed359ffe73d7 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1366,12 +1366,8 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn is_empty(&self) -> bool { - self.state.is_empty( - // SAFETY: `QueryState::is_empty` does not access world data. - unsafe { self.world.unsafe_world() }, - self.last_run, - self.this_run, - ) + self.state + .is_empty_unsafe_world_cell(self.world, self.last_run, self.this_run) } /// Returns `true` if the given [`Entity`] matches the query. From fc5778ef26ab62832e4835ac6aa78c608c0407d6 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 16 Aug 2023 20:47:20 -0400 Subject: [PATCH 2/7] validate the world in `is_empty` --- crates/bevy_ecs/src/query/state.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 8cb72553eb0cd..8d280db81e631 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -128,6 +128,10 @@ impl QueryState { } /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. + /// + /// # Panics + /// + /// If `world` does not match the one used to call `QueryState::new` for this instance. #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { self.is_empty_unsafe_world_cell(world.as_unsafe_world_cell_readonly(), last_run, this_run) @@ -141,9 +145,14 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> bool { + // Since this method never accesses any world data, it is not unsound + // to call this with a mismatched world. It is, however, definitely a bug to do so. + #[cfg(debug_assertions)] + self.validate_world(world.id()); // SAFETY: // - `as_nop()` ensures no world data is accessed. // - `&self` ensures no one has exclusive access. + // - Since world data is never accessed, the world does not need to match. unsafe { self.as_nop() .iter_unchecked_manual(world, last_run, this_run) From eb484bca185cc0f50dc7cdcaccd9a4c26ca41ff2 Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 19 Aug 2023 12:35:04 -0400 Subject: [PATCH 3/7] fix unsoundness --- crates/bevy_ecs/src/query/state.rs | 22 +++++++++++++++------- crates/bevy_ecs/src/system/query.rs | 7 +++++-- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 8d280db81e631..adf172972920e 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -134,25 +134,33 @@ impl QueryState { /// If `world` does not match the one used to call `QueryState::new` for this instance. #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { - self.is_empty_unsafe_world_cell(world.as_unsafe_world_cell_readonly(), last_run, this_run) + self.validate_world(world.id()); + // SAFETY: The world has been validated. + unsafe { + self.is_empty_unsafe_world_cell( + world.as_unsafe_world_cell_readonly(), + last_run, + this_run, + ) + } } /// Checks if the query is empty for the given [`UnsafeWorldCell`]. + /// + /// # Safety + /// + /// `world` must match the one used to create this [`QuerySate`]. #[inline] - pub(crate) fn is_empty_unsafe_world_cell( + pub(crate) unsafe fn is_empty_unsafe_world_cell( &self, world: UnsafeWorldCell, last_run: Tick, this_run: Tick, ) -> bool { - // Since this method never accesses any world data, it is not unsound - // to call this with a mismatched world. It is, however, definitely a bug to do so. - #[cfg(debug_assertions)] - self.validate_world(world.id()); // SAFETY: // - `as_nop()` ensures no world data is accessed. // - `&self` ensures no one has exclusive access. - // - Since world data is never accessed, the world does not need to match. + // - Caller ensures that the world matches. unsafe { self.as_nop() .iter_unchecked_manual(world, last_run, this_run) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 8ed359ffe73d7..4d43d6187d962 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1366,8 +1366,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn is_empty(&self) -> bool { - self.state - .is_empty_unsafe_world_cell(self.world, self.last_run, self.this_run) + // SAFETY: `self.world` matches `self.state`. + unsafe { + self.state + .is_empty_unsafe_world_cell(self.world, self.last_run, self.this_run) + } } /// Returns `true` if the given [`Entity`] matches the query. From 8b137a1fc6ee52b08b550e0d099e7c8582a2206b Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 19 Aug 2023 15:27:39 -0400 Subject: [PATCH 4/7] Update crates/bevy_ecs/src/query/state.rs --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index adf172972920e..76b00093314fc 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -149,7 +149,7 @@ impl QueryState { /// /// # Safety /// - /// `world` must match the one used to create this [`QuerySate`]. + /// `world` must match the one used to create this [`QueryState`]. #[inline] pub(crate) unsafe fn is_empty_unsafe_world_cell( &self, From df04e64154b8f61933279047ffa2b037bb355407 Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 21 Aug 2023 11:04:14 -0400 Subject: [PATCH 5/7] correct safety invariants --- crates/bevy_ecs/src/query/state.rs | 12 +++++++----- crates/bevy_ecs/src/system/query.rs | 5 ++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 76b00093314fc..769f86062d358 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -135,7 +135,9 @@ impl QueryState { #[inline] pub fn is_empty(&self, world: &World, last_run: Tick, this_run: Tick) -> bool { self.validate_world(world.id()); - // SAFETY: The world has been validated. + // SAFETY: + // - We have read-only access to the entire world. + // - The world has been validated. unsafe { self.is_empty_unsafe_world_cell( world.as_unsafe_world_cell_readonly(), @@ -149,7 +151,8 @@ impl QueryState { /// /// # Safety /// - /// `world` must match the one used to create this [`QueryState`]. + /// - `world` mut have permission to ready any components required by this instance's `F` [`WorldQuery`]. + /// - `world` must match the one used to create this [`QueryState`]. #[inline] pub(crate) unsafe fn is_empty_unsafe_world_cell( &self, @@ -158,9 +161,8 @@ impl QueryState { this_run: Tick, ) -> bool { // SAFETY: - // - `as_nop()` ensures no world data is accessed. - // - `&self` ensures no one has exclusive access. - // - Caller ensures that the world matches. + // - The caller ensures that `world` has permission to access any data used by the filter. + // - The caller ensures that the world matches. unsafe { self.as_nop() .iter_unchecked_manual(world, last_run, this_run) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 4d43d6187d962..9431d2c39903e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1366,7 +1366,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn is_empty(&self) -> bool { - // SAFETY: `self.world` matches `self.state`. + // SAFETY: + // - `self.world` has permission to read any data required by the WorldQuery. + // - `&self` ensures that no one currently has write access. + // - `self.world` matches `self.state`. unsafe { self.state .is_empty_unsafe_world_cell(self.world, self.last_run, self.this_run) From 46fe33b80448c6db2f636fd25e3548b8503903ca Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 22 Aug 2023 00:12:06 -0400 Subject: [PATCH 6/7] Update crates/bevy_ecs/src/query/state.rs --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 769f86062d358..7f42add1d0e8c 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -151,7 +151,7 @@ impl QueryState { /// /// # Safety /// - /// - `world` mut have permission to ready any components required by this instance's `F` [`WorldQuery`]. + /// - `world` must have permission to ready any components required by this instance's `F` [`WorldQuery`]. /// - `world` must match the one used to create this [`QueryState`]. #[inline] pub(crate) unsafe fn is_empty_unsafe_world_cell( From 8988857e1021000459881e9bf65d8aeb6dfcd57b Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 2 Sep 2023 17:21:48 -0400 Subject: [PATCH 7/7] Update crates/bevy_ecs/src/query/state.rs --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 7f42add1d0e8c..482c48dcf4d08 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -151,7 +151,7 @@ impl QueryState { /// /// # Safety /// - /// - `world` must have permission to ready any components required by this instance's `F` [`WorldQuery`]. + /// - `world` must have permission to read any components required by this instance's `F` [`WorldQuery`]. /// - `world` must match the one used to create this [`QueryState`]. #[inline] pub(crate) unsafe fn is_empty_unsafe_world_cell(