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

HRTB on subtrait unsoundly provides HTRB on supertrait with weaker implied bounds #84591

Open
steffahn opened this issue Apr 26, 2021 · 6 comments
Labels
A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-bug-has-test Status: A `known-bug` test has been added for this bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Apr 26, 2021

I’m giving an exploitation below at the end of this description. This is my interpretation of where exactly the unsoundness lies:

If I have a trait hierarchy

trait Subtrait<'a, 'b, R>: Supertrait<'a, 'b> {}
trait Supertrait<'a, 'b> {}

then a higher ranked trait bound (HRTB) like this

for<'a, 'b> Subtrait<'a, 'b, &'b &'a ()>

does/should only apply to such lifetimes 'a, 'b that fulfill the outlives-relation 'a: 'b.
('a: 'b is needed for &'b &'a () to be a valid type.)

However, the bound

for<'a, 'b> Subtrait<'a, 'b, &'b &'a ()>

appears to imply the bound

for<'a, 'b> Supertrait<'a, 'b>,

and in this one, the lifetimes 'a and 'b are universally quantified without any implicit outlives-relation.

This implication is demonstrated below:

fn need_hrtb_subtrait<S>()
where
    S: for<'a, 'b> Subtrait<'a, 'b, &'b &'a ()>,
{
    need_hrtb_supertrait::<S>() // <- this call works
}

fn need_hrtb_supertrait<S>()
where
    S: for<'a, 'b> Supertrait<'a, 'b>,
{
}

(playground)

One could of course debate whether for<'a, 'b> Subtrait<'a, 'b, &'b &'a ()> should perhaps in-fact include a for<'a, 'b> Supertrait<'a, 'b> bound, but that seems kind-of weird IMO, and also the following code demonstrates that you only need Supertrait<'a, 'b> with 'a: 'b for a fully generic Subtrait<'a, 'b, &'b &'a ()> implementation:

struct MyStruct;
impl<'a: 'b, 'b> Supertrait<'a, 'b> for MyStruct {}
impl<'a, 'b> Subtrait<'a, 'b, &'b &'a ()> for MyStruct {}

fn main() {
    need_hrtb_subtrait::<MyStruct>();
}

(playground)


Finally, here’s how to turn this into actual UB:

