From a9d2a9ea37d10b085244546ad24b7460ef98cbd5 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:23:12 -0400 Subject: [PATCH] Make QueryFilter an unsafe trait (#14790) # Objective It's possible to create UB using an implementation of `QueryFilter` that performs mutable access, but that does not violate any documented safety invariants. This code: ```rust #[derive(Component)] struct Foo(usize); // This derive is a simple way to get a valid WorldQuery impl. The QueryData impl isn't used. #[derive(QueryData)] #[query_data(mutable)] struct BadFilter<'w> { foo: &'w mut Foo, } impl QueryFilter for BadFilter<'_> { const IS_ARCHETYPAL: bool = false; unsafe fn filter_fetch( fetch: &mut Self::Fetch<'_>, entity: Entity, table_row: TableRow, ) -> bool { // SAFETY: fetch and filter_fetch have the same safety requirements let f: &mut usize = &mut unsafe { Self::fetch(fetch, entity, table_row) }.foo.0; println!("Got &mut at {f:p}"); true } } let mut world = World::new(); world.spawn(Foo(0)); world.run_system_once(|query: Query<&Foo, BadFilter>| { let f: &usize = &query.iter().next().unwrap().0; println!("Got & at {f:p}"); query.iter().next().unwrap(); println!("Still have & at {f:p}"); }); ``` prints: ``` Got &mut at 0x1924b92dfb0 Got & at 0x1924b92dfb0 Got &mut at 0x1924b92dfb0 Still have & at 0x1924b92dfb0 ``` Which means it had an `&` and `&mut` alive at the same time. The only `unsafe` there is around `Self::fetch`, but I believe that call correctly upholds the safety invariant, and matches what `Added` and `Changed` do. ## Solution Make `QueryFilter` an unsafe trait and document the requirement that the `WorldQuery` implementation be read-only. ## Migration Guide `QueryFilter` is now an `unsafe trait`. If you were manually implementing it, you will need to verify that the `WorldQuery` implementation is read-only and then add the `unsafe` keyword to the `impl`. --- crates/bevy_ecs/macros/src/query_filter.rs | 3 ++- crates/bevy_ecs/src/query/filter.rs | 25 ++++++++++++++++------ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/macros/src/query_filter.rs b/crates/bevy_ecs/macros/src/query_filter.rs index 2a2916905ae20..378e26df101a7 100644 --- a/crates/bevy_ecs/macros/src/query_filter.rs +++ b/crates/bevy_ecs/macros/src/query_filter.rs @@ -120,7 +120,8 @@ pub fn derive_query_filter_impl(input: TokenStream) -> TokenStream { ); let filter_impl = quote! { - impl #user_impl_generics #path::query::QueryFilter + // SAFETY: This only performs access that subqueries perform, and they impl `QueryFilter` and so perform no mutable access. + unsafe impl #user_impl_generics #path::query::QueryFilter for #struct_name #user_ty_generics #user_where_clauses { const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index affdaf2828b63..0385191053a65 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -70,12 +70,17 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// [`matches_component_set`]: Self::matches_component_set /// [`Query`]: crate::system::Query /// [`State`]: Self::State +/// +/// # Safety +/// +/// The [`WorldQuery`] implementation must not take any mutable access. +/// This is the same safety requirement as [`ReadOnlyQueryData`](crate::query::ReadOnlyQueryData). #[diagnostic::on_unimplemented( message = "`{Self}` is not a valid `Query` filter", label = "invalid `Query` filter", note = "a `QueryFilter` typically uses a combination of `With` and `Without` statements" )] -pub trait QueryFilter: WorldQuery { +pub unsafe trait QueryFilter: WorldQuery { /// Returns true if (and only if) this Filter relies strictly on archetypes to limit which /// components are accessed by the Query. /// @@ -201,7 +206,8 @@ unsafe impl WorldQuery for With { } } -impl QueryFilter for With { +// SAFETY: WorldQuery impl performs no access at all +unsafe impl QueryFilter for With { const IS_ARCHETYPAL: bool = true; #[inline(always)] @@ -311,7 +317,8 @@ unsafe impl WorldQuery for Without { } } -impl QueryFilter for Without { +// SAFETY: WorldQuery impl performs no access at all +unsafe impl QueryFilter for Without { const IS_ARCHETYPAL: bool = true; #[inline(always)] @@ -490,7 +497,8 @@ macro_rules! impl_or_query_filter { } } - impl<$($filter: QueryFilter),*> QueryFilter for Or<($($filter,)*)> { + // SAFETY: This only performs access that subqueries perform, and they impl `QueryFilter` and so perform no mutable access. + unsafe impl<$($filter: QueryFilter),*> QueryFilter for Or<($($filter,)*)> { const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*; #[inline(always)] @@ -512,7 +520,8 @@ macro_rules! impl_tuple_query_filter { #[allow(non_snake_case)] #[allow(clippy::unused_unit)] $(#[$meta])* - impl<$($name: QueryFilter),*> QueryFilter for ($($name,)*) { + // SAFETY: This only performs access that subqueries perform, and they impl `QueryFilter` and so perform no mutable access. + unsafe impl<$($name: QueryFilter),*> QueryFilter for ($($name,)*) { const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; #[inline(always)] @@ -734,7 +743,8 @@ unsafe impl WorldQuery for Added { } } -impl QueryFilter for Added { +// SAFETY: WorldQuery impl performs only read access on ticks +unsafe impl QueryFilter for Added { const IS_ARCHETYPAL: bool = false; #[inline(always)] unsafe fn filter_fetch( @@ -949,7 +959,8 @@ unsafe impl WorldQuery for Changed { } } -impl QueryFilter for Changed { +// SAFETY: WorldQuery impl performs only read access on ticks +unsafe impl QueryFilter for Changed { const IS_ARCHETYPAL: bool = false; #[inline(always)]