Fix unsound query transmutes on queries obtained from Query::as_readonly() (#17973)

# Objective

Fix unsound query transmutes on queries obtained from
`Query::as_readonly()`.

The following compiles, and the call to `transmute_lens()` should panic,
but does not:
```rust
fn bad_system(query: Query<&mut A>) {
    let mut readonly = query.as_readonly();
    let mut lens: QueryLens<&mut A> = readonly.transmute_lens();
    let other_readonly: Query<&A> = query.as_readonly();
    // `lens` and `other_readonly` alias, and are both alive here!
}
```

To make `Query::as_readonly()` zero-cost, we pointer-cast
`&QueryState<D, F>` to `&QueryState<D::ReadOnly, F>`. This means that
the `component_access` for a read-only query's state may include
accesses for the original mutable version, but the `Query` does not have
exclusive access to those components! `transmute` and `join` use that
access to ensure that a join is valid, and will incorrectly allow a
transmute that includes mutable access.

As a bonus, allow `Query::join`s that output `FilteredEntityRef` or
`FilteredEntityMut` to receive access from the `other` query. Currently
they only receive access from `self`.

## Solution

When transmuting or joining from a read-only query, remove any writes
before performing checking that the transmute is valid. For joins, be
sure to handle the case where one input query was the result of
`as_readonly()` but the other has valid mutable access.

This requires identifying read-only queries, so add a
`QueryData::IS_READ_ONLY` associated constant. Note that we only call
`QueryState::as_transmuted_state()` with `NewD: ReadOnlyQueryData`, so
checking for read-only queries is sufficient to check for
`as_transmuted_state()`.

Removing writes requires allocating a new `FilteredAccess`, so only do
so if the query is read-only and the state has writes. Otherwise, the
existing access is correct and we can continue using a reference to it.

Use the new read-only state to call `NewD::set_access`, so that
transmuting to a `FilteredAccessMut` results in a read-only
`FilteredAccessMut`. Otherwise, it would take the original write access,
and then the transmute would panic because it had too much access.

Note that `join` was previously passing `self.component_access` to
`NewD::set_access`. Switching it to `joined_component_access` also
allows a join that outputs `FilteredEntity(Ref|Mut)` to receive access
from `other`. The fact that it didn't do that before seems like an
oversight, so I didn't try to prevent that change.

## Testing

Added unit tests with the unsound transmute and join.

crates/bevy_ecs/macros/src/query_data.rs

@@ -254,6 +254,7 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream {
                 /// SAFETY: we assert fields are readonly below
                 unsafe impl #user_impl_generics #path::query::QueryData
                 for #read_only_struct_name #user_ty_generics #user_where_clauses {
+                    const IS_READ_ONLY: bool = true;
                     type ReadOnly = #read_only_struct_name #user_ty_generics;
                     type Item<'__w> = #read_only_item_struct_name #user_ty_generics_with_world;
 
@@ -284,10 +285,13 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream {
             quote! {}
         };
 
+        let is_read_only = !attributes.is_mutable;
+
         quote! {
             /// SAFETY: we assert fields are readonly below
             unsafe impl #user_impl_generics #path::query::QueryData
             for #struct_name #user_ty_generics #user_where_clauses {
+                const IS_READ_ONLY: bool = #is_read_only;
                 type ReadOnly = #read_only_struct_name #user_ty_generics;
                 type Item<'__w> = #item_struct_name #user_ty_generics_with_world;
 

crates/bevy_ecs/src/query/fetch.rs

@@ -265,8 +265,9 @@ use variadics_please::all_tuples;
 ///
 /// # Safety
 ///
-/// Component access of `Self::ReadOnly` must be a subset of `Self`
+/// - Component access of `Self::ReadOnly` must be a subset of `Self`
-/// and `Self::ReadOnly` must match exactly the same archetypes/tables as `Self`
+///   and `Self::ReadOnly` must match exactly the same archetypes/tables as `Self`
+/// - `IS_READ_ONLY` must be `true` if and only if `Self: ReadOnlyQueryData`
 ///
 /// [`Query`]: crate::system::Query
 /// [`ReadOnly`]: Self::ReadOnly
@@ -276,6 +277,9 @@ use variadics_please::all_tuples;
     note = "if `{Self}` is a component type, try using `&{Self}` or `&mut {Self}`"
 )]
 pub unsafe trait QueryData: WorldQuery {
+    /// True if this query is read-only and may not perform mutable access.
+    const IS_READ_ONLY: bool;
+
     /// The read-only variant of this [`QueryData`], which satisfies the [`ReadOnlyQueryData`] trait.
     type ReadOnly: ReadOnlyQueryData<State = <Self as WorldQuery>::State>;
 
@@ -367,6 +371,7 @@ unsafe impl WorldQuery for Entity {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl QueryData for Entity {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
 
     type Item<'w> = Entity;
@@ -443,6 +448,7 @@ unsafe impl WorldQuery for EntityLocation {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl QueryData for EntityLocation {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = EntityLocation;
 
@@ -524,6 +530,7 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<'a> QueryData for EntityRef<'a> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = EntityRef<'w>;
 
@@ -604,6 +611,7 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> {
 
 /// SAFETY: access of `EntityRef` is a subset of `EntityMut`
 unsafe impl<'a> QueryData for EntityMut<'a> {
+    const IS_READ_ONLY: bool = false;
     type ReadOnly = EntityRef<'a>;
     type Item<'w> = EntityMut<'w>;
 
@@ -696,6 +704,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<'a> QueryData for FilteredEntityRef<'a> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = FilteredEntityRef<'w>;
 
@@ -790,6 +799,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> {
 
 /// SAFETY: access of `FilteredEntityRef` is a subset of `FilteredEntityMut`
 unsafe impl<'a> QueryData for FilteredEntityMut<'a> {
+    const IS_READ_ONLY: bool = false;
     type ReadOnly = FilteredEntityRef<'a>;
     type Item<'w> = FilteredEntityMut<'w>;
 
@@ -888,6 +898,7 @@ unsafe impl<'a, B> QueryData for EntityRefExcept<'a, B>
 where
     B: Bundle,
 {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = EntityRefExcept<'w, B>;
 
@@ -988,6 +999,7 @@ unsafe impl<'a, B> QueryData for EntityMutExcept<'a, B>
 where
     B: Bundle,
 {
+    const IS_READ_ONLY: bool = false;
     type ReadOnly = EntityRefExcept<'a, B>;
     type Item<'w> = EntityMutExcept<'w, B>;
 
@@ -1060,6 +1072,7 @@ unsafe impl WorldQuery for &Archetype {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl QueryData for &Archetype {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = &'w Archetype;
 
@@ -1204,6 +1217,7 @@ unsafe impl<T: Component> WorldQuery for &T {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<T: Component> QueryData for &T {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = &'w T;
 
@@ -1375,6 +1389,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<'__w, T: Component> QueryData for Ref<'__w, T> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = Ref<'w, T>;
 
@@ -1569,6 +1584,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
 
 /// SAFETY: access of `&T` is a subset of `&mut T`
 unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for &'__w mut T {
+    const IS_READ_ONLY: bool = false;
     type ReadOnly = &'__w T;
     type Item<'w> = Mut<'w, T>;
 
@@ -1711,6 +1727,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Mut<'__w, T> {
 
 // SAFETY: access of `Ref<T>` is a subset of `Mut<T>`
 unsafe impl<'__w, T: Component<Mutability = Mutable>> QueryData for Mut<'__w, T> {
+    const IS_READ_ONLY: bool = false;
     type ReadOnly = Ref<'__w, T>;
     type Item<'w> = Mut<'w, T>;
 
@@ -1838,6 +1855,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
 
 // SAFETY: defers to soundness of `T: WorldQuery` impl
 unsafe impl<T: QueryData> QueryData for Option<T> {
+    const IS_READ_ONLY: bool = T::IS_READ_ONLY;
     type ReadOnly = Option<T::ReadOnly>;
     type Item<'w> = Option<T::Item<'w>>;
 
@@ -2001,6 +2019,7 @@ unsafe impl<T: Component> WorldQuery for Has<T> {
 
 /// SAFETY: `Self` is the same as `Self::ReadOnly`
 unsafe impl<T: Component> QueryData for Has<T> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = bool;
 
@@ -2049,6 +2068,7 @@ macro_rules! impl_tuple_query_data {
         $(#[$meta])*
         // SAFETY: defers to soundness `$name: WorldQuery` impl
         unsafe impl<$($name: QueryData),*> QueryData for ($($name,)*) {
+            const IS_READ_ONLY: bool = true $(&& $name::IS_READ_ONLY)*;
             type ReadOnly = ($($name::ReadOnly,)*);
             type Item<'w> = ($($name::Item<'w>,)*);
 
@@ -2211,6 +2231,7 @@ macro_rules! impl_anytuple_fetch {
         $(#[$meta])*
         // SAFETY: defers to soundness of `$name: WorldQuery` impl
         unsafe impl<$($name: QueryData),*> QueryData for AnyOf<($($name,)*)> {
+            const IS_READ_ONLY: bool = true $(&& $name::IS_READ_ONLY)*;
             type ReadOnly = AnyOf<($($name::ReadOnly,)*)>;
             type Item<'w> = ($(Option<$name::Item<'w>>,)*);
 
@@ -2315,6 +2336,7 @@ unsafe impl<D: QueryData> WorldQuery for NopWorldQuery<D> {
 
 /// SAFETY: `Self::ReadOnly` is `Self`
 unsafe impl<D: QueryData> QueryData for NopWorldQuery<D> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'w> = ();
 
@@ -2384,6 +2406,7 @@ unsafe impl<T: ?Sized> WorldQuery for PhantomData<T> {
 
 /// SAFETY: `Self::ReadOnly` is `Self`
 unsafe impl<T: ?Sized> QueryData for PhantomData<T> {
+    const IS_READ_ONLY: bool = true;
     type ReadOnly = Self;
     type Item<'a> = ();
 

crates/bevy_ecs/src/query/mod.rs

@@ -881,6 +881,7 @@ mod tests {
 
     /// SAFETY: `Self` is the same as `Self::ReadOnly`
     unsafe impl QueryData for ReadsRData {
+        const IS_READ_ONLY: bool = true;
         type ReadOnly = Self;
         type Item<'w> = ();
 

crates/bevy_ecs/src/query/state.rs

@@ -72,6 +72,9 @@ pub struct QueryState<D: QueryData, F: QueryFilter = ()> {
     pub(crate) matched_archetypes: FixedBitSet,
     /// [`FilteredAccess`] computed by combining the `D` and `F` access. Used to check which other queries
     /// this query can run in parallel with.
+    /// Note that because we do a zero-cost reference conversion in `Query::as_readonly`,
+    /// the access for a read-only query may include accesses for the original mutable version,
+    /// but the `Query` does not have exclusive access to those components.
     pub(crate) component_access: FilteredAccess<ComponentId>,
     // NOTE: we maintain both a bitset and a vec because iterating the vec is faster
     pub(super) matched_storage_ids: Vec<StorageId>,
@@ -132,7 +135,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
     /// `NewD` must have a subset of the access that `D` does and match the exact same archetypes/tables
     /// `NewF` must have a subset of the access that `F` does and match the exact same archetypes/tables
     pub(crate) unsafe fn as_transmuted_state<
-        NewD: QueryData<State = D::State>,
+        NewD: ReadOnlyQueryData<State = D::State>,
         NewF: QueryFilter<State = F::State>,
     >(
         &self,
@@ -750,7 +753,21 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         let mut fetch_state = NewD::get_state(world.components()).expect("Could not create fetch_state, Please initialize all referenced components before transmuting.");
         let filter_state = NewF::get_state(world.components()).expect("Could not create filter_state, Please initialize all referenced components before transmuting.");
 
-        NewD::set_access(&mut fetch_state, &self.component_access);
+        fn to_readonly(mut access: FilteredAccess<ComponentId>) -> FilteredAccess<ComponentId> {
+            access.access_mut().clear_writes();
+            access
+        }
+
+        let self_access = if D::IS_READ_ONLY && self.component_access.access().has_any_write() {
+            // The current state was transmuted from a mutable
+            // `QueryData` to a read-only one.
+            // Ignore any write access in the current state.
+            &to_readonly(self.component_access.clone())
+        } else {
+            &self.component_access
+        };
+
+        NewD::set_access(&mut fetch_state, self_access);
         NewD::update_component_access(&fetch_state, &mut component_access);
 
         let mut filter_component_access = FilteredAccess::default();
@@ -758,7 +775,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
 
         component_access.extend(&filter_component_access);
         assert!(
-            component_access.is_subset(&self.component_access),
+            component_access.is_subset(self_access),
             "Transmuted state for {} attempts to access terms that are not allowed by original state {}.",
             core::any::type_name::<(NewD, NewF)>(), core::any::type_name::<(D, F)>()
         );
@@ -840,7 +857,31 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
         let new_filter_state = NewF::get_state(world.components())
             .expect("Could not create filter_state, Please initialize all referenced components before transmuting.");
 
-        NewD::set_access(&mut new_fetch_state, &self.component_access);
+        let mut joined_component_access = self.component_access.clone();
+        joined_component_access.extend(&other.component_access);
+
+        if D::IS_READ_ONLY && self.component_access.access().has_any_write()
+            || OtherD::IS_READ_ONLY && other.component_access.access().has_any_write()
+        {
+            // One of the input states was transmuted from a mutable
+            // `QueryData` to a read-only one.
+            // Ignore any write access in that current state.
+            // The simplest way to do this is to clear *all* writes
+            // and then add back in any writes that are valid
+            joined_component_access.access_mut().clear_writes();
+            if !D::IS_READ_ONLY {
+                joined_component_access
+                    .access_mut()
+                    .extend(self.component_access.access());
+            }
+            if !OtherD::IS_READ_ONLY {
+                joined_component_access
+                    .access_mut()
+                    .extend(other.component_access.access());
+            }
+        }
+
+        NewD::set_access(&mut new_fetch_state, &joined_component_access);
         NewD::update_component_access(&new_fetch_state, &mut component_access);
 
         let mut new_filter_component_access = FilteredAccess::default();
@@ -848,9 +889,6 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
 
         component_access.extend(&new_filter_component_access);
 
-        let mut joined_component_access = self.component_access.clone();
-        joined_component_access.extend(&other.component_access);
-
         assert!(
             component_access.is_subset(&joined_component_access),
             "Joined state for {} attempts to access terms that are not allowed by state {} joined with {}.",
@@ -1759,8 +1797,11 @@ impl<D: QueryData, F: QueryFilter> From<QueryBuilder<'_, D, F>> for QueryState<D
 #[cfg(test)]
 mod tests {
     use crate::{
-        component::Component, entity_disabling::DefaultQueryFilters, prelude::*,
+        component::Component,
-        world::FilteredEntityRef,
+        entity_disabling::DefaultQueryFilters,
+        prelude::*,
+        system::{QueryLens, RunSystemOnce},
+        world::{FilteredEntityMut, FilteredEntityRef},
     };
 
     #[test]
@@ -2002,6 +2043,23 @@ mod tests {
         let _new_query = query.transmute_filtered::<Entity, Changed<B>>(&world);
     }
 
+    #[test]
+    #[should_panic(
+        expected = "Transmuted state for (&mut bevy_ecs::query::state::tests::A, ()) attempts to access terms that are not allowed by original state (&bevy_ecs::query::state::tests::A, ())."
+    )]
+    fn cannot_transmute_mutable_after_readonly() {
+        let mut world = World::new();
+        // Calling this method would mean we had aliasing queries.
+        fn bad(_: Query<&mut A>, _: Query<&A>) {}
+        world
+            .run_system_once(|query: Query<&mut A>| {
+                let mut readonly = query.as_readonly();
+                let mut lens: QueryLens<&mut A> = readonly.transmute_lens();
+                bad(lens.query(), query.as_readonly());
+            })
+            .unwrap();
+    }
+
     // Regression test for #14629
     #[test]
     #[should_panic]
@@ -2120,6 +2178,37 @@ mod tests {
         let _: QueryState<Entity, Changed<C>> = query_1.join_filtered(&world, &query_2);
     }
 
+    #[test]
+    #[should_panic(
+        expected = "Joined state for ((&mut bevy_ecs::query::state::tests::A, &mut bevy_ecs::query::state::tests::B), ()) attempts to access terms that are not allowed by state (&bevy_ecs::query::state::tests::A, ()) joined with (&mut bevy_ecs::query::state::tests::B, ())."
+    )]
+    fn cannot_join_mutable_after_readonly() {
+        let mut world = World::new();
+        // Calling this method would mean we had aliasing queries.
+        fn bad(_: Query<(&mut A, &mut B)>, _: Query<&A>) {}
+        world
+            .run_system_once(|query_a: Query<&mut A>, mut query_b: Query<&mut B>| {
+                let mut readonly = query_a.as_readonly();
+                let mut lens: QueryLens<(&mut A, &mut B)> = readonly.join(&mut query_b);
+                bad(lens.query(), query_a.as_readonly());
+            })
+            .unwrap();
+    }
+
+    #[test]
+    fn join_to_filtered_entity_mut() {
+        let mut world = World::new();
+        world.spawn((A(2), B(3)));
+
+        let query_1 = QueryState::<&mut A>::new(&mut world);
+        let query_2 = QueryState::<&mut B>::new(&mut world);
+        let mut new_query: QueryState<FilteredEntityMut> = query_1.join(&world, &query_2);
+
+        let mut entity = new_query.single_mut(&mut world).unwrap();
+        assert!(entity.get_mut::<A>().is_some());
+        assert!(entity.get_mut::<B>().is_some());
+    }
+
     #[test]
     fn query_respects_default_filters() {
         let mut world = World::new();

crates/bevy_ecs/src/system/query.rs

@@ -441,10 +441,13 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
     }
 
     /// Returns another `Query` from this does not return any data, which can be faster.
-    fn as_nop(&self) -> Query<'w, 's, NopWorldQuery<D>, F> {
+    fn as_nop(&self) -> Query<'_, 's, NopWorldQuery<D>, F> {
         let new_state = self.state.as_nop();
         // SAFETY:
-        // - This is memory safe because it performs no access.
+        // - The reborrowed query is converted to read-only, so it cannot perform mutable access,
+        //   and the original query is held with a shared borrow, so it cannot perform mutable access either.
+        //   Note that although `NopWorldQuery` itself performs *no* access and could soundly alias a mutable query,
+        //   it has the original `QueryState::component_access` and could be `transmute`d to a read-only query.
         // - The world matches because it was the same one used to construct self.
         unsafe { Query::new(self.world, new_state, self.last_run, self.this_run) }
     }

crates/bevy_render/src/sync_world.rs

@@ -358,6 +358,7 @@ mod render_entities_world_query_impls {
     // SAFETY: Component access of Self::ReadOnly is a subset of Self.
     // Self::ReadOnly matches exactly the same archetypes/tables as Self.
     unsafe impl QueryData for RenderEntity {
+        const IS_READ_ONLY: bool = true;
         type ReadOnly = RenderEntity;
         type Item<'w> = Entity;
 
@@ -457,6 +458,7 @@ mod render_entities_world_query_impls {
     // SAFETY: Component access of Self::ReadOnly is a subset of Self.
     // Self::ReadOnly matches exactly the same archetypes/tables as Self.
     unsafe impl QueryData for MainEntity {
+        const IS_READ_ONLY: bool = true;
         type ReadOnly = MainEntity;
         type Item<'w> = Entity;