Bug: nested closure outlives borrowed value.

[alexander-irbis]

extern crate futures;
extern crate tokio_core;

use futures::{future, Future};
use tokio_core::reactor::Core;

pub trait Action {
    type Output: Future<Item = (), Error = ()>;

    fn run(self) -> Self::Output;
}

impl<T: Future<Item=(), Error=()>, F: FnOnce() -> T> Action for F {
    type Output = T;

    fn run(self) -> Self::Output {
        self()
    }
}

fn retry<A: Action>(action: A) -> impl Future<Item = (), Error = ()> {
    action.run()
}

/*

The `lazy` closure avoids the check of its lifetime here, if:
- the `lazy` closure is nested into the `action` closure, and
- the `action` closure is passed into the `retry` function, and
- the `retry` function take a generic by the `Action` trait argument, and
- the `Action` trait is implemented for an `Fn*` trait.

As a result, we get arbitrary values in variables and at best SIGSEGV.

*/

fn main() {
    let mut core = Core::new().unwrap();
    let handle = core.handle();

    for i in &[1, 2, 3, 4, 5] {
        println!("outer: {}", i);

        let f = move || {
            println!("inner: {}", i);
            future::ok::<(), ()>(())
        };

        let action = move || {
            future::lazy(|| { // The `lazy` closure
                f()
            })
        };
        handle.spawn(retry(action))
    }

    core.run(future::empty::<(), ()>()).expect("Core::run");
}

(Playground)

Output:

outer: 1
outer: 2
outer: 3
outer: 4
outer: 5
inner: 1027752016
inner: 1027752016
inner: 1027752016
inner: 1027752016
inner: 1027752016

Errors:

   Compiling playground v0.0.1 (file:///playground)
    Finished dev [unoptimized + debuginfo] target(s) in 2.29s
     Running `target/debug/playground`
/root/entrypoint.sh: line 8:     8 Killed                  timeout --signal=KILL ${timeout} "$@"

This emits a warning in the 2018 edition mode, but silently accepts code leading to UB in the 2015 edition.

[hellow554]

Can you provide a even smaller example at best without any external dependy? ❤️ (that's a very good bug you found!)

[alexander-irbis]

pub trait Future {
    fn run(self);
}

impl<F> Future for F where F: FnOnce() {
    fn run(self) {
        self();
    }
}

pub trait Action {
    type Output: Future;
    fn run(self) -> Self::Output;
}

impl<T: Future, F: FnOnce() -> T> Action for F {
    type Output = T;
    fn run(self) -> Self::Output {
        self()
    }
}

fn retry<A: Action>(action: A) -> impl Future {
    action.run()
}

struct Core<F: Future> {
    vec: Vec<F>,
}

impl<F: Future> Core<F> {
    pub fn spawn(&mut self, f: F) where F: Future + 'static {
        self.vec.push(f);
    }

    pub fn run(self) {
        for f in self.vec.into_iter() {
            f.run()
        };
    }
}

/*
The `nested` closure avoids the check of its lifetime here, if:
- the `nasted` closure is nested into the `action` closure, and
- the `action` closure is passed into the `retry` function, and
- the `retry` function take a generic by the `Action` trait argument, and
- the `Action` trait is implemented for an `Fn*` trait.

As a result, we get arbitrary values in variables and at best SIGSEGV.
*/
fn main() {
    let mut core = Core { vec: Vec::new() };
    for i in &[1, 2, 3, 4, 5] {
        println!("outer: {}", i);
        let f = move || {
            println!("inner: {}", i);
        };
        let action = move || {
            || f() // The `nested` closure
        };
        core.spawn(retry(action));
    }
    core.run();
}

(Playground)

Output:

outer: 1
outer: 2
outer: 3
outer: 4
outer: 5

Errors:

   Compiling playground v0.0.1 (file:///playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.72s
     Running `target/debug/playground`
timeout: the monitored command dumped core
/root/entrypoint.sh: line 8:     7 Segmentation fault      timeout --signal=KILL ${timeout} "$@"

[earthengine]

Simplified:

pub trait Action {
    type Output: FnOnce();
    fn run(self) -> Self::Output;
}

impl<T: FnOnce(), F: FnOnce() -> T> Action for F {
    type Output = T;
    fn run(self) -> Self::Output {
        self()
    }
}

fn retry<A: Action>(action: A) -> impl FnOnce() {
    action.run()
}
//Note: when specifying the associate type explicitly, the borrow checker complained.
//fn retry<F:FnOnce(),A: Action<Output=F>>(action: A) -> impl FnOnce() {
//    action.run()
//}

/*
The `nested` closure avoids the check of its lifetime here, if:
- the `nasted` closure is nested into the `action` closure, and
- the `action` closure is passed into the `retry` function, and
- the `retry` function take a generic by the `Action` trait argument, and
- the `Action` trait is implemented for an `Fn*` trait.

As a result, we get arbitrary values in variables and at best SIGSEGV.
*/
fn main() {
    let mut core = Vec::new();
    for i in &[1, 2, 3, 4, 5] {
        println!("outer: {}", i);
        let f = move || {
            println!("inner: {}", i);
        };
        let action = move || {
            || f() // The `nested` closure
        };
        core.push(retry(action));
    }
    (core.remove(0)())
}

The above prints random numbers (no segfault).

[alexander-irbis]

The above prints random numbers (no segfault).

@earthengine This is how an undefined behavior works. It depends on many factors.
Segfault is a good luck, because you can notice an error immediately and don't use the wrong data in a sensetive environment.
Random numbers or even worse: plausible numbers, are the worst, you can get. It makes the machine an insideous enemy who will strike at the most inopportune moment...

[hellow554]

So, what should we do about this one? Do we just wait until it will become a hard fault in X relaeses? Or should it get fixed?

[oli-obk]

It is already a future incompat warning on the edition. We'll get NLL on the 2015 editon at some point, too. So I presume we'll just wait until then.

[alexander-irbis]

Also there is a way to use NLL checks with the nightly, it's enough to catch such bugs.
https://internals.rust-lang.org/t/help-us-get-non-lexical-lifetimes-nll-over-the-finish-line/7807/7

[pnkfelix]

NLL (migrate mode) is enabled in all editions as of PR #59114.

The only policy question is that, under migrate mode, we only emit a warning on this (unsound) test case. Therefore, I am not 100% sure whether we should close this until that warning has been turned into a hard-error under our (still in development) future-compatibility lint policy.