Skip to content

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

Closed
BoxyUwU wants to merge 1 commit intobevyengine:mainfrom
BoxyUwU:unsoundnessagainlol
Closed

[Merged by Bors] - Fix unsound lifetime annotation on Query::get_component#2964
BoxyUwU wants to merge 1 commit intobevyengine:mainfrom
BoxyUwU:unsoundnessagainlol

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

+1 more reviewer

@TheRawMeatball TheRawMeatball TheRawMeatball approved these changes

Reviewers whose approvals may not affect merge requirements

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