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

RPITITs may imply unsound outlives for late-bound args in signature #133427

Closed
compiler-errors opened this issue Nov 24, 2024 · 3 comments · Fixed by #133428
Closed

RPITITs may imply unsound outlives for late-bound args in signature #133427

compiler-errors opened this issue Nov 24, 2024 · 3 comments · Fixed by #133428
Assignees
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Nov 24, 2024

I tried this code:

use std::sync::Mutex;

static MUTEX: Mutex<Option<&'static str>> = Mutex::new(None);

trait Foo {
    fn foo<'a: 'static>(&self) -> impl Sized;
}

impl Foo for str {
    fn foo<'a: 'static>(&'a self) -> impl Sized + 'a {
        *MUTEX.lock().unwrap() = Some(self);
    }
}

fn call_foo<T: Foo + ?Sized>(s: &T) {
    s.foo();
}

fn main() {
    let s = String::from("hello, world");
    call_foo(s.as_str());
    drop(s);
    println!("> {}", MUTEX.lock().unwrap().unwrap());
}

I expected to see this happen: Compilation failure.

Instead, this happened: Segfault due to UAF

Why?

See comment below.

Meta

rustc --version --verbose:

2024-11-24

Not present on beta or stable.

@compiler-errors compiler-errors added A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-types Relevant to the types team, which will review and decide on the PR/issue. labels Nov 24, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Nov 24, 2024
@compiler-errors compiler-errors self-assigned this Nov 24, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 24, 2024
@compiler-errors
Copy link
Member Author

While it may seem from bisection that this is due to:

It's actually only uncovered by that PR because it flipped the order that we listed opaque lifetimes (which changes what regions we end up remapping in the collect_return_position_impl_trait_in_trait_tys query). Instead, this is due to my stupidity in:

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 24, 2024

Actually, here's an unsoundness that is on stable.

trait MkStatic {
    fn mk_static(self) -> &'static str;
}

impl MkStatic for &'static str {
    fn mk_static(self) -> &'static str { self }
}

trait Foo {
    fn foo<'a: 'static, 'late>(&'late self) -> impl MkStatic;
}

impl Foo for str {
    fn foo<'a: 'static>(&'a self) -> impl MkStatic + 'static {
        self
    }
}

fn call_foo<T: Foo + ?Sized>(t: &T) -> &'static str {
    t.foo().mk_static()
}

fn main() {
    let s = call_foo(String::from("hello, world").as_str());
    println!("> {s}");
}

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 25, 2024
@bors bors closed this as completed in 3e095e8 Nov 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 28, 2024
Rollup merge of rust-lang#133428 - compiler-errors:rpitit-unsound, r=lcnr

Actually use placeholder regions for trait method late bound regions in `collect_return_position_impl_trait_in_trait_tys`

So in rust-lang#113182, I introduced a "diagnostics improvement" in the form of 473c88d, which changes which signature we end up instantiating with placeholder regions and which signature we end up instantiating with fresh region vars so that we have placeholders corresponding to the names of the late-bound regions coming from the *impl*.

However, this is not sound, since now we're essentially no longer proving that *all* instantiations of the trait method are compatible with an instantiation of the impl method, but vice versa (which is weaker).  Let's look at the example `tests/ui/impl-trait/in-trait/do-not-imply-from-trait-impl.rs`:

```rust
trait MkStatic {
    fn mk_static(self) -> &'static str;
}

impl MkStatic for &'static str {
    fn mk_static(self) -> &'static str { self }
}

trait Foo {
    fn foo<'a: 'static, 'late>(&'late self) -> impl MkStatic;
}

impl Foo for str {
    fn foo<'a: 'static>(&'a self) -> impl MkStatic + 'static {
        self
    }
}

fn call_foo<T: Foo + ?Sized>(t: &T) -> &'static str {
    t.foo().mk_static()
}

fn main() {
    let s = call_foo(String::from("hello, world").as_str());
    println!("> {s}");
}
```

To collect RPITITs, we were previously instantiating the trait signature with infer vars (`fn(&'?0 str) -> ?1t` where `?1t` is the variable we use to infer the RPITIT) and the impl signature with placeholders (there are no late-bound regions in that signature, so we just have `fn(&'a str) -> Opaque`).

Equating the signatures works, since all we do is unify `?1t` with `Opaque` and `'?0` with `'a`. However, conceptually it *shouldn't* hold, since this definition is not valid for *all* instantiations of the trait method but just the one where `'0` (i.e. `'late`) is equal to `'a` :(

## So what

This PR effectively reverts 473c88d to fix the unsoundness.

Fixes rust-lang#133427
Also fixes rust-lang#133425, which is actually coincidentally another instance of this bug (but not one that is weaponized into UB, just one that causes an ICE in refinement checking).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. C-bug Category: This is a bug. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
4 participants