[Merged by Bors] - unsafeify `World::entities_mut` #4093

BoxyUwU · 2022-03-04T01:30:36Z

Objective

make bevy ecs a lil bit less unsound

Solution

make unsound API unsafe so that there is an unsafe block to blame:

use bevy_ecs::prelude::*;

#[derive(Debug, Component)]
struct Foo(u8);

fn main() {
    let mut world = World::new();
    let e1 = world.spawn().id();
    let e2 = world.spawn().insert(Foo(2)).id();
    world.entities_mut().meta[0] = world.entities_mut().meta[1].clone();
    let foo = world.entity(e1).get::<Foo>().unwrap();
    // whoo i love having components i dont have
    dbg!(foo);
}

This is not strictly speaking UB, however:

Query::get_multiple cannot work if this is allowed
bevy_ecs is a pile of unsafe code whose soundness generally depends on the world being in a "correct" state with "no funny business" so it seems best to disallow this
it is trivial to get bevy to panic inside of functions with safety invariants that have been violated (the entity location is not valid)
it seems to violate what the safety invariant on Entities::flush is trying to ensure

crates/bevy_ecs/src/world/mod.rs

james7132 · 2022-03-04T02:12:14Z

crates/bevy_render/src/lib.rs

@@ -198,7 +198,7 @@ impl Plugin for RenderPlugin {
                    // flushing as "invalid" ensures that app world entities aren't added as "empty archetype" entities by default
                    // these entities cannot be accessed without spawning directly onto them
                    // this _only_ works as expected because clear_entities() is called at the end of every frame.
-                    render_app.world.entities_mut().flush_as_invalid();
+                    unsafe { render_app.world.entities_mut() }.flush_as_invalid();


Is there a way to avoid this entirely?

not sure, I dont particularly understand what the render app is actually trying to accomplish here 😅

the render app "reserve" all the entities from the main app, but actually spawn something only on some of them. this marks all entities by default as invalid so that the one which won't be used aren't usable

The "right" way to do this would be to expose a safe mirror of flush_as_invalid() on World. But honestly this feature is niche enough that im fine leaving this as-is for now.

crates/bevy_ecs/src/entity/mod.rs

crates/bevy_ecs/src/world/mod.rs

Guvante · 2022-03-17T17:14:36Z

Figuring out how to fix the flush_as_invalid use case seems valuable but the changes otherwise seem good.

alice-i-cecile

I agree with this change: this is very easy to create UB in safe code with, and so it's unsound if this is safe.

Entities is not very useful in general to the average user, and mutating it is particularly rare.

I really wish we would could avoid this being pub, but we can't, as it's needed in bevy_render.

alice-i-cecile · 2022-03-30T22:38:34Z

Adding to the 0.7 milestone; this is now actually UB since #4298 is merged.

cart · 2022-03-30T23:52:29Z

bors r+

# Objective make bevy ecs a lil bit less unsound ## Solution make unsound API unsafe so that there is an unsafe block to blame: ```rust use bevy_ecs::prelude::*; #[derive(Debug, Component)] struct Foo(u8); fn main() { let mut world = World::new(); let e1 = world.spawn().id(); let e2 = world.spawn().insert(Foo(2)).id(); world.entities_mut().meta[0] = world.entities_mut().meta[1].clone(); let foo = world.entity(e1).get::<Foo>().unwrap(); // whoo i love having components i dont have dbg!(foo); } ``` This is not _strictly_ speaking UB, however: - `Query::get_multiple` cannot work if this is allowed - bevy_ecs is a pile of unsafe code whose soundness generally depends on the world being in a "correct" state with "no funny business" so it seems best to disallow this - it is trivial to get bevy to panic inside of functions with safety invariants that have been violated (the entity location is not valid) - it seems to violate what the safety invariant on `Entities::flush` is trying to ensure

bors · 2022-03-31T00:09:23Z

Pull request successfully merged into main.

Build succeeded:

# Objective make bevy ecs a lil bit less unsound ## Solution make unsound API unsafe so that there is an unsafe block to blame: ```rust use bevy_ecs::prelude::*; #[derive(Debug, Component)] struct Foo(u8); fn main() { let mut world = World::new(); let e1 = world.spawn().id(); let e2 = world.spawn().insert(Foo(2)).id(); world.entities_mut().meta[0] = world.entities_mut().meta[1].clone(); let foo = world.entity(e1).get::<Foo>().unwrap(); // whoo i love having components i dont have dbg!(foo); } ``` This is not _strictly_ speaking UB, however: - `Query::get_multiple` cannot work if this is allowed - bevy_ecs is a pile of unsafe code whose soundness generally depends on the world being in a "correct" state with "no funny business" so it seems best to disallow this - it is trivial to get bevy to panic inside of functions with safety invariants that have been violated (the entity location is not valid) - it seems to violate what the safety invariant on `Entities::flush` is trying to ensure

BoxyUwU added the A-ECS Entities, components, systems, and events label Mar 4, 2022

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 4, 2022

IceSentry removed the S-Needs-Triage This issue needs to be labelled label Mar 4, 2022

BoxyUwU force-pushed the remove_entities_mut branch 2 times, most recently from db0f44b to 52bdff0 Compare March 4, 2022 02:19

james7132 requested changes Mar 4, 2022

View reviewed changes

BoxyUwU changed the title ~~yeet World::entities_mut >:(~~ yeet safe World::entities_mut >:( Mar 4, 2022

alice-i-cecile added C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention labels Mar 4, 2022

alice-i-cecile reviewed Mar 4, 2022

View reviewed changes

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

alice-i-cecile reviewed Mar 4, 2022

View reviewed changes

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

>:(

6b3cd61

BoxyUwU force-pushed the remove_entities_mut branch from 68499b4 to 6b3cd61 Compare March 5, 2022 01:50

BoxyUwU changed the title ~~yeet safe World::entities_mut >:(~~ unsafeify World::entities_mut Mar 22, 2022

alice-i-cecile requested changes Mar 24, 2022

View reviewed changes

alice-i-cecile approved these changes Mar 24, 2022

View reviewed changes

TheRawMeatball approved these changes Mar 28, 2022

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 Mar 28, 2022

alice-i-cecile added this to the Bevy 0.7 milestone Mar 30, 2022

bors bot changed the title ~~unsafeify World::entities_mut~~ [Merged by Bors] - unsafeify World::entities_mut Mar 31, 2022

bors bot closed this Mar 31, 2022

james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - unsafeify `World::entities_mut` #4093

[Merged by Bors] - unsafeify `World::entities_mut` #4093

BoxyUwU commented Mar 4, 2022 •

edited

Loading

james7132 Mar 4, 2022

BoxyUwU Mar 4, 2022

mockersf Mar 4, 2022 •

edited

Loading

cart Mar 30, 2022

Guvante commented Mar 17, 2022

alice-i-cecile left a comment

alice-i-cecile commented Mar 30, 2022

cart commented Mar 30, 2022

bors bot commented Mar 31, 2022

[Merged by Bors] - unsafeify World::entities_mut #4093

[Merged by Bors] - unsafeify World::entities_mut #4093

Conversation

BoxyUwU commented Mar 4, 2022 • edited Loading

Objective

Solution

james7132 Mar 4, 2022

Choose a reason for hiding this comment

BoxyUwU Mar 4, 2022

Choose a reason for hiding this comment

mockersf Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

cart Mar 30, 2022

Choose a reason for hiding this comment

Guvante commented Mar 17, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Mar 30, 2022

cart commented Mar 30, 2022

bors bot commented Mar 31, 2022

[Merged by Bors] - unsafeify `World::entities_mut` #4093

[Merged by Bors] - unsafeify `World::entities_mut` #4093

BoxyUwU commented Mar 4, 2022 •

edited

Loading

mockersf Mar 4, 2022 •

edited

Loading