[Merged by Bors] - fix mutable aliases for a very short time if WorldCell is already borrowed
#6639
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakobhellermann
added
A-ECS
alice-i-cecile
approved these changes
Nov 22, 2022
tim-blackbird
approved these changes
Nov 22, 2022
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Nov 22, 2022
…orrowed (#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))) ```
|
Build failed: |
|
bors retry |
bors bot
pushed a commit
that referenced
this pull request
Nov 22, 2022
…orrowed (#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))) ```
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
WorldCell is already borrowedWorldCell is already borrowed
cart
pushed a commit
that referenced
this pull request
Nov 30, 2022
…orrowed (#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))) ```
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…orrowed (bevyengine#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))) ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Consider the test
Currently, this will roughly execute
where
WorldBorrowMut::newwill panic if the resource is already borrowed.This means, that
_value_awill be created, the access checked (OK), thenvalue_bwill be created, and the access checked (panic).For a moment, both
_value_aand_value_bexisted as&mut Tto the same location, which is insta-UB as far as I understand it.Solution
Flip the order so that
WorldBorrowMut::newfirst checks the access, then fetches creates the value. To do that, we pass aimpl FnOnce() -> Mut<T>instead of theMut<T>directly: