Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Fix unsoundness with Or/AnyOf/Option component access' #4659

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Fix unsoundness with Or/AnyOf/Option component access' #4659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44aa3aa
yeet
BoxyUwU May 4, 2022
6d5fe30
filtered access docs
BoxyUwU May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions crates/bevy_ecs/src/query/access.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,23 @@ impl<T: SparseSetIndex> Access<T> {
/// An [`Access`] that has been filtered to include and exclude certain combinations of elements.
///
/// Used internally to statically check if queries are disjoint.
///
/// Subtle: a `read` or `write` in `access` should not be considered to imply a
/// `with` access.
///
/// For example consider `Query<Option<&T>>` this only has a `read` of `T` as doing
/// otherwise would allow for queriess to be considered disjoint that actually arent:
/// - `Query<(&mut T, Option<&U>)>` read/write `T`, read `U`, with `U`
/// - `Query<&mut T, Without<U>>` read/write `T`, without `U`
/// from this we could reasonably conclude that the queries are disjoint but they aren't.
///
/// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has
/// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following
/// queries would be incorrectly considered disjoint:
/// - `Query<&mut T>` read/write `T`
/// - `Query<Option<&T>` accesses nothing
///
/// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information.
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct FilteredAccess<T: SparseSetIndex> {
access: Access<T>,
Expand Down Expand Up @@ -210,6 +227,15 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
self.without.insert(index.sparse_set_index());
}

pub fn extend_intersect_filter(&mut self, other: &FilteredAccess<T>) {
self.without.intersect_with(&other.without);
self.with.intersect_with(&other.with);
}

pub fn extend_access(&mut self, other: &FilteredAccess<T>) {
self.access.extend(&other.access);
}

/// Returns `true` if this and `other` can be active at the same time.
pub fn is_compatible(&self, other: &FilteredAccess<T>) -> bool {
if self.access.is_compatible(&other.access) {
Expand Down
37 changes: 35 additions & 2 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,13 @@ unsafe impl<T: FetchState> FetchState for OptionState<T> {
}

fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
self.state.update_component_access(access);
// We don't want to add the `with`/`without` of `T` as `Option<T>` will match things regardless of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring filters here is safe because we're "increasing what we don't allow", but I think its important to call out that this makes us stricter than we theoretically need to be:

#[test]
fn this_is_now_too_strict_and_fails_when_it_doesnt_need_to() {
    fn sys(
        x: Query<(&mut A, Option<(&mut C, With<B>)>)>,
        y: Query<&mut C, (Without<B>, With<A>)>,
    ) {
    }
    let mut world = World::default();
    run_system(&mut world, sys);
}

Query x writes A (all archetypes) and only writes Cs on archetypes that have A and B. Query y only writes Cs on archetypes without Bs and with As. This means the C writes are disjoint. But because we're ignoring filter information we don't allow this.

I do think this particular case is pretty niche and solving soundness takes priority. But if we agree that this is "overly strict" its worth recording as a comment here. I haven't yet thought about solutions to this problem, but I suspect that our "flattened filtered accesses" might prevent this level of granularity? Idk if moving to a more complicated model is worth the complexity / computation costs. Definitely don't need to solve this problem in this pr (at least in the context of Option ... about to start looking at AnyOf/Or).

This also kind of breaks my brain, so lemme know if i miscalculated :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea you're right that Option<(&mut C, With<B>)> isnt handled as efficiently as possible (slash the current impl is overly restrictive), I'm not sure exactly how we'd get this to get work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the analysis here. I have ideas for a more grounded refactor of this space; I'll see if I can tackle it there.

Definitely don't block on this.

// `T`'s filters. for example `Query<(Option<&U>, &mut V)>` will match every entity with a `V` component
// regardless of whether it has a `U` component. If we dont do this the query will not conflict with
// `Query<&mut V, Without<U>>` which would be unsound.
let mut intermediate = access.clone();
self.state.update_component_access(&mut intermediate);
access.extend_access(&intermediate);
}

fn update_archetype_component_access(
Expand Down Expand Up @@ -1660,7 +1666,34 @@ macro_rules! impl_anytuple_fetch {

fn update_component_access(&self, _access: &mut FilteredAccess<ComponentId>) {
let ($($name,)*) = &self.0;
$($name.update_component_access(_access);)*

// We do not unconditionally add `$name`'s `with`/`without` accesses to `_access`
// as this would be unsound. For example the following two queries should conflict:
// - Query<(AnyOf<(&A, ())>, &mut B)>
// - Query<&mut B, Without<A>>
//
// If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>`
// would have a `With<A>` access which is incorrect as this `WorldQuery` will match entities that
// do not have the `A` component. This is the same logic as the `Or<...>: WorldQuery` impl.
//
// The correct thing to do here is to only add a `with`/`without` access to `_access` if all
// `$name` params have that `with`/`without` access. More jargony put- we add the intersection
// of all `with`/`without` accesses of the `$name` params to `_access`.
let mut _intersected_access = _access.clone();
let mut _not_first = false;
$(
if _not_first {
let mut intermediate = _access.clone();
$name.update_component_access(&mut intermediate);
_intersected_access.extend_intersect_filter(&intermediate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up until this point we've been recording "filtered access" reads as a combination of "with" and "reads_and_writes" entries for a given component id (the add_read api adds both). This effectively decouples the two and pretends something can "read" an id while simultaneously not having a corresponding "with" for the id.

Given that this whole system was built with that behavior in mind, changing the assumption feels risky. I haven't fully explored the thought yet, just curious if you've run through all of the scenarios here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the existence of with / without is currently only used to allow things to not conflict that normally would. If that is completely true, then the only risk is making more things conflict unnecessarily (so this isn't a soundness risk). Playing around with scenarios now to convince myself this is ok.

Copy link
Member

@cart cart May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm reasonably convinced this won't break anything. Although this is "hackey" enough that I think its worth expanding on how and why it violates the reads/with coupling. We should probably also add a note to FilteredAccess that "read" should not be assumed to imply "with" in some cases.

_intersected_access.extend_access(&intermediate);
} else {
$name.update_component_access(&mut _intersected_access);
_not_first = true;
}
)*

*_access = _intersected_access;
}

fn update_archetype_component_access(&self, _archetype: &Archetype, _access: &mut Access<ArchetypeComponentId>) {
Expand Down
29 changes: 28 additions & 1 deletion crates/bevy_ecs/src/query/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,34 @@ macro_rules! impl_query_filter_tuple {

fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
let ($($filter,)*) = &self.0;
$($filter.update_component_access(access);)*

// We do not unconditionally add `$filter`'s `with`/`without` accesses to `access`
// as this would be unsound. For example the following two queries should conflict:
// - Query<&mut B, Or<(With<A>, ())>>
// - Query<&mut B, Without<A>>
//
// If we were to unconditionally add `$name`'s `with`/`without` accesses then `Or<(With<A>, ())>`
// would have a `With<A>` access which is incorrect as this `WorldQuery` will match entities that
// do not have the `A` component. This is the same logic as the `AnyOf<...>: WorldQuery` impl.
//
// The correct thing to do here is to only add a `with`/`without` access to `_access` if all
// `$filter` params have that `with`/`without` access. More jargony put- we add the intersection
// of all `with`/`without` accesses of the `$filter` params to `access`.
let mut _intersected_access = access.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good way to deduplicate this code? I'd like to try and avoid having to manually maintain two copies of soundness critical code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not in this PR pls, in general tho AnyOf and Or are basically duplicate code 🤔 it'd be nice to deduplicate them (I think with some of the ptrification refactorings to remove FilterFetch we actually can get quite close to this)

let mut _not_first = false;
$(
if _not_first {
let mut intermediate = access.clone();
$filter.update_component_access(&mut intermediate);
_intersected_access.extend_intersect_filter(&intermediate);
_intersected_access.extend_access(&intermediate);
} else {
$filter.update_component_access(&mut _intersected_access);
_not_first = true;
}
)*

*access = _intersected_access;
}

fn update_archetype_component_access(&self, archetype: &Archetype, access: &mut Access<ArchetypeComponentId>) {
Expand Down
60 changes: 60 additions & 0 deletions crates/bevy_ecs/src/system/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ mod tests {
bundle::Bundles,
component::{Component, Components},
entity::{Entities, Entity},
prelude::AnyOf,
query::{Added, Changed, Or, With, Without},
schedule::{Schedule, Stage, SystemStage},
system::{
Expand Down Expand Up @@ -281,6 +282,65 @@ mod tests {
assert_eq!(world.resource::<Changed>().0, 2);
}

#[test]
#[should_panic = "error[B0001]"]
fn option_has_no_filter_with() {
fn sys(_: Query<(Option<&A>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn option_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<(Option<&A>, &mut B, &A)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "error[B0001]"]
fn any_of_has_no_filter_with() {
fn sys(_: Query<(AnyOf<(&A, ())>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn any_of_has_filter_with_when_both_have_it() {
fn sys(_: Query<(AnyOf<(&A, &A)>, &mut B)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn any_of_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<(AnyOf<(&A, ())>, &mut B, &A)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic = "error[B0001]"]
fn or_has_no_filter_with() {
fn sys(_: Query<&mut B, Or<(With<A>, With<B>)>>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn or_has_filter_with_when_both_have_it() {
fn sys(_: Query<&mut B, Or<(With<A>, With<A>)>>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
fn or_doesnt_remove_unrelated_filter_with() {
fn sys(_: Query<&mut B, (Or<(With<A>, With<B>)>, With<A>)>, _: Query<&mut B, Without<A>>) {}
let mut world = World::default();
run_system(&mut world, sys);
}

#[test]
#[should_panic]
fn conflicting_query_mut_system() {
Expand Down