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.

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

Bug: nested closure outlives borrowed value. #53432

Closed
alexander-irbis opened this issue Aug 16, 2018 · 8 comments · Fixed by #64221
Closed

Bug: nested closure outlives borrowed value. #53432

alexander-irbis opened this issue Aug 16, 2018 · 8 comments · Fixed by #64221
Labels
A-borrow-checker Area: The borrow checker A-closures Area: Closures (`|…| { … }`) A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.

Comments

@alexander-irbis
Copy link

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.

@matthewjasper matthewjasper added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Aug 16, 2018
@hellow554
Copy link
Contributor

hellow554 commented Aug 17, 2018

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

@alexander-irbis
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

oli-obk commented Dec 5, 2018

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
Copy link
Author

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
Copy link
Member

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.

@jonas-schievink jonas-schievink added A-closures Area: Closures (`|…| { … }`) C-bug Category: This is a bug. A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) labels Sep 15, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 25, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019
…hewjasper

 Rust 2015: No longer downgrade NLL errors

As per decision on a language team meeting as described in rust-lang#63565 (comment), in Rust 2015, we refuse to downgrade NLL errors, that AST borrowck accepts, into warnings and keep them as hard errors.

The remaining work to throw out AST borrowck and adjust some tests still remains after this PR.

Fixes rust-lang#38899
Fixes rust-lang#53432
Fixes rust-lang#45157
Fixes rust-lang#31567
Fixes rust-lang#27868
Fixes rust-lang#47366

r? @matthewjasper
@bors bors closed this as completed in 3b5fbb6 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-closures Area: Closures (`|…| { … }`) A-NLL Area: Non-lexical lifetimes (NLL) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants