Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

World entity access is not sound #3408

Closed
odanek opened this issue Dec 21, 2021 · 9 comments
Closed

World entity access is not sound #3408

odanek opened this issue Dec 21, 2021 · 9 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention

Comments

@odanek
Copy link

odanek commented Dec 21, 2021

Bevy version

0.5

Operating system & version

Windows 10

What you did

The World entity api allows use after free

use bevy::prelude::*;

struct Name(String);

fn test(world: &mut World) {
    let mut entity = world.spawn();
    entity.insert(Name("Test".to_string()));
    let name = entity.get::<Name>().unwrap();    
    entity.despawn(); // Or entity.remove::<Name>();
    println!("{}", name.0); // This line should be a compile error
}

fn main() {
    App::build()
        .add_system(test.exclusive_system())
        .run();
}

What you expected to happen

The code should not compile

What actually happened

The code compiles without any errors or warnings

@odanek odanek added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Dec 21, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Dec 21, 2021

There's a fix in #3001

@DJMcNab DJMcNab added A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention and removed S-Needs-Triage This issue needs to be labelled labels Dec 21, 2021
@alice-i-cecile
Copy link
Member

Despite that, we may want a smaller, tightly scoped fix to this problem for now.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.6.1 milestone Jan 15, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 15, 2022

This will be fixed by #3685 (the scoped fix mentioned above).

Nope, this fixes #3147 instead.

@cart
Copy link
Member

cart commented Feb 10, 2022

Yup this is pretty heinous. Not pretty, but we could probably quickly fix this by making despawn use a borrow and "internally" invalidating the entity ref?

// we still don't return &mut Self, which makes misusing EntityMut via the "builder pattern" style harder
fn despawn(&mut self) {
  self.invalid = true;
  // despawn here
}

fn insert_bundle<B: Bundle>(&mut self, Bundle) -> &mut Self {
  if self.invalid { panic!() }
  // insert here
}

@cart
Copy link
Member

cart commented Feb 10, 2022

But before rolling with that I'd like to consider #3001 first.

@odanek
Copy link
Author

odanek commented Feb 10, 2022

Yup this is pretty heinous. Not pretty, but we could probably quickly fix this by making despawn use a borrow and "internally" invalidating the entity ref?

This does not fix the remove case though

let name = entity.get::<Name>().unwrap();    
entity.remove::<Name>();
println!("{}", name.0);

@cart
Copy link
Member

cart commented Feb 10, 2022

Groosssss. I suppose we could tighten the lifetimes by tying them to EntityMut's lifetime.

@cart
Copy link
Member

cart commented Feb 10, 2022

Thats what #3001 does.

@odanek
Copy link
Author

odanek commented Feb 10, 2022

Yes, that could work, as far as I can tell.

@bors bors bot closed this as completed in dba7790 Apr 4, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
Fixes bevyengine#3408
bevyengine#3001 also solves this but I dont see it getting merged any time soon so...
# Objective
make bevy ecs a lil bit less unsound

## Solution
make `EntityMut::get_component_mut` return borrows from self instead of `'w`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
Fixes bevyengine#3408
bevyengine#3001 also solves this but I dont see it getting merged any time soon so...
# Objective
make bevy ecs a lil bit less unsound

## Solution
make `EntityMut::get_component_mut` return borrows from self instead of `'w`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants