Fix query.to_readonly().get_component_mut() soundness bug (#6401)

# Objective

Fix the soundness issue outlined in #5866. In short the problem is that `query.to_readonly().get_component_mut::<T>()` can provide unsound mutable access to the component. This PR is an alternative to just removing the offending api. Given that `to_readonly` is a useful tool, I think this approach is a preferable short term solution. Long term I think theres a better solution out there, but we can find that on its own time.

## Solution

Add what amounts to a "dirty flag" that marks Queries that have been converted to their read-only variant via `to_readonly` as dirty. When this flag is set to true, `get_component_mut` will fail with an error, preventing the unsound access.

Author: cart

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>(T);
 
     #[derive(StageLabel)]
@@ -1163,4 +1163,17 @@ mod tests {
             }
         });
     }
+
+    #[test]
+    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<u32>>| {
+            let mut rq = q.to_readonly();
+            assert_eq!(
+                QueryComponentError::MissingWriteAccess,
+                rq.get_component_mut::<W<u32>>(entity).unwrap_err(),
+            );
+        });
+    }
 }

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<Q, F>,
     pub(crate) last_change_tick: u32,
     pub(crate) change_tick: u32,
+    // SAFETY: This is used to ensure that `get_component_mut::<C>` 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<Mut<'_, T>, 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,