[Merged by Bors] - Fix unsoundness in `EntityMut::world_scope` #7387

JoJoJet · 2023-01-27T20:51:40Z

Objective

Found while working on #7385.

The struct EntityMut has the safety invariant that it's cached EntityLocation must always accurately specify where the entity is stored. Thus, any time its location might be invalidated (such as by calling EntityMut::world_mut and moving archetypes), the cached location must be updated by calling EntityMut::update_location.

The method world_scope encapsulates this pattern in safe API by requiring world mutations to be done in a closure, after which update_location will automatically be called. However, this method has a soundness hole: if a panic occurs within the closure, then update_location will never get called. If the panic is caught in an outer scope, then the EntityMut will be left with an outdated location, which is undefined behavior.

An example of this can be seen in the unit test entity_mut_world_scope_panic, which has been added to this PR as a regression test. Without the other changes in this PR, that test will invoke undefined behavior in safe code.

Solution

Call EntityMut::update_location() from within a Drop impl, which ensures that it will get executed even if EntityMut::world_scope unwinds.

james7132

Looks good to me. Nice catch. We might want to audit the other areas of bevy_ecs that use callbacks like this.

crates/bevy_ecs/src/world/entity_ref.rs

james7132 · 2023-01-27T20:55:35Z

crates/bevy_ecs/src/world/entity_ref.rs

