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

Unconstrained lifetime in associated type using trait object leads to unsound lifetime extension and ICE #130347

Closed
konnorandrews opened this issue Sep 14, 2024 · 13 comments · Fixed by #130367
Assignees
Labels
A-trait-objects Area: trait objects, vtable layout C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ 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

@konnorandrews
Copy link

I was playing around with defining higher ranked types using trait objects and tried the following pattern.

trait A<T>: B<T = T> {}

trait B {
    type T;
}

To my surprise trait objects of the form dyn for<'a> A<&'a T> were allowed. After some experimentation I found that this leads to UB in safe rust.

trait A<T>: B<T = T> {}

trait B {
    type T;
}

struct Erase<T: ?Sized + B>(T::T);

fn main() {
    let x = {
        let x = String::from("hello");
        
        Erase::<dyn for<'a> A<&'a _>>(x.as_str())
    };
    
    dbg!(x.0);
}
[src/main.rs:16:5] x.0 = "\0\0\0\0\0"

playground

This is likely related to 25860 somehow, but I haven't seen this flavor before.

Meta

This effects all versions from latest nightly to 1.33.0 (on 1.32.0 it causes a ICE, and before it wasn't allowed).

@konnorandrews konnorandrews added the C-bug Category: This is a bug. label Sep 14, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 14, 2024
@konnorandrews
Copy link
Author

@rustbot labels +I-unsound

@rustbot rustbot added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Sep 14, 2024
@BoxyUwU BoxyUwU added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 14, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 14, 2024
@fmease fmease added the A-trait-objects Area: trait objects, vtable layout label Sep 14, 2024
@theemathas
Copy link
Contributor

Another variant:

#![feature(type_alias_impl_trait)]

trait A<T>: B<T = T> {}

trait B {
    type T;
}

struct Erase<T: ?Sized + B>(T::T);

type Alias = impl for<'a> A<&'a str> + ?Sized;

fn conjure() -> &'static dyn for<'a> A<&'a str> {
    unimplemented!()
}

fn dummy() -> &'static Alias {
    conjure()
}

fn main() {
    let x = {
        let x = String::from("hello");
        
        Erase::<Alias>(x.as_str())
    };
    
    dbg!(x.0);
}

@konnorandrews
Copy link
Author

With more testing the struct type

struct Erase<T: ?Sized + B>(T::T);

is important for this issue to be seen. Rustc seems to not have a consistent lifetime given to the .0 field so it's free to unify with whatever it needs to.

@konnorandrews
Copy link
Author

konnorandrews commented Sep 14, 2024

Obligatory lifetime transmute example.

fn make_static<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    trait A<T>: B<T = T> {}
    
    trait B {
        type T;
    }
    
    struct Erase<T: ?Sized + B>(T::T);

    Erase::<dyn for<'c> A<&'c T>>(x).0
}

fn main() {
    let s = {
        make_static(String::from("hello").as_str())
    };
    
    dbg!(s);
}

@theemathas
Copy link
Contributor

A variant that doesn't use the Erase struct

trait A<T>: B<T = T> {}

trait B {
    type T;
}

fn foo(a: <dyn for<'a> A<&'a str> as B>::T) -> &'static str {
    a
}

fn main() {
    let x = foo(String::from("hello").as_str());
    dbg!(x);
}

@theemathas
Copy link
Contributor

Also works the other way around:

trait A<T>: B<T = T> {}

trait B {
    type T;
}

fn foo<'b>(a: &'b str) -> <dyn for<'a> A<&'a str> as B>::T {
    a
}

fn main() {
    let x = foo(String::from("hello").as_str());
    dbg!(x);
}

@konnorandrews
Copy link
Author

The function seems to play the same role of allowing rustc a space to have an undefined lifetime it can assign to anything.

@konnorandrews
Copy link
Author

More experimentation resulted in a ICE.

trait A<'a, T: 'a>: B<T, T = &'a T> {
    
}

trait B<U> {
    type T;
}

