Observers get a `'static` `Trigger` and that's unsound

SkiFire13 commented 3 weeks ago

Bevy version

0.14.1

What you did

I stored a Trigger<'static, E, ()> in a Local (could also be a static, a component, or anything else that can escape the observer system), and then I read it after the end of the first system call.

use bevy_ecs::event::Event;
use bevy_ecs::observer::Trigger;
use bevy_ecs::system::{In, Local};
use bevy_ecs::world::World;

#[derive(Debug, Event)]
struct E(String);

fn observer(
    In(trigger): In<Trigger<'static, E, ()>>,
    mut last_trigger: Local<Option<Trigger<E, ()>>>,
) {
    println!("Now: {:?}", trigger.event());

    if let Some(last_trigger) = &*last_trigger {
        println!("Before: {:?}", last_trigger.event());
    }

    *last_trigger = Some(trigger);
}

fn main() {
    let mut world = World::new();
    world.observe(observer);
    world.flush();
    world.trigger(E("foo".to_string()));
    world.trigger(E("bar".to_string()));
}

What went wrong

On my system it prints the following:

Now: E("foo")
Now: E("bar")
Before: E("bar")

Before should print E("foo"), not E("bar"), showing that it is actually accessing the same memory location as the new event.

MIRI also reports UB:

error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
  --> E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\mod.rs:42:9
   |
42 |         self.event
   |         ^^^^^^^^^^ constructing invalid value: encountered a dangling reference (use-after-free)
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `bevy_ecs::observer::Trigger::<'_, E>::event` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\mod.rs:42:9: 42:19
note: inside `observer`
  --> src/main.rs:16:34
   |
