Skip to content

Commit 3643074

Browse files
jakobhellermanncart
authored andcommitted
fix mutable aliases for a very short time if WorldCell is already borrowed (#6639)
# Objective Consider the test ```rust let cell = world.cell(); let _value_a = cell.resource_mut::<A>(); let _value_b = cell.resource_mut::<A>(); ``` Currently, this will roughly execute ```rust // first call let value = unsafe { self.world .get_non_send_unchecked_mut_with_id(component_id)? }; return Some(WorldBorrowMut::new(value, archetype_component_id, self.access))) // second call let value = unsafe { self.world .get_non_send_unchecked_mut_with_id(component_id)? }; return Some(WorldBorrowMut::new(value, archetype_component_id, self.access))) ``` where `WorldBorrowMut::new` will panic if the resource is already borrowed. This means, that `_value_a` will be created, the access checked (OK), then `value_b` will be created, and the access checked (`panic`). For a moment, both `_value_a` and `_value_b` existed as `&mut T` to the same location, which is insta-UB as far as I understand it. ## Solution Flip the order so that `WorldBorrowMut::new` first checks the access, _then_ fetches creates the value. To do that, we pass a `impl FnOnce() -> Mut<T>` instead of the `Mut<T>` directly: ```rust let get_value = || unsafe { self.world .get_non_send_unchecked_mut_with_id(component_id)? }; return Some(WorldBorrowMut::new(get_value, archetype_component_id, self.access))) ```
1 parent 5265b47 commit 3643074

File tree

1 file changed

+24
-28
lines changed

1 file changed

+24
-28
lines changed

crates/bevy_ecs/src/world/world_cell.rs

Lines changed: 24 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -88,21 +88,22 @@ pub struct WorldBorrow<'w, T> {
8888
}
8989

9090
impl<'w, T> WorldBorrow<'w, T> {
91-
fn new(
92-
value: &'w T,
91+
fn try_new(
92+
value: impl FnOnce() -> Option<&'w T>,
9393
archetype_component_id: ArchetypeComponentId,
9494
access: Rc<RefCell<ArchetypeComponentAccess>>,
95-
) -> Self {
95+
) -> Option<Self> {
9696
assert!(
9797
access.borrow_mut().read(archetype_component_id),
9898
"Attempted to immutably access {}, but it is already mutably borrowed",
9999
std::any::type_name::<T>(),
100100
);
101-
Self {
101+
let value = value()?;
102+
Some(Self {
102103
value,
103104
archetype_component_id,
104105
access,
105-
}
106+
})
106107
}
107108
}
108109

@@ -129,21 +130,22 @@ pub struct WorldBorrowMut<'w, T> {
129130
}
130131

131132
impl<'w, T> WorldBorrowMut<'w, T> {
132-
fn new(
133-
value: Mut<'w, T>,
133+
fn try_new(
134+
value: impl FnOnce() -> Option<Mut<'w, T>>,
134135
archetype_component_id: ArchetypeComponentId,
135136
access: Rc<RefCell<ArchetypeComponentAccess>>,
136-
) -> Self {
137+
) -> Option<Self> {
137138
assert!(
138139
access.borrow_mut().write(archetype_component_id),
139140
"Attempted to mutably access {}, but it is already mutably borrowed",
140141
std::any::type_name::<T>(),
141142
);
142-
Self {
143+
let value = value()?;
144+
Some(Self {
143145
value,
144146
archetype_component_id,
145147
access,
146-
}
148+
})
147149
}
148150
}
149151

@@ -189,12 +191,12 @@ impl<'w> WorldCell<'w> {
189191
let archetype_component_id = self
190192
.world
191193
.get_resource_archetype_component_id(component_id)?;
192-
Some(WorldBorrow::new(
194+
WorldBorrow::try_new(
193195
// SAFETY: ComponentId matches TypeId
194-
unsafe { self.world.get_resource_with_id(component_id)? },
196+
|| unsafe { self.world.get_resource_with_id(component_id) },
195197
archetype_component_id,
196198
self.access.clone(),
197-
))
199+
)
198200
}
199201

200202
/// Gets a reference to the resource of the given type
@@ -222,15 +224,12 @@ impl<'w> WorldCell<'w> {
222224
let archetype_component_id = self
223225
.world
224226
.get_resource_archetype_component_id(component_id)?;
225-
Some(WorldBorrowMut::new(
227+
WorldBorrowMut::try_new(
226228
// SAFETY: ComponentId matches TypeId and access is checked by WorldBorrowMut
227-
unsafe {
228-
self.world
229-
.get_resource_unchecked_mut_with_id(component_id)?
230-
},
229+
|| unsafe { self.world.get_resource_unchecked_mut_with_id(component_id) },
231230
archetype_component_id,
232231
self.access.clone(),
233-
))
232+
)
234233
}
235234

236235
/// Gets a mutable reference to the resource of the given type
@@ -258,12 +257,12 @@ impl<'w> WorldCell<'w> {
258257
let archetype_component_id = self
259258
.world
260259
.get_resource_archetype_component_id(component_id)?;
261-
Some(WorldBorrow::new(
260+
WorldBorrow::try_new(
262261
// SAFETY: ComponentId matches TypeId
263-
unsafe { self.world.get_non_send_with_id(component_id)? },
262+
|| unsafe { self.world.get_non_send_with_id(component_id) },
264263
archetype_component_id,
265264
self.access.clone(),
266-
))
265+
)
267266
}
268267

269268
/// Gets an immutable reference to the non-send resource of the given type, if it exists.
@@ -291,15 +290,12 @@ impl<'w> WorldCell<'w> {
291290
let archetype_component_id = self
292291
.world
293292
.get_resource_archetype_component_id(component_id)?;
294-
Some(WorldBorrowMut::new(
293+
WorldBorrowMut::try_new(
295294
// SAFETY: ComponentId matches TypeId and access is checked by WorldBorrowMut
296-
unsafe {
297-
self.world
298-
.get_non_send_unchecked_mut_with_id(component_id)?
299-
},
295+
|| unsafe { self.world.get_non_send_unchecked_mut_with_id(component_id) },
300296
archetype_component_id,
301297
self.access.clone(),
302-
))
298+
)
303299
}
304300

305301
/// Gets a mutable reference to the non-send resource of the given type, if it exists.

0 commit comments

Comments
 (0)