type Magic<T> = <dyn for<'a> A<'a, T> as B<T>>::T;

fn make_static<'b, T: ?Sized>(a: Magic<T>) -> &'b T {
    a
}

fn main() {
    let x = make_static(String::from("hello").as_str());
    dbg!(x);
}
note: no errors encountered even though delayed bugs were created

note: those delayed bugs will now be shown as internal compiler errors

error: internal compiler error: error performing operation: fully_perform
  --> src/main.rs:16:13
   |
16 |     let x = make_static(String::from("hello").as_str());
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: delayed at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/compiler/rustc_trait_selection/src/traits/query/type_op/custom.rs:85:25 - disabled backtrace
  --> src/main.rs:16:13
   |
16 |     let x = make_static(String::from("hello").as_str());
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.81.0 (eeb90cda1 2024-09-04) running on x86_64-unknown-linux-gnu

note: compiler flags: --crate-type bin -C embed-bitcode=no -C codegen-units=1 -C debuginfo=2

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack

@konnorandrews
Copy link
Author

@rustbot labels +I-ICE

@rustbot rustbot added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Sep 14, 2024
@konnorandrews
Copy link
Author

An ICE appears to happen on every version after 1.65.0, but with different messages and locations for almost every version.

@konnorandrews konnorandrews changed the title Unconstrained lifetime in associated type leads to unsound lifetime extension Unconstrained lifetime in associated type using trait object leads to unsound lifetime extension and ICE Sep 14, 2024
@matthiaskrgr
Copy link
Member

The ice is very old, see #103899

@compiler-errors
Copy link
Member

I took a quick look at this, and I've got the vague idea of a check to close this soundness hole. I'll put it up for crater soon.

@compiler-errors compiler-errors self-assigned this Sep 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 14, 2024
… r=<try>

Check elaborated projections from dyn don't mention unconstrained late bound lifetimes

Check that the projections that are *not* explicitly written but which we deduce from elaborating the principal of a `dyn` *also* do not reference unconstrained late-bound lifetimes, just like the ones that the user writes by hand.

Fixes rust-lang#130347
@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 Sep 16, 2024
@bors bors closed this as completed in 9510c73 Oct 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 5, 2024
Rollup merge of rust-lang#130367 - compiler-errors:super-unconstrained, r=spastorino

Check elaborated projections from dyn don't mention unconstrained late bound lifetimes

Check that the projections that are *not* explicitly written but which we deduce from elaborating the principal of a `dyn` *also* do not reference unconstrained late-bound lifetimes, just like the ones that the user writes by hand.

That is to say, given:

```
trait Foo<T>: Bar<Assoc = T> {}

trait Bar {
    type Assoc;
}
```

The type `dyn for<'a> Foo<&'a T>` (basically) elaborates to `dyn for<'a> Foo<&'a T> + for<'a> Bar<Assoc = &'a T>`[^1]. However, the `Bar` projection predicate is not well-formed, since `'a` must show up in the trait's arguments to be referenced in the term of a projection. We must error in this situation[^well], or else `dyn for<'a> Foo<&'a T>` is unsound.

We already detect this for user-written projections during HIR->rustc_middle conversion, so this largely replicates that logic using the helper functions that were already conveniently defined.

---

I'm cratering this first to see the fallout; if it's minimal or zero, then let's land it as-is. If not, the way that this is implemented is very conducive to an FCW.

---

Fixes rust-lang#130347

[^1]: We don't actually elaborate it like that in rustc; we only keep the principal trait ref `Foo<&'a T>` and the projection part of `Bar<Assoc = ...>`, but it's useful to be a bit verbose here for the purpose of explaining the issue.
[^well]: Well, we could also make `dyn for<'a> Foo<&'a T>` *not* implement `for<'a> Bar<Assoc = &'a T>`, but this is inconsistent with the case where the user writes `Assoc = ...` in the type itself, and it overly complicates the implementation of trait objects' built-in impls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-objects Area: trait objects, vtable layout C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ 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
8 participants