trait Subtrait<'a, 'b, R>: Supertrait<'a, 'b> {}
trait Supertrait<'a, 'b> {
    fn convert<T: ?Sized>(x: &'a T) -> &'b T;
}

fn need_hrtb_subtrait<'a_, 'b_, S, T: ?Sized>(x: &'a_ T) -> &'b_ T
where
    S: for<'a, 'b> Subtrait<'a, 'b, &'b &'a ()>,
{
    need_hrtb_supertrait::<S, T>(x) // <- this call works
}

fn need_hrtb_supertrait<'a_, 'b_, S, T: ?Sized>(x: &'a_ T) -> &'b_ T
where
    S: for<'a, 'b> Supertrait<'a, 'b>,
{
    S::convert(x)
}

struct MyStruct;
impl<'a: 'b, 'b> Supertrait<'a, 'b> for MyStruct {
    fn convert<T: ?Sized>(x: &'a T) -> &'b T {
        x
    }
}
impl<'a, 'b> Subtrait<'a, 'b, &'b &'a ()> for MyStruct {}

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    need_hrtb_subtrait::<MyStruct, T>(x)
}

fn main() {
    let d;
    {
        let x = "Hello World".to_string();
        d = extend_lifetime(&x);
    }
    println!("{}", d);
}
��~��U�

(playground)

This demonstration compiles since Rust 1.7.

@rustbot modify labels: T-compiler, A-traits, A-lifetimes, A-typesystem
and someone please add “I-unsound 💥”.

@steffahn steffahn added the C-bug Category: This is a bug. label Apr 26, 2021
@rustbot rustbot added A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 26, 2021
@steffahn steffahn changed the title HRTBs are unsound: HRTB transfers to subtraits with weaker implied bounds. HRTBs are unsound: HRTB on supertrait provides HTRB on subtrait with weaker implied bounds. Apr 26, 2021
@jonas-schievink jonas-schievink added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Apr 26, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 26, 2021
@jackh726
Copy link
Member

Just wanted to write a couple thoughts:
First,

trait Supertrait<'a, 'b, R>: Subtrait<'a, 'b> {}

is kind of the wrong naming. Here Subtrait is actually a supertrait of Supertrait. Naming is backwards.

Second, I wonder if this might just be solved to make this fn convert<T: ?Sized>(x: &'a T) -> &'b T; not allowed as-is. Theoretically, that shouldn't be WF unless 'a: 'b.

@steffahn
Copy link
Member Author

steffahn commented Apr 26, 2021

Here Subtrait is actually a supertrait of Supertrait. Naming is backwards.

How did I miss this… One moment, let me fix this…… Edit: Done.

I wonder if this might just be solved to make this fn convert<T: ?Sized>(x: &'a T) -> &'b T; not allowed as-is. Theoretically, that shouldn't be WF unless 'a: 'b.

Why should it not be WF? The implementation could just panic!() unconditionally. The only bounds necessary for the type are T: 'a and T: 'b. By the way are you referring to the declaration of the implementation of this method that should be rejected? Edit: Probably referring to the declaration, since the implementation does come with a 'a: 'b bound anyways.

@steffahn steffahn changed the title HRTBs are unsound: HRTB on supertrait provides HTRB on subtrait with weaker implied bounds. HRTBs are unsound: HRTB on subtrait provides HTRB on supertrait with weaker implied bounds. Apr 26, 2021
@JohnTitor
Copy link
Member

Assigning P-critical as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@JohnTitor JohnTitor added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 28, 2021
@pnkfelix pnkfelix added T-lang Relevant to the language team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Apr 29, 2021
@pnkfelix
Copy link
Member

Discussed in T-compiler triage meeting. Added relevant labels. Believed to be known long-standing issue, and something that the traits WG wants to address alongside improvements to trait infrastructure.

But: Not a release blocker, so downgrading to P-high.

@pnkfelix pnkfelix added P-high High priority and removed P-critical Critical priority labels Apr 29, 2021
@nikomatsakis
Copy link
Contributor

Strongly related to #25860

@dtolnay dtolnay changed the title HRTBs are unsound: HRTB on subtrait provides HTRB on supertrait with weaker implied bounds. HRTB on subtrait unsoundly provides HTRB on supertrait with weaker implied bounds Jan 17, 2022
@lcnr
Copy link
Contributor

lcnr commented Jun 30, 2022

here's an example where the trait hierarchy is a bit more problematic.

trait Subtrait<T>: Supertrait {}
trait Supertrait {
    fn action(self);
}

fn subs_to_soup<T, U>(x: T)
where
    T: Subtrait<U>,
{
    soup(x)
}

fn soup<T: Supertrait>(x: T) {
    x.action();
}

impl<'a, 'b: 'a> Supertrait for (&'b str, &mut &'a str) {
    fn action(self) {
        *self.1 = self.0;
    }
}

impl<'a, 'b> Subtrait<&'a &'b str> for (&'b str, &mut &'a str) {}

fn main() {
    let mut d = "hi";
    {
        let x = "Hello World".to_string();
        subs_to_soup((x.as_str(), &mut d));
    }
    println!("{}", d);
}

we should require trait solving to prove wf for the types it creates while matching impls ✨

@oli-obk oli-obk added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Jul 15, 2022
@Nilstrieb Nilstrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 23, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? `@jackh726`

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 24, 2023
…unsound-issues, r=jackh726

Add `known-bug` tests for 11 unsound issues

r? ``@jackh726``

Should tests for other issues be in separate PRs?  Thanks.

Edit: Partially addresses rust-lang#105107.  This PR adds `known-bug` tests for 11 unsound issues:
- rust-lang#25860
- rust-lang#49206
- rust-lang#57893
- rust-lang#84366
- rust-lang#84533
- rust-lang#84591
- rust-lang#85099
- rust-lang#98117
- rust-lang#100041
- rust-lang#100051
- rust-lang#104005
@jackh726 jackh726 added the S-bug-has-test Status: A `known-bug` test has been added for this bug. label Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: lifetime related A-traits Area: Trait system A-typesystem Area: The type system C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority S-bug-has-test Status: A `known-bug` test has been added for this bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Status: new solver everywhere
Development

No branches or pull requests

10 participants