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

System::run does not validate which world it is being run on #4363

Closed
alice-i-cecile opened this issue Mar 30, 2022 · 2 comments
Closed

System::run does not validate which world it is being run on #4363

alice-i-cecile opened this issue Mar 30, 2022 · 2 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

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 30, 2022

the "safe" System::run doesn't validate world.
Obviously thats not good. But I'd prefer to solve the problem holistically / leave the current pattern in-tact. I think validating world at the System::run_unsafe level (and updating the _unchecked_manual safety docs) has my preference here, given that we are embracing the "just run a system" pattern.

This is blocking #4090, as otherwise the API exposed there will be similarly unsound.

Simple example (compiles on main, but not 0.6) demonstrating UB:

use bevy::prelude::*;

struct A;
#[derive(Debug)]
struct B(i8);

fn main() {
    let mut world_1 = World::new();
    let mut world_2 = World::new();

    // Making sure that the memory layout of our worlds differ
    world_1.insert_resource(A);
    world_1.insert_resource(B(1));

    // Note that this test *succeeds* if the order is swapped
    world_2.insert_resource(B(2));
    world_2.insert_resource(A);

    let mut test_system = IntoSystem::into_system(hello_cursed_world);

    // Playing nice
    test_system.initialize(&mut world_1);
    test_system.run((), &mut world_1);
    // Oh no...
    // This should always panic, but instead crashes if the memory is wrong with
    // STATUS_ACCESS_VIOLATION
    test_system.run((), &mut world_2);
}

fn hello_cursed_world(world: &World, b: Res<B>) {
    let world_id = world.id();
    dbg!(b);
    println!("Hello, I'm running on {world_id:?}!");
}
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention labels Mar 30, 2022
@maniwani
Copy link
Contributor

maniwani commented Mar 30, 2022

Worth mentioning that #4115 includes this change. (check is in run tho, not run_unsafe)

@james7132
Copy link
Member

This seems to be addressed by #4115. Though, as mentioned, it does not do the check in run__unsafe, which should probably be mentioned in the docs.

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

No branches or pull requests

3 participants