+
+        let mut entity = world.spawn_empty();
+        let id = entity.id();
+        let res = std::panic::catch_unwind(AssertUnwindSafe(|| {


Do you need the AssertUnwindSafe by making the closure move?

This would move the variable entity into the closure and make it inaccessible to the rest of the test.

Co-authored-by: James Liu <[email protected]>

alice-i-cecile

Ouch, that's subtle. Good explanation and test though.

I don't have full expertise to decide if this is the best possible solution though.

crates/bevy_ecs/src/world/entity_ref.rs

BoxyUwU · 2023-01-27T21:45:11Z

imo it should be:

    /// Gives mutable access to this `EntityMut`'s [`World`] in a temporary scope.
    pub fn world_scope<T>(&mut self, f: impl FnOnce(&mut World) -> T) -> T {
        let result = std::panic::catch_unwind(AssertUnwindSafe(|| f(self.world)));
        self.update_location();
        result.unwrap_or_else(|e| std::panic::panic_any(e))
    }

this PR's current unsafe code is tbh incredibly sketchy, im so surprised that it doesnt throw a miri error, and if you tweak it a little it -does-. plus many smart people who know how stacked borrows work were also confused why it didnt error (hard to state how funny this is tbh).

JoJoJet · 2023-01-27T22:16:07Z

That's definitely a much cleaner solution. I didn't realize something that straightforward was possible.

~~I take it BlobVec::replace_unchecked could use a similar approach?~~

james7132 · 2023-01-28T01:19:09Z

I'm curious what impact this has on performance, particularly since we use world_scope everywhere in the hierarchy commands.

JoJoJet · 2023-01-28T01:36:48Z

What would be the most relevant benchmarks for measuring that?

This reverts commit 41369f3.

JoJoJet · 2023-01-28T01:55:03Z

I ended up switching to the approach suggested by @hymm, which I like more after thinking about it. This approach should have better codegen since it doesn't pull in the panic machinery (correct me if I'm wrong).

james7132 · 2023-01-28T02:33:32Z

I ended up switching to the approach suggested by @hymm, which I like more after thinking about it. This approach should have better codegen since it doesn't pull in the panic machinery (correct me if I'm wrong).

This solution definitely is cleaner without the unsafe and shouldn't force writing out a panic handler on every call. Should work just fine.

crates/bevy_ecs/src/world/entity_ref.rs

djeedai · 2023-01-28T08:09:35Z

crates/bevy_ecs/src/world/entity_ref.rs

+        impl Drop for Guard<'_, '_> {
+            #[inline]
+            fn drop(&mut self) {
+                self.entity_mut.update_location();


Can we document that world_scope() calls update_location()? This was missing in the original code too.

This was a conscious choice not to mention unsafe implementation details in the docs of a safe method

Fair. But on the other hand if I see the update_location() on docs.rs then I will immediately ask myself "should I use that after world_scope()?", and safety doesn't answer my question; if anything I'd be tempted to think I do need it since it's unsafe but world_scope() is not.

The docs for EntityMut::update_location say that it only needs to be called if you are using EntityMut::world_mut. Perhaps it could be phrased differently to make that more clear?

Then that should be fine.

Co-authored-by: Jerome Humbert <[email protected]>

djeedai

Ah yes thanks for fixing the doc, that was the wrong reference indeed.

james7132 · 2023-01-29T00:10:28Z

bors r+

# Objective Found while working on #7385. The struct `EntityMut` has the safety invariant that it's cached `EntityLocation` must always accurately specify where the entity is stored. Thus, any time its location might be invalidated (such as by calling `EntityMut::world_mut` and moving archetypes), the cached location *must* be updated by calling `EntityMut::update_location`. The method `world_scope` encapsulates this pattern in safe API by requiring world mutations to be done in a closure, after which `update_location` will automatically be called. However, this method has a soundness hole: if a panic occurs within the closure, then `update_location` will never get called. If the panic is caught in an outer scope, then the `EntityMut` will be left with an outdated location, which is undefined behavior. An example of this can be seen in the unit test `entity_mut_world_scope_panic`, which has been added to this PR as a regression test. Without the other changes in this PR, that test will invoke undefined behavior in safe code. ## Solution Call `EntityMut::update_location()` from within a `Drop` impl, which ensures that it will get executed even if `EntityMut::world_scope` unwinds.

bors · 2023-01-29T00:25:49Z

Pull request successfully merged into main.

Build succeeded:

# Objective Found while working on bevyengine#7385. The struct `EntityMut` has the safety invariant that it's cached `EntityLocation` must always accurately specify where the entity is stored. Thus, any time its location might be invalidated (such as by calling `EntityMut::world_mut` and moving archetypes), the cached location *must* be updated by calling `EntityMut::update_location`. The method `world_scope` encapsulates this pattern in safe API by requiring world mutations to be done in a closure, after which `update_location` will automatically be called. However, this method has a soundness hole: if a panic occurs within the closure, then `update_location` will never get called. If the panic is caught in an outer scope, then the `EntityMut` will be left with an outdated location, which is undefined behavior. An example of this can be seen in the unit test `entity_mut_world_scope_panic`, which has been added to this PR as a regression test. Without the other changes in this PR, that test will invoke undefined behavior in safe code. ## Solution Call `EntityMut::update_location()` from within a `Drop` impl, which ensures that it will get executed even if `EntityMut::world_scope` unwinds.

JoJoJet added 10 commits January 27, 2023 13:50

add a test case for EntityMut unsoundness

ce4a192

catch the panic in the test

3412c82

use archetype moves instead of despawning

f134b43

fix unsoundness

bb59e54

rename on_drop -> cleanup_guard

b85453e

be overly cautious

0aff619

fix a stacked borrows failure

f8ff83a

Merge remote-tracking branch 'upstream/main' into world-scope-unsound

33cc770

update a comment

ecb90f7

add a message to a test panic

dfb9934

JoJoJet added A-ECS Entities, components, systems, and events P-Unsound A bug that results in undefined compiler behavior labels Jan 27, 2023

alice-i-cecile requested a review from BoxyUwU January 27, 2023 20:55

alice-i-cecile added this to the 0.10 milestone Jan 27, 2023

james7132 approved these changes Jan 27, 2023

View reviewed changes

Add a link to this PR on the regression test

3cb2ab0

Co-authored-by: James Liu <[email protected]>

alice-i-cecile reviewed Jan 27, 2023

View reviewed changes

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 27, 2023

hymm reviewed Jan 27, 2023

View reviewed changes

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved

catch panics using only safe code

41369f3

JoJoJet added 2 commits January 27, 2023 20:38

Revert "catch panics using only safe code"

feecc66

This reverts commit 41369f3.

use a custom guard type to to perform the cleanup

ed2bc62

hymm approved these changes Jan 28, 2023

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 28, 2023

djeedai suggested changes Jan 28, 2023

View reviewed changes

JoJoJet and others added 2 commits January 28, 2023 08:15

fix a typo

708abcc

Co-authored-by: Jerome Humbert <[email protected]>

make the regression test more rigorous

c082001

djeedai approved these changes Jan 28, 2023

View reviewed changes

minor correction to the docs for EntityMut::update_location

75f507f

djeedai approved these changes Jan 28, 2023

View reviewed changes

bors bot changed the title ~~Fix unsoundness in EntityMut::world_scope~~ [Merged by Bors] - Fix unsoundness in EntityMut::world_scope Jan 29, 2023

bors bot closed this Jan 29, 2023

JoJoJet deleted the world-scope-unsound branch January 29, 2023 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fix unsoundness in `EntityMut::world_scope` #7387

[Merged by Bors] - Fix unsoundness in `EntityMut::world_scope` #7387

JoJoJet commented Jan 27, 2023 •

edited

Loading

james7132 left a comment

james7132 Jan 27, 2023

JoJoJet Jan 27, 2023

alice-i-cecile left a comment

BoxyUwU commented Jan 27, 2023

JoJoJet commented Jan 27, 2023 •

edited

Loading

james7132 commented Jan 28, 2023

JoJoJet commented Jan 28, 2023

JoJoJet commented Jan 28, 2023

james7132 commented Jan 28, 2023

djeedai Jan 28, 2023

tim-blackbird Jan 28, 2023

djeedai Jan 28, 2023 •

edited

Loading

JoJoJet Jan 28, 2023

djeedai Jan 28, 2023

djeedai left a comment

james7132 commented Jan 29, 2023

bors bot commented Jan 29, 2023

[Merged by Bors] - Fix unsoundness in EntityMut::world_scope #7387

[Merged by Bors] - Fix unsoundness in EntityMut::world_scope #7387

Conversation

JoJoJet commented Jan 27, 2023 • edited Loading

Objective

Solution

james7132 left a comment

Choose a reason for hiding this comment

james7132 Jan 27, 2023

Choose a reason for hiding this comment

JoJoJet Jan 27, 2023

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

BoxyUwU commented Jan 27, 2023

JoJoJet commented Jan 27, 2023 • edited Loading

james7132 commented Jan 28, 2023

JoJoJet commented Jan 28, 2023

JoJoJet commented Jan 28, 2023

james7132 commented Jan 28, 2023

djeedai Jan 28, 2023

Choose a reason for hiding this comment

tim-blackbird Jan 28, 2023

Choose a reason for hiding this comment

djeedai Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

JoJoJet Jan 28, 2023

Choose a reason for hiding this comment

djeedai Jan 28, 2023

Choose a reason for hiding this comment

djeedai left a comment

Choose a reason for hiding this comment

james7132 commented Jan 29, 2023

bors bot commented Jan 29, 2023

[Merged by Bors] - Fix unsoundness in `EntityMut::world_scope` #7387

[Merged by Bors] - Fix unsoundness in `EntityMut::world_scope` #7387

JoJoJet commented Jan 27, 2023 •

edited

Loading

JoJoJet commented Jan 27, 2023 •

edited

Loading

djeedai Jan 28, 2023 •

edited

Loading