forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add basic debug checks for read-only UnsafeWorldCell (#17393)

Browse Source 
# 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 <alice.i.cecile@gmail.com>
This commit is contained in:
Joseph 2025-02-03 13:46:39 -08:00 committed by GitHub
parent adcc80c43d
commit 721bb91987
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 89 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

96
crates/bevy_ecs/src/world/unsafe_world_cell.rs
View File

@ -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<World>)>);
pub struct UnsafeWorldCell<'w> {
ptr: *mut World,
#[cfg(debug_assertions)]
allows_mutable_access: bool,
_marker: PhantomData<(&'w World, &'w UnsafeCell<World>)>,
}
// 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<R: Resource>(self) -> Option<Mut<'w, R>> {
self.assert_allows_mutable_access();
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
// SAFETY:
// - caller ensures `self` has permission to access the resource mutably
@ -478,6 +509,7 @@ impl<'w> UnsafeWorldCell<'w> {
self,
component_id: ComponentId,
) -> Option<MutUntyped<'w>> {
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<R: 'static>(self) -> Option<Mut<'w, R>> {
self.assert_allows_mutable_access();
let component_id = self.components().get_resource_id(TypeId::of::<R>())?;
// SAFETY:
// - caller ensures that `self` has permission to access the resource
@ -544,6 +577,7 @@ impl<'w> UnsafeWorldCell<'w> {
self,
component_id: ComponentId,
) -> Option<MutUntyped<'w>> {
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<Mut<'w, T>> {
self.world.assert_allows_mutable_access();
let component_id = self.world.components().get_id(TypeId::of::<T>())?;
// SAFETY:
@ -994,6 +1032,8 @@ impl<'w> UnsafeEntityCell<'w> {
self,
component_id: ComponentId,
) -> Result<MutUntyped<'w>, 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::<R>() };
}
#[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::<C>() };
}
}