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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Fix unsound lifetime annotation on Query::get_component #2964

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix unsound lifetime annotation on Query::get_component #2964

wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Oct 14, 2021

#2605 changed the lifetime annotations on get_component introducing unsoundness as you could keep the returned borrow even after using the query.

Example unsoundness:

use bevy::prelude::*;

fn main() {
    App::new()
        .add_startup_system(startup)
        .add_system(unsound)
        .run();
}

#[derive(Debug, Component, PartialEq, Eq)]
struct Foo(Vec<u32>);

fn startup(mut c: Commands) {
    let e = c.spawn().insert(Foo(vec![10])).id();
    c.insert_resource(e);
}

fn unsound(mut q: Query<&mut Foo>, res: Res<Entity>) {
    let foo = q.get_component::<Foo>(*res).unwrap();
    let mut foo2 = q.iter_mut().next().unwrap();

    let first_elem = &foo.0[0];
    for _ in 0..16 {
        foo2.0.push(12);
    }
    dbg!(*first_elem);
}

output:
[src/main.rs:26] *first_elem = 0

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 14, 2021
@BoxyUwU BoxyUwU changed the title Fix unsoundness in Query::get_component lifetimes Fix unsound lifetime annotation on Query::get_component Oct 14, 2021
@TheRawMeatball TheRawMeatball added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-Critical This must be fixed immediately or contributors or users will be severely impacted and removed S-Needs-Triage This issue needs to be labelled labels Oct 14, 2021
@adsick
Copy link
Contributor

adsick commented Oct 14, 2021

Sorry for stupid question, but how removing 'w lifetime helps here?

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 14, 2021

It turns the function signature from

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> where F::Fetch: FilterFetch {
    pub fn get_component<'a, T: Component>(&'a self, entity: Entity) -> Result<&'w T, QueryComponentError> { ... }
}

into

impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> where F::Fetch: FilterFetch {
    pub fn get_component<'a, T: Component>(&'a self, entity: Entity) -> Result<&'a T, QueryComponentError> { ... }
}

diff:

```rust
 impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> where F::Fetch: FilterFetch {
-    pub fn get_component<'a, T: Component>(&'a self, entity: Entity) -> Result<&'w T, QueryComponentError> { ... }
+    pub fn get_component<'a, T: Component>(&'a self, entity: Entity) -> Result<&'a T, QueryComponentError> { ... }
 }

Previously the result reference was bound to the world ('w) and now it is bound to the query ('a).

@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Oct 14, 2021
TheRawMeatball
TheRawMeatball approved these changes Oct 15, 2021
View reviewed changes
Copy link
Member

@TheRawMeatball TheRawMeatball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 15, 2021
@cart
Copy link
Member

cart commented Oct 15, 2021

bors r+

bors bot pushed a commit that referenced this pull request Oct 15, 2021
@BoxyUwU
Fix unsound lifetime annotation on Query::get_component (#2964)
099386f 
#2605 changed the lifetime annotations on `get_component` introducing unsoundness as you could keep the returned borrow even after using the query.

Example unsoundness:
```rust
use bevy::prelude::*;

fn main() {
    App::new()
        .add_startup_system(startup)
        .add_system(unsound)
        .run();
}

#[derive(Debug, Component, PartialEq, Eq)]
struct Foo(Vec<u32>);

fn startup(mut c: Commands) {
    let e = c.spawn().insert(Foo(vec![10])).id();
    c.insert_resource(e);
}

fn unsound(mut q: Query<&mut Foo>, res: Res<Entity>) {
    let foo = q.get_component::<Foo>(*res).unwrap();
    let mut foo2 = q.iter_mut().next().unwrap();

    let first_elem = &foo.0[0];
    for _ in 0..16 {
        foo2.0.push(12);
    }
    dbg!(*first_elem);
}
```
output:
`[src/main.rs:26] *first_elem = 0`
@bors
Copy link
Contributor

bors bot commented Oct 15, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix unsound lifetime annotation on Query::get_component [Merged by Bors] - Fix unsound lifetime annotation on Query::get_component Oct 15, 2021
@bors bors bot closed this Oct 15, 2021
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 15, 2021

oh god bors is so damn fast on this repo compared to rust-lang/rust..... 😆

@BoxyUwU BoxyUwU mentioned this pull request Mar 17, 2022
Closed
bors bot pushed a commit that referenced this pull request Mar 22, 2022
@BoxyUwU @cart
yeet unsound lifetime annotations on Query methods (#4243)
024d984 
# Objective
Continuation of #2964 (I really should have checked other methods when I made that PR)

yeet unsound lifetime annotations on `Query` methods.
Example unsoundness:
```rust
use bevy::prelude::*;

