Trait object with non-static lifetime is accepted where static lifetime is expected and required

[VFLashM]

Problem: function is explicitly defined as accepting Box<dyn Fn + 'static> and yet it accepts Box<dyn Fn + 'a> where 'a is definitely less than 'static.

It allows to keep reference to an object past object lifetime and therefore cause a crash.

Sorry for an extremely long example, but issue in question is very fragile and I couldn't reproduce it with smaller code.

use std::cell::RefCell;

// a simple trait with one function accepting two arguments:
// one by mutable reference and another one by value
trait Foo {
    type MutArg; // type of an argument passed by mutable reference
    type ValArg; // type of an argument passed by value

    fn foo(&self, marg: &mut Self::MutArg, varg: Self::ValArg);
}

// ValArg can be a complex function which can be used
// to pass MutArg from parent to child
// note that all trait objects must have 'static lifetime
pub type FnValArg<MutArg> = Box<dyn
    FnOnce(Box<dyn
        FnOnce(&mut MutArg) + 'static
    >) + 'static
>;

// FnFooBox is boxed Foo which ValArg is a function
type FnFooBox<MutArg1, MutArg2> = Box<dyn
    Foo<ValArg=FnValArg<MutArg1>, MutArg=MutArg2>
>;

// FnFoo factory creates boxed FnFoo value on the fly
type FnFooFactory<MutArg1, MutArg2> = Box<dyn
    Fn() -> FnFooBox<MutArg1, MutArg2>
>;

// FactoryFoo is a struct storing factory
// and implementing Foo
struct FactoryFoo<MutArg1, MutArg2> {
    factory: FnFooFactory<MutArg1, MutArg2>,
    // Note: if instead of factory I store `subfoo` directly,
    // bug does not reproduce (i.e. I get lifetime error as expected):
    // subfoo: FnFooBox<MutArg1, MutArg2>,
}

impl<MutArg1, MutArg2> Foo for FactoryFoo<MutArg1, MutArg2> {
    type MutArg = (MutArg1, MutArg2);
    type ValArg = i32; // irrelevant

    fn foo(&self, marg: &mut Self::MutArg, _varg: Self::ValArg) {
        let (marg1, marg2) = marg;
        let subfoo = (self.factory)();
        subfoo.foo(
            marg2,
            // `subfoo.foo` requires `varg` of type `FnValArg`
            // `FnValArg` defined as boxed closure with 'static lifetime
            //
            // this closure captures mutable `marg1`, and therefore
            // has the same lifetime as `marg`,
            // which is obviously less than 'static
            //
            // and yet it is accepted and everything works,
            // which allows `subfoo` to modify `marg1` if desired
            //
            // Note: if I move this expression into local variable
            // bug does not reproduce (i.e. I get lifetime error as expected)
            Box::new(move |subfun| subfun(marg1)),
        );
    }
}

// now let me illustrate that it's possible to cause a crash
// with this lifetime issue:
// idea is to capture reference to marg1 and store it globally

// global variable storing FnValArg, which in turn captures marg1 from FactoryFoo
thread_local! {
    static GLOBAL_FN: RefCell<Option<FnValArg<String>>> = RefCell::new(None);
}

// struct implementing Foo and storing varg
// (i.e. closure with captured `&mut marg1` inside)
// in global variable
struct CrashFnFoo {}

impl Foo for CrashFnFoo {
    type ValArg = FnValArg<String>;
    type MutArg = i32; // irrelevant

    fn foo(&self, _marg: &mut Self::MutArg, varg: Self::ValArg) {
        // store varg function in global variable
        GLOBAL_FN.with(|global_fn| {
            global_fn.replace(Some(varg))
        });
    }
}

fn main() {
    let factory = || -> FnFooBox<String, i32> { Box::new(CrashFnFoo{}) };
    let factory_foo = FactoryFoo { factory: Box::new(factory) };

    {
        let mut marg = (String::from("some data"), 0);
        // this captures `&mut marg.0` into `GLOBAL_FN`
        factory_foo.foo(&mut marg, 0);
    }

    // by now marg is gone, but reference to it is still
    // captured in closure inside of GLOBAL_FN
    // now we just have to access it
    GLOBAL_FN.with(|global_fn| {
        let fn_refcell = RefCell::new(None);
        global_fn.swap(&fn_refcell);

        if let Some(func) = fn_refcell.into_inner() {
            println!("crashing now");
            // modifying marg.0 String, which is long dead by now
            func(Box::new(|marg1: &mut String| {
                marg1.push_str("crash here, please");
            }));
        }
    });
}

I expected this error to happen (and it actually does happen if you change the example even a little bit):

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/lib.rs:45:14
   |
45 |         let (marg1, marg2) = marg;
   |              ^^^^^
   |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the method body at 44:5...
  --> src/lib.rs:44:5
   |
44 | /     fn foo(&self, marg: &mut Self::MutArg, _varg: Self::ValArg) {
45 | |         let (marg1, marg2) = marg;
46 | |         let subfoo = (self.factory)();
47 | |         self.subfoo.foo(
...  |
62 | |         );
63 | |     }
   | |_____^
note: ...so that reference does not outlive borrowed content
  --> src/lib.rs:45:14
   |
45 |         let (marg1, marg2) = marg;
   |              ^^^^^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the expression is assignable
  --> src/lib.rs:61:13
   |
61 |             Box::new(move |subfun| subfun(marg1)),
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected  `std::boxed::Box<(dyn std::ops::FnOnce(std::boxed::Box<(dyn for<'r> std::ops::FnOnce(&'r mut MutArg1) + 'static)>) + 'static)>`
              found  `std::boxed::Box<dyn std::ops::FnOnce(std::boxed::Box<(dyn for<'r> std::ops::FnOnce(&'r mut MutArg1) + 'static)>)>`

Instead this code is silently accepted and causes a segmentation error when executed.

Meta

I tried it both on stable and nightly rust:

$ rustc --version --verbose
rustc 1.45.0-nightly (7ebd87a7a 2020-05-08)
binary: rustc
commit-hash: 7ebd87a7a1e0e21767422e115c9455ef6e6d4bee
commit-date: 2020-05-08
host: x86_64-pc-windows-msvc
release: 1.45.0-nightly
LLVM version: 9.0

$ rustc --version --verbose
rustc 1.43.0 (4fb7144ed 2020-04-20)
binary: rustc
commit-hash: 4fb7144ed159f94491249e86d5bbd033b5d60550
commit-date: 2020-04-20
host: x86_64-pc-windows-msvc
release: 1.43.0
LLVM version: 9.0

[nbdd0121]

Seems to be an issue with type inference. Giving subfoo an explicit type would cause borrow checker to work properly.

[matthewjasper]

Maybe fixed by #71896 ?

[RalfJung]

Wow, nice catch!

[spastorino]

Would be nice to find and mcve ...

@rustbot ping cleanup

[rustbot]

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

[spastorino]

Assigning P-critical as discussed as part of the Prioritization Working Group process and removing I-prioritize.