rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.01k stars 12.68k forks source link

Arguments of an impl method may have stronger implied bounds than trait method #105295

Closed adamritter closed 11 months ago

adamritter commented 1 year ago

I tried this code:

use std::{cell::RefCell};

pub struct MessageListeners<'a> {
    listeners: RefCell<Vec<Box<dyn FnMut(()) + 'a>>>
}

impl<'a> MessageListeners<'a> {
    pub fn new()->Self {
        MessageListeners { listeners: RefCell::new(Vec::new()) }
    }
    pub fn listen(&self, f: impl FnMut(())+'a) {
        self.listeners.borrow_mut().push(Box::new(f));
    }
    pub fn send(&self, message: ()) {
        for listener in self.listeners.borrow_mut().iter_mut() {
            (*listener)(message.clone());
        }
    }
    pub fn map(&self, f: impl Fn(())->())->MessageListeners<'a> {
        let r = MessageListeners::new();
        self.listeners().listen(|m|  r.send(f(m)));
        r
    }
}

pub trait MessageListenersInterface {
    fn listeners(&self)->&MessageListeners;
}

impl<'a> MessageListenersInterface for MessageListeners<'a> {
    fn listeners<'b>(&'b self)->&'a MessageListeners<'b> {
        self
    }
}

#[test]
fn test_message_listeners_map0() {
    let ml = MessageListeners::new();
    let f = |m: ()| {};
    let g = |m: ()| m;
    ml.map(g).listen(f);
    ml.send(());
}

#[test]
fn test_message_listeners_map2() {
    let ml = MessageListeners::new();
    let f = |m: ()| {};
    let g = |m: ()| m;
    let temp_variable=ml.map(g);
    temp_variable.listen(f);
    ml.send(());
}

I expected to see this happen: the two tests do the same thing

Instead, this happened: the first unit test SEG faults, the second succeeds

Meta

rustc --version --verbose:

rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: aarch64-apple-darwin
release: 1.65.0
LLVM version: 15.0.0```

<!--
Include a backtrace in the code block by setting `RUST_BACKTRACE=1` in your
environment. E.g. `RUST_BACKTRACE=1 cargo build`.
-->
<details><summary>Backtrace</summary>
<p>

Caused by: process didn't exit successfully: /Users/adamritter/Documents/GitHub/synced_collection/target/debug/deps/synccollection-9d89a83a383169be (signal: 11, SIGSEGV: invalid memory reference)```

frxstrem commented 1 year ago

MIRI claims undefined behavior here:

error: Undefined Behavior: constructing invalid value: encountered a dangling reference (use-after-free)
  --> src/lib.rs:21:38
   |
21 |         self.listeners().listen(|m|  r.send(f(m)));
   |                                      ^^^^^^^^^^^^ 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 closure at src/lib.rs:21:38
   = note: inside `<std::boxed::Box<dyn std::ops::FnMut(())> as std::ops::FnMut<((),)>>::call_mut` at /Users/cognite/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:2016:9
note: inside `MessageListeners::<'_>::send` at src/lib.rs:16:13
  --> src/lib.rs:16:13
   |
16 |             (*listener)(message.clone());
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `test_message_listeners_map0` at src/lib.rs:43:5
  --> src/lib.rs:43:5
   |
43 |     ml.send(());
   |     ^^^^^^^^^^^
note: inside closure at src/lib.rs:38:34
  --> src/lib.rs:38:34
   |
37 | #[test]
   | ------- in this procedural macro expansion
