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

BoxyUwU · 2022-05-04T17:10:09Z

Objective

Example code that wasnt panic'ing before this PR (and so was unsound):

    #[test]
    #[should_panic = "error[B0001]"]
    fn option_has_no_filter_with() {
        fn sys(_1: Query<(Option<&A>, &mut B)>, _2: 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(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: 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(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

Solution

Only add the intersection of with/without accesses of all the elements in Or/AnyOf to the world query's FilteredAccess<ComponentId> instead of the union.
Option's fix can be thought of the same way since its basically AnyOf<T, ()> but its impl is just simpler as () has no with/without accesses

Changelog

Or/AnyOf/Option will now report more query conflicts in order to fix unsoundness

Migration Guide

If you are now getting query conflicts from Or/AnyOf/Option rip to you and ur welcome for it now being caught

alice-i-cecile · 2022-05-04T18:55:18Z

crates/bevy_ecs/src/query/filter.rs

+                // 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();


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.

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)

Nilirad · 2022-05-04T19:36:28Z

Please I want that migration guide on the next release.

cart · 2022-05-05T22:38:33Z

crates/bevy_ecs/src/query/fetch.rs

@@ -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


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)>)>, y: Query<&mut C, (Without, 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 :)

Yea you're right that Option<(&mut C, With)> 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.

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.

cart · 2022-05-06T02:48:02Z

crates/bevy_ecs/src/query/fetch.rs

+                    if _not_first {
+                        let mut intermediate = _access.clone();
+                        $name.update_component_access(&mut intermediate);
+                        _intersected_access.extend_intersect_filter(&intermediate);


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.

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.

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.

cart

I'm on board for this fix. As mentioned in some comments, I think we should add some docs to capture some of the weirder stuff going on here. I'll be gone for a wedding starting tomorrow, so maintainers should feel free to merge this when those docs are added.

alice-i-cecile · 2022-05-18T20:57:08Z

bors r+

# Objective Fixes #4657 Example code that wasnt panic'ing before this PR (and so was unsound): ```rust #[test] #[should_panic = "error[B0001]"] fn option_has_no_filter_with() { fn sys(_1: Query<(Option<&A>, &mut B)>, _2: 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(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: 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(_1: Query<&mut B, Or<(With<A>, With)>>, _2: Query<&mut B, Without<A>>) {} let mut world = World::default(); run_system(&mut world, sys); } ``` ## Solution - Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union. - `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses --- ## Changelog - `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness ## Migration Guide - If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught

bors · 2022-05-18T21:09:14Z

Pull request successfully merged into main.

Build succeeded:

…ine#4659) # Objective Fixes bevyengine#4657 Example code that wasnt panic'ing before this PR (and so was unsound): ```rust #[test] #[should_panic = "error[B0001]"] fn option_has_no_filter_with() { fn sys(_1: Query<(Option<&A>, &mut B)>, _2: 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(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: 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(_1: Query<&mut B, Or<(With<A>, With)>>, _2: Query<&mut B, Without<A>>) {} let mut world = World::default(); run_system(&mut world, sys); } ``` ## Solution - Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union. - `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses --- ## Changelog - `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness ## Migration Guide - If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught

github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 4, 2022

BoxyUwU added A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels May 4, 2022

BoxyUwU force-pushed the fix_access_unsoundness branch from 4262c9e to 098f4b7 Compare May 4, 2022 17:38

IceSentry added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 4, 2022

alice-i-cecile reviewed May 4, 2022

View reviewed changes

alice-i-cecile self-requested a review May 4, 2022 22:33

cart added this to the Bevy 0.8 milestone May 5, 2022

cart added the P-Unsound A bug that results in undefined compiler behavior label May 5, 2022

cart reviewed May 5, 2022

View reviewed changes

cart reviewed May 6, 2022

View reviewed changes

cart approved these changes May 6, 2022

View reviewed changes

BoxyUwU force-pushed the fix_access_unsoundness branch from 1f6984d to 70b9cc4 Compare May 8, 2022 12:37

BoxyUwU added 2 commits May 12, 2022 15:18

yeet

44aa3aa

filtered access docs

6d5fe30

BoxyUwU force-pushed the fix_access_unsoundness branch from 70b9cc4 to 6d5fe30 Compare May 12, 2022 14:20

alice-i-cecile approved these changes May 18, 2022

View reviewed changes

bors bot changed the title ~~Fix unsoundness with Or/AnyOf/Option component access'~~ [Merged by Bors] - Fix unsoundness with Or/AnyOf/Option component access' May 18, 2022

bors bot closed this May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BoxyUwU commented May 4, 2022

alice-i-cecile May 4, 2022

BoxyUwU May 4, 2022

Nilirad commented May 4, 2022

cart May 5, 2022

BoxyUwU May 5, 2022

alice-i-cecile May 6, 2022

cart May 6, 2022

cart May 6, 2022

cart May 6, 2022 •

edited

Loading

cart left a comment

alice-i-cecile commented May 18, 2022

bors bot commented May 18, 2022

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

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

Conversation

BoxyUwU commented May 4, 2022

Objective

Solution

Changelog

Migration Guide

alice-i-cecile May 4, 2022

Choose a reason for hiding this comment

BoxyUwU May 4, 2022

Choose a reason for hiding this comment

Nilirad commented May 4, 2022

cart May 5, 2022

Choose a reason for hiding this comment

BoxyUwU May 5, 2022

Choose a reason for hiding this comment

alice-i-cecile May 6, 2022

Choose a reason for hiding this comment

cart May 6, 2022

Choose a reason for hiding this comment

cart May 6, 2022

Choose a reason for hiding this comment

cart May 6, 2022 • edited Loading

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

alice-i-cecile commented May 18, 2022

bors bot commented May 18, 2022

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

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

cart May 6, 2022 •

edited

Loading