-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Potential for UB in systems using AssetChanged #23057
Copy link
Copy link
Closed
Labels
A-AssetsLoad files from disk to use for things like images, models, and soundsLoad files from disk to use for things like images, models, and soundsA-ECSEntities, components, systems, and eventsEntities, components, systems, and eventsC-BugAn unexpected or incorrect behaviorAn unexpected or incorrect behaviorP-UnsoundA bug that results in undefined compiler behaviorA bug that results in undefined compiler behaviorS-Needs-DesignThis issue requires design work to think about how it would best be accomplishedThis issue requires design work to think about how it would best be accomplished
Milestone
Metadata
Metadata
Assignees
Labels
A-AssetsLoad files from disk to use for things like images, models, and soundsLoad files from disk to use for things like images, models, and soundsA-ECSEntities, components, systems, and eventsEntities, components, systems, and eventsC-BugAn unexpected or incorrect behaviorAn unexpected or incorrect behaviorP-UnsoundA bug that results in undefined compiler behaviorA bug that results in undefined compiler behaviorS-Needs-DesignThis issue requires design work to think about how it would best be accomplishedThis issue requires design work to think about how it would best be accomplished
Bevy version and features
08a3f24(main)What you did
I ran the following test in
bevy_asset/src/asset_changed.rs.It failed.
What went wrong
q1, throughAssetChangedplaces a read access onAssetChanges<MyAsset>. Meanwhile,q2writes toAssetChanges<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
SystemParamtries to access both an entity and a resource is rare, this only became UB after #20934. Currently, this has zero impact, becauseAssetChanges<MyAsset>ispub(crate)and we never create aQuery<&mut AssetChanges<A>>query. However, if we remove resources fromAccess, this problem can also be triggered throughResMut<AssetChanges<A>>. Such a query does exist in the bevy codebase now, and as such this issue blocks #22910.There are several solutions.
AssetChangesin anArc<RwLock<...>>somehow, so conflicts don't matter.