38 | fn test_message_listeners_map0() {
   |                                  ^
   = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

Given that this code only consists of safe code, that smells like a compiler bug.

jruderman commented 1 year ago

First crashes in nightly-2020-10-07.

Bisection details I used: ```bash cargo-bisect-rustc --preserve --script ./unsound.sh --start 2020-01-01 --end 2020-12-05 ``` With unsound.sh containing: ```bash ! cargo test 2>&1 | grep "SEGV" ```

Earlier versions report a lifetime error:

Lifetime error reported with nightly-2020-10-06 ``` error[E0308]: method not compatible with trait --> src/main.rs:35:5 | 35 | fn listeners<'b>(&'b self)->&'a MessageListeners<'b> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lifetime mismatch | = note: expected fn pointer `fn(&MessageListeners<'a>) -> &MessageListeners<'_>` found fn pointer `fn(&MessageListeners<'a>) -> &'a MessageListeners<'_>` note: the lifetime `'a` as defined on the impl at 34:6... --> src/main.rs:34:6 | 34 | impl<'a> MessageListenersInterface for MessageListeners<'a> { | ^^ note: ...does not necessarily outlive the anonymous lifetime #1 defined on the method body at 35:5 --> src/main.rs:35:5 | 35 | fn listeners<'b>(&'b self)->&'a MessageListeners<'b> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ```

Was that lifetime error correct?

apiraino commented 1 year ago

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

RustyYato commented 1 year ago

Was that lifetime error correct?

Yes, the lifetime error is correct fn(&self) -> &'a _ is incompatible with the definition in the trait: fn(&self) -> &_.

compiler-errors commented 1 year ago

Looks like it may be a bug in compare_predicate_entailment (trait<=>impl method compatibility checking). I'll take a look at this today, and if I can't find a root cause + fix in a few hours, I'll give it back.

@rustbot claim

compiler-errors commented 1 year ago
found 6 bors merge commits in the specified range
  commit[0] 2020-10-05UTC: Auto merge of #77080 - richkadel:llvm-coverage-counters-2, r=tmandry
  commit[1] 2020-10-06UTC: Auto merge of #77606 - JohnTitor:rollup-7rgahdt, r=JohnTitor
  commit[2] 2020-10-06UTC: Auto merge of #77594 - timvermeulen:chain_advance_by, r=scottmcm
  commit[3] 2020-10-06UTC: Auto merge of #73905 - matthewjasper:projection-bounds-2, r=nikomatsakis
  commit[4] 2020-10-06UTC: Auto merge of #76356 - caass:hooks, r=jyn514
  commit[5] 2020-10-06UTC: Auto merge of #77386 - joshtriplett:static-glibc, r=petrochenkov

Almost certainly due to #73905.

compiler-errors commented 1 year ago

The issue is minimized to this basically:

pub struct Listeners<'a> {
    listeners: RefCell<Vec<Box<dyn FnMut(()) + 'a>>>
}

pub trait ListenersInterface {
    fn listeners<'b>(&'b self) -> &'b Listeners<'b>;
}

impl<'a> ListenersInterface for Listeners<'a> {
    fn listeners<'b>(&'b self) -> &'a Listeners<'b> {
        self
    }
}

The fact that the impl compiles is incorrect, I think.

Basically, for this to be sound, given the assumptions of the impl + the assumptions from the trait's method's where clauses (and assuming the trait's method's types are well-formed), we should be able to prove that the impl's method's types are well-formed.

This boils down to: given WellFormed(&'b Listeners<'a>) + WellFormed(&'b Listeners<'b>) (i.e. given 'a: 'b), proving that WellFormed(&'a Listeners<'b>) (i.e. 'b: 'a), which is not possible.

aliemjay commented 1 year ago

edit: Oh it's already a runtime error. I thought it was an ICE :eyes:.

An exploit of this bug for an actual ub:

trait Trait {
    fn get<'s>(s: &'s str, _: &'static &'static ()) -> &'static str;
}
impl Trait for () {
    fn get<'s>(s: &'s str, _: &'static &'s ()) -> &'static str {
        s
    }
}
fn main() {
    let val = <() as Trait>::get(&String::from("blah blah blah"), &&());
    println!("{}", val);
}
albertlarsan68 commented 1 year ago

An exploit of this bug for an actual ub:

trait Trait {
    fn get<'s>(s: &'s str, _: &'static &'static str) -> &'static str;
}
impl Trait for () {
    fn get<'s>(s: &'s str, _: &'static &'s str) -> &'static str {
        s
    }
}
fn main() {
    let val = <() as Trait>::get(&String::from("blah blah blah"), &"");
    println!("{}", val);
}

This reminds me of #25860

apiraino commented 1 year ago

Visiting during T-compiler meeting, reprioritizing.

@rustbot label -P-critical +P-high

lcnr commented 11 months ago

closing as a duplicate of #80176, currently have a deny by default future-compat lint here. We'll convert that lint to a hard error soon-ish