fn main() {
    App::new().add_startup_system(bar).add_system(foo).run();
}

pub fn bar(mut cmds: Commands) {
    let e = cmds.spawn().insert(Foo { a: 10 }).id();
    cmds.insert_resource(e);
}

#[derive(Component, Debug, PartialEq, Eq)]
pub struct Foo {
    a: u32,
}
pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) {
    dbg!("hi");
    {
        let data: &Foo = query.get(*e).unwrap();
        let data2: Mut<Foo> = query.get_mut(*e).unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.single();
        let data2: Mut<Foo> = query.single_mut();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.get_single().unwrap();
        let data2: Mut<Foo> = query.get_single_mut().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.iter().next().unwrap();
        let data2: Mut<Foo> = query.iter_mut().next().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let mut opt_data: Option<&Foo> = None;
        let mut opt_data_2: Option<Mut<Foo>> = None;
        query.for_each(|data| opt_data = Some(data));
        query.for_each_mut(|data| opt_data_2 = Some(data));
        assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB
    }
    dbg!("bye");
}

```

## Solution
yeet unsound lifetime annotations on `Query` methods

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@BoxyUwU @cart @aevyrie
yeet unsound lifetime annotations on Query methods (bevyengine#4243)
69b1fe9 
# Objective
Continuation of bevyengine#2964 (I really should have checked other methods when I made that PR)

yeet unsound lifetime annotations on `Query` methods.
Example unsoundness:
```rust
use bevy::prelude::*;

fn main() {
    App::new().add_startup_system(bar).add_system(foo).run();
}

pub fn bar(mut cmds: Commands) {
    let e = cmds.spawn().insert(Foo { a: 10 }).id();
    cmds.insert_resource(e);
}

#[derive(Component, Debug, PartialEq, Eq)]
pub struct Foo {
    a: u32,
}
pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) {
    dbg!("hi");
    {
        let data: &Foo = query.get(*e).unwrap();
        let data2: Mut<Foo> = query.get_mut(*e).unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.single();
        let data2: Mut<Foo> = query.single_mut();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.get_single().unwrap();
        let data2: Mut<Foo> = query.get_single_mut().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.iter().next().unwrap();
        let data2: Mut<Foo> = query.iter_mut().next().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let mut opt_data: Option<&Foo> = None;
        let mut opt_data_2: Option<Mut<Foo>> = None;
        query.for_each(|data| opt_data = Some(data));
        query.for_each_mut(|data| opt_data_2 = Some(data));
        assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB
    }
    dbg!("bye");
}

```

## Solution
yeet unsound lifetime annotations on `Query` methods

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@BoxyUwU @cart @ItsDoot
yeet unsound lifetime annotations on Query methods (bevyengine#4243)
9bc894d 
# Objective
Continuation of bevyengine#2964 (I really should have checked other methods when I made that PR)

yeet unsound lifetime annotations on `Query` methods.
Example unsoundness:
```rust
use bevy::prelude::*;

fn main() {
    App::new().add_startup_system(bar).add_system(foo).run();
}

pub fn bar(mut cmds: Commands) {
    let e = cmds.spawn().insert(Foo { a: 10 }).id();
    cmds.insert_resource(e);
}

#[derive(Component, Debug, PartialEq, Eq)]
pub struct Foo {
    a: u32,
}
pub fn foo(mut query: Query<&mut Foo>, e: Res<Entity>) {
    dbg!("hi");
    {
        let data: &Foo = query.get(*e).unwrap();
        let data2: Mut<Foo> = query.get_mut(*e).unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.single();
        let data2: Mut<Foo> = query.single_mut();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.get_single().unwrap();
        let data2: Mut<Foo> = query.get_single_mut().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let data: &Foo = query.iter().next().unwrap();
        let data2: Mut<Foo> = query.iter_mut().next().unwrap();
        assert_eq!(data, &*data2); // oops UB
    }

    {
        let mut opt_data: Option<&Foo> = None;
        let mut opt_data_2: Option<Mut<Foo>> = None;
        query.for_each(|data| opt_data = Some(data));
        query.for_each_mut(|data| opt_data_2 = Some(data));
        assert_eq!(opt_data.unwrap(), &*opt_data_2.unwrap()); // oops UB
    }
    dbg!("bye");
}

```

## Solution
yeet unsound lifetime annotations on `Query` methods

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@TheRawMeatball TheRawMeatball TheRawMeatball approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-Critical This must be fixed immediately or contributors or users will be severely impacted S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
Bevy 0.6
Development

Successfully merging this pull request may close these issues.
6 participants
@BoxyUwU @adsick @bjorn3 @cart @alice-i-cecile @TheRawMeatball