16 |         println!("Before: {:?}", last_trigger.event());
   |                                  ^^^^^^^^^^^^^^^^^^^^
   = note: inside `<for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer} as std::ops::FnMut<(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'_, E>>, bevy_ecs::system::Local<'_, std::option::Option<bevy_ecs::observer::Trigger<'_, E>>>)>>::call_mut - shim(for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer})` at E:\Programmi\Rust\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:166:5: 166:75
   = note: inside `std::ops::function::impls::<impl std::ops::FnMut<(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'_, E>>, bevy_ecs::system::Local<'_, std::option::Option<bevy_ecs::observer::Trigger<'_, E>>>)> for &mut for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer}>::call_mut` at E:\Programmi\Rust\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:294:13: 294:35
   = note: inside `<Func as bevy_ecs::system::SystemParamFunction<fn(bevy_ecs::system::In<Input>, F0) -> Out>>::run::call_inner::<bevy_ecs::observer::Trigger<'_, E>, (), bevy_ecs::system::Local<'_, std::option::Option<bevy_ecs::observer::Trigger<'_, E>>>, &mut for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer}>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\system\function_system.rs:735:21: 735:42
   = note: inside `<for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer} as bevy_ecs::system::SystemParamFunction<fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'_, E>>, bevy_ecs::system::Local<'_, std::option::Option<bevy_ecs::observer::Trigger<'_, E>>>)>>::run` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\system\function_system.rs:738:17: 738:57
   = note: inside `<bevy_ecs::system::FunctionSystem<fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'_, E>>, bevy_ecs::system::Local<'_, std::option::Option<bevy_ecs::observer::Trigger<'_, E>>>), for<'a, 'b> fn(bevy_ecs::system::In<bevy_ecs::observer::Trigger<'static, E>>, bevy_ecs::system::Local<'a, std::option::Option<bevy_ecs::observer::Trigger<'b, E>>>) {observer}> as bevy_ecs::system::System>::run_unsafe` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\system\function_system.rs:534:19: 534:47
   = note: inside `bevy_ecs::observer::runner::observer_system_runner::<E, ()>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\runner.rs:409:9: 409:42
   = note: inside closure at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\mod.rs:192:13: 200:14
   = note: inside `std::ops::function::impls::<impl std::ops::FnMut<((&bevy_ecs::entity::Entity, &for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)),)> for &mut {closure@bevy_ecs::observer::Observers::invoke<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>::{closure#0}}>::call_mut` at E:\Programmi\Rust\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:294:13: 294:35
   = note: inside closure at E:\Programmi\Rust\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\iter\traits\iterator.rs:815:29: 815:36
   = note: inside closure at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\hashbrown-0.14.5\src\map.rs:4749:13: 4749:27
   = note: inside `hashbrown::raw::inner::RawIterRange::<(bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>))>::fold_impl::<{closure@<hashbrown::map::Iter<'_, bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)> as std::iter::Iterator>::fold<(), {closure@std::iter::Iterator::for_each::call<(&bevy_ecs::entity::Entity, &for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)), &mut {closure@bevy_ecs::observer::Observers::invoke<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>::{closure#0}}>::{closure#0}}>::{closure#0}}, ()>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\hashbrown-0.14.5\src\raw\mod.rs:3880:23: 3880:37
   = note: inside `<hashbrown::raw::inner::RawIter<(bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>))> as std::iter::Iterator>::fold::<(), {closure@<hashbrown::map::Iter<'_, bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)> as std::iter::Iterator>::fold<(), {closure@std::iter::Iterator::for_each::call<(&bevy_ecs::entity::Entity, &for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)), &mut {closure@bevy_ecs::observer::Observers::invoke<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>::{closure#0}}>::{closure#0}}>::{closure#0}}>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\hashbrown-0.14.5\src\raw\mod.rs:4151:18: 4151:58
   = note: inside `<hashbrown::map::Iter<'_, bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)> as std::iter::Iterator>::fold::<(), {closure@std::iter::Iterator::for_each::call<(&bevy_ecs::entity::Entity, &for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)), &mut {closure@bevy_ecs::observer::Observers::invoke<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>::{closure#0}}>::{closure#0}}>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\hashbrown-0.14.5\src\map.rs:4747:9: 4750:11
   = note: inside `<hashbrown::map::Iter<'_, bevy_ecs::entity::Entity, for<'a, 'b> fn(bevy_ecs::world::DeferredWorld<'a>, bevy_ecs::observer::ObserverTrigger, bevy_ecs::bevy_ptr::PtrMut<'b>)> as std::iter::Iterator>::for_each::<&mut {closure@bevy_ecs::observer::Observers::invoke<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>::{closure#0}}>` at E:\Programmi\Rust\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\iter\traits\iterator.rs:818:9: 818:31
   = note: inside `bevy_ecs::observer::Observers::invoke::<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\mod.rs:204:9: 204:61
   = note: inside `bevy_ecs::world::DeferredWorld::<'_>::trigger_observers_with_data::<E, std::array::IntoIter<bevy_ecs::component::ComponentId, 0>>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\world\deferred_world.rs:368:9: 368:76
   = note: inside `bevy_ecs::observer::trigger_event::trigger_event::<E, ()>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\trigger_event.rs:61:13: 66:14
   = note: inside `<bevy_ecs::observer::TriggerEvent<E> as bevy_ecs::world::Command>::apply` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\trigger_event.rs:20:9: 20:72
   = note: inside `bevy_ecs::observer::<impl bevy_ecs::world::World>::trigger::<E>` at E:\Programmi\Rust\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_ecs-0.14.1\src\observer\mod.rs:275:9: 275:56
note: inside `main`
  --> src/main.rs:27:5
   |
27 |     world.trigger(E("bar".to_string()));
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

ItsDoot commented 3 weeks ago

We will likely have to turn ObserverSystem into a duplicate of System entirely, due to #9584.

EDIT: Or we introduce a supertrait of both of them that takes In as a generic parameter.

hymm commented 3 weeks ago

what unsafe code is causing the problem? i.e. where are we missing a safety invariant?

SkiFire13 commented 3 weeks ago

what unsafe code is causing the problem? i.e. where are we missing a safety invariant?

I believe this is the problematic unsafe use.

https://github.com/bevyengine/bevy/blob/5f061ea0086c170853e8a9eb8f1c2e6ece414ef3/crates/bevy_ecs/src/observer/runner.rs#L391-L397

The assumption that "IntoObserverSystem is only implemented for functions starting with for<'a> Trigger<'a>" is wrong, since it is implemented for any IntoSystem<In = Trigger<'static>> and that includes systems starting with In<Trigger<'static>> where the 'static lifetime is exposed.

hymm commented 3 weeks ago

Isn't ObserverSystem taking a fake 'static more the issue here? i.e. couldn't you leak the trigger instead and still cause UB?

SkiFire13 commented 3 weeks ago

Isn't ObserverSystem taking a fake 'static more the issue here?

Yes, that's also part of the issue since it's safe to implement and directly receives a Trigger<'static>, so the same issue can be reimplemented without relying on the SystemParamFunction implementation with the In first parameter. However fixing that will likely involve changing how ObserverSystem is defined, since now the Trigger is expected to be the input of the underlying System and that's required to be 'static (which likely was the orignal reason why the Trigger<'static> was used). The possibilities I see are:

System is somehow changed to support non-'static inputs (I did try this in the past in a local branch and it didn't seem trivial)
ObserverSystem is changed to not require System, and instead has its own implementations (essentially just moving the ones for SystemParamFunction with their first argument being Trigger over to IntoObserverSystem)

i.e. couldn't you leak the trigger instead and still cause UB?

Yes, for example Box::leak(Box::new(trigger)).event() will return a &'static E. Ultimately that's just another way to make the trigger escape the current function lifetime.

bevyengine / bevy