Potential for UB in systems using `AssetChanged`

[Trashtalk217]

Bevy version and features

08a3f24 (main)

What you did

I ran the following test in bevy_asset/src/asset_changed.rs.

#[test]
#[should_panic]
fn should_conflict() {
    // should conflict, doesn't.
    fn system(
        q1: Query<&Foo, AssetChanged<MyComponent>>,
        q2: Query<&mut AssetChanges<MyAsset>, bevy_ecs::query::Without<Foo>>,
    ) {
    }
   assert_is_system(system);
}

It failed.

What went wrong

q1, through AssetChanged places a read access on AssetChanges<MyAsset>. Meanwhile, q2 writes to AssetChanges<MyAsset>, and so the two queries should conflict. Yet they don't. This is potentially a source of UB.

Additional information

While the situation where one SystemParam tries to access both an entity and a resource is rare, this only became UB after #20934. Currently, this has zero impact, because AssetChanges<MyAsset> is pub(crate) and we never create a Query<&mut AssetChanges<A>> query. However, if we remove resources from Access, this problem can also be triggered through ResMut<AssetChanges<A>>. Such a query does exist in the bevy codebase now, and as such this issue blocks #22910.

There are several solutions.

• The most promising is Nested Queries #21557, which allows queries to query multiple different entities.
• Assets as entities v0 #22939 might also be a solution, but I haven't had time to look at how that redesign is going to work.
• A very ugly solution would be to wrap AssetChanges in an Arc<RwLock<...>> somehow, so conflicts don't matter.

[hymm]

I think there's a typo in your example. q1 should be on MyAsset not MyComponent.

Why can't AssetChanged<T> register a read on AssetChanges<T>?

[hymm]

I would expect "assets as entities" would remove the need for a AssetChanges resource as you could get the change ticks from the asset component.

[hymm]

There's more potential for UB as you could add a system with AssetChanged<A> to the same schedule as the asset_events system that updates the ResMut<AssetChanges<A> and it would be a data race if they run in parallel. I think this is possible for a user to do.