Skip to content

EntityRef as WorldQuery is unsound. #9422

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.

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

Closed
joseph-gio opened this issue Aug 11, 2023 · 2 comments · Fixed by #9419
Closed

EntityRef as WorldQuery is unsound. #9422

joseph-gio opened this issue Aug 11, 2023 · 2 comments · Fixed by #9419
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior

Comments

@joseph-gio
Copy link
Member

joseph-gio commented Aug 11, 2023

Bevy version

Main branch.

What you did

    #[test]
    fn unsound_disjoint_mutable_access() {
        fn unsound_system(
            mut query: Query<(Entity, &mut TestComponent)>,
            refs: Query<super::EntityRef, Without<TestComponent>>,
        ) {
            let (id, mut c) = query.iter_mut().next().unwrap();
            let world = refs.iter().next().unwrap().world();

            // We now have mutable and shared access to the same component at the same time.
            let c2 = world.entity(id).get::<TestComponent>().unwrap();
            *c = TestComponent(1);
            println!("{c2:?}");
        }

        let mut world = World::new();
        world.spawn(TestComponent(0));
        world.spawn_empty();

        let mut schedule = Schedule::new();
        schedule.add_systems(unsound_system);

        schedule.run(&mut world);
    }

What went wrong

This should panic at runtime since the accesses of the two system params conflict. Instead, it runs and allows aliased mutable access.

@joseph-gio joseph-gio added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior and removed S-Needs-Triage This issue needs to be labelled labels Aug 11, 2023
@james7132 james7132 added the P-High This is particularly urgent, and deserves immediate attention label Aug 12, 2023
@james7132
Copy link
Member

EntityRef::world was supposed to be removed in #6960 to make this impossible. Apparently it wasn't. Probably the result of a bad merge or rebase.

@joseph-gio
Copy link
Member Author

Ah, looks like it snuck back in during this merge: 0c91c16

github-merge-queue bot pushed a commit that referenced this issue Aug 29, 2023
# Objective

Fix #4278
Fix #5504
Fix #9422

Provide safe ways to borrow an entire entity, while allowing disjoint
mutable access. `EntityRef` and `EntityMut` are not suitable for this,
since they provide access to the entire world -- they are just helper
types for working with `&World`/`&mut World`.

This has potential uses for reflection and serialization

## Solution

Remove `EntityRef::world`, which allows it to soundly be used within
queries.

`EntityMut` no longer supports structural world mutations, which allows
multiple instances of it to exist for different entities at once.
Structural world mutations are performed using the new type
`EntityWorldMut`.

```rust
fn disjoint_system(
     q2: Query<&mut A>,
     q1: Query<EntityMut, Without<A>>,
) { ... }

let [entity1, entity2] = world.many_entities_mut([id1, id2]);
*entity1.get_mut::<T>().unwrap() = *entity2.get().unwrap();

for entity in world.iter_entities_mut() {
    ...
}
```

---

## Changelog

- Removed `EntityRef::world`, to fix a soundness issue with queries.
+ Removed the ability to structurally mutate the world using
`EntityMut`, which allows it to be used in queries.
+ Added `EntityWorldMut`, which is used to perform structural mutations
that are no longer allowed using `EntityMut`.

## Migration Guide

**Note for maintainers: ensure that the guide for #9604 is updated
accordingly.**

Removed the method `EntityRef::world`, to fix a soundness issue with
queries. If you need access to `&World` while using an `EntityRef`,
consider passing the world as a separate parameter.

`EntityMut` can no longer perform 'structural' world mutations, such as
adding or removing components, or despawning the entity. Additionally,
`EntityMut::world`, `EntityMut::world_mut` , and
`EntityMut::world_scope` have been removed.
Instead, use the newly-added type `EntityWorldMut`, which is a helper
type for working with `&mut World`.

---------

Co-authored-by: Alice Cecile <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
# Objective

Fix bevyengine#4278
Fix bevyengine#5504
Fix bevyengine#9422

Provide safe ways to borrow an entire entity, while allowing disjoint
mutable access. `EntityRef` and `EntityMut` are not suitable for this,
since they provide access to the entire world -- they are just helper
types for working with `&World`/`&mut World`.

This has potential uses for reflection and serialization

## Solution

Remove `EntityRef::world`, which allows it to soundly be used within
queries.

`EntityMut` no longer supports structural world mutations, which allows
multiple instances of it to exist for different entities at once.
Structural world mutations are performed using the new type
`EntityWorldMut`.

```rust
fn disjoint_system(
     q2: Query<&mut A>,
     q1: Query<EntityMut, Without<A>>,
) { ... }

let [entity1, entity2] = world.many_entities_mut([id1, id2]);
*entity1.get_mut::<T>().unwrap() = *entity2.get().unwrap();

for entity in world.iter_entities_mut() {
    ...
}
```

---

## Changelog

- Removed `EntityRef::world`, to fix a soundness issue with queries.
+ Removed the ability to structurally mutate the world using
`EntityMut`, which allows it to be used in queries.
+ Added `EntityWorldMut`, which is used to perform structural mutations
that are no longer allowed using `EntityMut`.

## Migration Guide

**Note for maintainers: ensure that the guide for bevyengine#9604 is updated
accordingly.**

Removed the method `EntityRef::world`, to fix a soundness issue with
queries. If you need access to `&World` while using an `EntityRef`,
consider passing the world as a separate parameter.

`EntityMut` can no longer perform 'structural' world mutations, such as
adding or removing components, or despawning the entity. Additionally,
`EntityMut::world`, `EntityMut::world_mut` , and
`EntityMut::world_scope` have been removed.
Instead, use the newly-added type `EntityWorldMut`, which is a helper
type for working with `&mut World`.

---------

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants