From 721bb919878f7a48f1c954991f03195c8843f6ca Mon Sep 17 00:00:00 2001 From: Joseph <21144246+joseph-gio@users.noreply.github.com> Date: Mon, 3 Feb 2025 13:46:39 -0800 Subject: [PATCH] Add basic debug checks for read-only `UnsafeWorldCell` (#17393) # Objective The method `World::as_unsafe_world_cell_readonly` is used to create an `UnsafeWorldCell` which is only allowed to access world data immutably. This can be tricky to use, as the data that an `UnsafeWorldCell` is allowed to access exists only in documentation (you could think of it as a "doc-time abstraction" rather than a "compile-time" abstraction). It's quite easy to forget where a particular instance came from and attempt to use it for mutable access, leading to instant, silent undefined behavior. ## Solution Add a debug-mode only flag to `UnsafeWorldCell` which tracks whether or not the instance can be used to access world data mutably. This should catch basic improper usages of `as_unsafe_world_cell_readonly`. ## Future work There are a few ways that you can bypass the runtime checks introduced by this PR: * Any world accesses done via `UnsafeWorldCell::storages` are completely invisible to these runtime checks. Unfortunately, `storages` constitutes most of the world accesses used in the engine itself, so this PR will mostly benefit downstream users of bevy. * It's possible to call `get_resource_by_id`, and then convert the returned `Ptr` to a `PtrMut` by calling `assert_unique`. In the future we'll probably want to add a debug-mode only flag to `Ptr` which tracks whether or not it can be upgraded to a `PtrMut`. I didn't include this change in this PR as those types are currently defined using macros which makes it a bit tricky to modify their definitions. * Any data accesses done through a mutable `UnsafeWorldCell` are completely unchecked, meaning it's possible to unsoundly create multiple mutable references to a single component, for example. In the future we may want to store an `Access<>` set inside of the world's `Storages` to add granular debug-mode runtime checks. That said, I'd consider this PR to be a good first step towards adding full runtime checks to `UnsafeWorldCell`. ## Testing Added a few tests that basic invalid mutable world access result in a panic. --------- Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Alice I Cecile --- .../bevy_ecs/src/world/unsafe_world_cell.rs | 96 +++++++++++++++++-- 1 file changed, 89 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index a5ff0f2c9a0bd..8486ee950cc07 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -78,7 +78,12 @@ use {bevy_ptr::UnsafeCellDeref, core::panic::Location}; /// } /// ``` #[derive(Copy, Clone)] -pub struct UnsafeWorldCell<'w>(*mut World, PhantomData<(&'w World, &'w UnsafeCell)>); +pub struct UnsafeWorldCell<'w> { + ptr: *mut World, + #[cfg(debug_assertions)] + allows_mutable_access: bool, + _marker: PhantomData<(&'w World, &'w UnsafeCell)>, +} // SAFETY: `&World` and `&mut World` are both `Send` unsafe impl Send for UnsafeWorldCell<'_> {} @@ -101,13 +106,37 @@ impl<'w> UnsafeWorldCell<'w> { /// Creates a [`UnsafeWorldCell`] that can be used to access everything immutably #[inline] pub(crate) fn new_readonly(world: &'w World) -> Self { - Self(ptr::from_ref(world).cast_mut(), PhantomData) + Self { + ptr: ptr::from_ref(world).cast_mut(), + #[cfg(debug_assertions)] + allows_mutable_access: false, + _marker: PhantomData, + } } /// Creates [`UnsafeWorldCell`] that can be used to access everything mutably #[inline] pub(crate) fn new_mutable(world: &'w mut World) -> Self { - Self(ptr::from_mut(world), PhantomData) + Self { + ptr: ptr::from_mut(world), + #[cfg(debug_assertions)] + allows_mutable_access: true, + _marker: PhantomData, + } + } + + #[cfg_attr(debug_assertions, inline(never), track_caller)] + #[cfg_attr(not(debug_assertions), inline(always))] + pub(crate) fn assert_allows_mutable_access(self) { + // This annotation is needed because the + // allows_mutable_access field doesn't exist otherwise. + // Kinda weird, since debug_assert would never be called, + // but CI complained in https://github.com/bevyengine/bevy/pull/17393 + #[cfg(debug_assertions)] + debug_assert!( + self.allows_mutable_access, + "mutating world data via `World::as_unsafe_world_cell_readonly` is forbidden" + ); } /// Gets a mutable reference to the [`World`] this [`UnsafeWorldCell`] belongs to. @@ -155,9 +184,10 @@ impl<'w> UnsafeWorldCell<'w> { /// ``` #[inline] pub unsafe fn world_mut(self) -> &'w mut World { + self.assert_allows_mutable_access(); // SAFETY: // - caller ensures the created `&mut World` is the only borrow of world - unsafe { &mut *self.0 } + unsafe { &mut *self.ptr } } /// Gets a reference to the [`&World`](World) this [`UnsafeWorldCell`] belongs to. @@ -206,7 +236,7 @@ impl<'w> UnsafeWorldCell<'w> { // SAFETY: // - caller ensures that the returned `&World` is not used in a way that would conflict // with any existing mutable borrows of world data - unsafe { &*self.0 } + unsafe { &*self.ptr } } /// Retrieves this world's unique [ID](WorldId). @@ -451,6 +481,7 @@ impl<'w> UnsafeWorldCell<'w> { /// - no other references to the resource exist at the same time #[inline] pub unsafe fn get_resource_mut(self) -> Option> { + self.assert_allows_mutable_access(); let component_id = self.components().get_resource_id(TypeId::of::())?; // SAFETY: // - caller ensures `self` has permission to access the resource mutably @@ -478,6 +509,7 @@ impl<'w> UnsafeWorldCell<'w> { self, component_id: ComponentId, ) -> Option> { + self.assert_allows_mutable_access(); // SAFETY: we only access data that the caller has ensured is unaliased and `self` // has permission to access. let (ptr, ticks, _caller) = unsafe { self.storages() } @@ -514,6 +546,7 @@ impl<'w> UnsafeWorldCell<'w> { /// - no other references to the resource exist at the same time #[inline] pub unsafe fn get_non_send_resource_mut(self) -> Option> { + self.assert_allows_mutable_access(); let component_id = self.components().get_resource_id(TypeId::of::())?; // SAFETY: // - caller ensures that `self` has permission to access the resource @@ -544,6 +577,7 @@ impl<'w> UnsafeWorldCell<'w> { self, component_id: ComponentId, ) -> Option> { + self.assert_allows_mutable_access(); let change_tick = self.change_tick(); // SAFETY: we only access data that the caller has ensured is unaliased and `self` // has permission to access. @@ -617,18 +651,20 @@ impl<'w> UnsafeWorldCell<'w> { /// - the [`UnsafeWorldCell`] has permission to access the queue mutably /// - no mutable references to the queue exist at the same time pub(crate) unsafe fn get_raw_command_queue(self) -> RawCommandQueue { + self.assert_allows_mutable_access(); // SAFETY: // - caller ensures there are no existing mutable references // - caller ensures that we have permission to access the queue - unsafe { (*self.0).command_queue.clone() } + unsafe { (*self.ptr).command_queue.clone() } } /// # Safety /// It is the callers responsibility to ensure that there are no outstanding /// references to `last_trigger_id`. pub(crate) unsafe fn increment_trigger_id(self) { + self.assert_allows_mutable_access(); // SAFETY: Caller ensure there are no outstanding references - unsafe { (*self.0).last_trigger_id += 1 } + unsafe { (*self.ptr).last_trigger_id += 1 } } } @@ -878,6 +914,8 @@ impl<'w> UnsafeEntityCell<'w> { last_change_tick: Tick, change_tick: Tick, ) -> Option> { + self.world.assert_allows_mutable_access(); + let component_id = self.world.components().get_id(TypeId::of::())?; // SAFETY: @@ -994,6 +1032,8 @@ impl<'w> UnsafeEntityCell<'w> { self, component_id: ComponentId, ) -> Result, GetEntityMutByIdError> { + self.world.assert_allows_mutable_access(); + let info = self .world .components() @@ -1178,3 +1218,45 @@ impl EntityBorrow for UnsafeEntityCell<'_> { self.id() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate as bevy_ecs; + + #[test] + #[should_panic = "is forbidden"] + fn as_unsafe_world_cell_readonly_world_mut_forbidden() { + let world = World::new(); + let world_cell = world.as_unsafe_world_cell_readonly(); + // SAFETY: this invalid usage will be caught by a runtime panic. + let _ = unsafe { world_cell.world_mut() }; + } + + #[derive(Resource)] + struct R; + + #[test] + #[should_panic = "is forbidden"] + fn as_unsafe_world_cell_readonly_resource_mut_forbidden() { + let mut world = World::new(); + world.insert_resource(R); + let world_cell = world.as_unsafe_world_cell_readonly(); + // SAFETY: this invalid usage will be caught by a runtime panic. + let _ = unsafe { world_cell.get_resource_mut::() }; + } + + #[derive(Component)] + struct C; + + #[test] + #[should_panic = "is forbidden"] + fn as_unsafe_world_cell_readonly_component_mut_forbidden() { + let mut world = World::new(); + let entity = world.spawn(C).id(); + let world_cell = world.as_unsafe_world_cell_readonly(); + let entity_cell = world_cell.get_entity(entity).unwrap(); + // SAFETY: this invalid usage will be caught by a runtime panic. + let _ = unsafe { entity_cell.get_mut::() }; + } +}