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

unchecked region constraints for opaque types in dead code #112417

Open
lcnr opened this issue Jun 8, 2023 · 5 comments
Open

unchecked region constraints for opaque types in dead code #112417

lcnr opened this issue Jun 8, 2023 · 5 comments
Assignees
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. 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-medium Medium priority S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jun 8, 2023

edit: while this tests currently results in an error, the underying issue still exists #112417 (comment)

trait CallMeMaybe<'a, 'b> {
    fn mk() -> Self;
    fn subtype<T>(self, x: &'b T) -> &'a T;
}

struct Foo<'a, 'b: 'a>(&'a (), &'b ());
impl<'a, 'b> CallMeMaybe<'a, 'b> for Foo<'a, 'b> {
    fn mk() -> Self {
        Foo(&(), &())
    }

    fn subtype<T>(self, x: &'b T) -> &'a T {
        x
    }
}

// `foo` must not compile but does.
fn foo<'a, 'b>() -> impl CallMeMaybe<'a, 'b> {
    panic!();
    Foo(&(), &())
}

// The rest is only to exploit that unsoundness.
trait Func {
    type RetTy;
}
impl<F: FnOnce() -> R, R> Func for F {
    type RetTy = R;
}

fn subtype<'a, 'b, F: FnOnce() -> R, R: CallMeMaybe<'a, 'b>, T>(_: F, x: &'b T) -> &'a T {
    let proof = R::mk();
    proof.subtype(x)
}

fn main() {
    let y = subtype(foo, &String::from("Hello World"));
    println!("{y}"); // use after free
}

With the current implementation foo only defines the opaque type during HIR typeck and not in MIR typeck as the defining use is in dead code. HIR typeck does not check regions so we use erased regions instead. This hidden type with erased regions is then used as the actual hidden type of the RPIT because there is no hidden type defined by MIR typeck.

We then never check that the RPIT is well-formed outside of HIR typeck itself, so we never check that the region constraints hold for the opaque type.

@lcnr lcnr added C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. labels Jun 8, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 8, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Jun 8, 2023

this unsoundness seems to not work for TAIT:

#![feature(type_alias_impl_trait)]

trait CallMeMaybe<'a, 'b> {
    fn mk() -> Self;
    fn subtype<T>(self, x: &'b T) -> &'a T;
}

struct Foo<'a, 'b: 'a>(&'a (), &'b ());
impl<'a, 'b> CallMeMaybe<'a, 'b> for Foo<'a, 'b> {
    fn mk() -> Self {
        Foo(&(), &())
    }

    fn subtype<T>(self, x: &'b T) -> &'a T {
        x
    }
}

type Alias<'a, 'b> = impl CallMeMaybe<'a, 'b>;
fn foo<'a, 'b>() -> Alias<'a, 'b> {
    panic!();
    Foo(&(), &())
}


fn subtype<'a, 'b, T>(x: &'b T) -> &'a T {
    let proof = Alias::mk();
    proof.subtype(x)
}

fn main() {
    let y = subtype(&String::from("Hello World"));
    println!("{y}");
}

instead results in

error: unconstrained opaque type
  --> src/main.rs:19:22
   |
19 | type Alias<'a, 'b> = impl CallMeMaybe<'a, 'b>;
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `Alias` must be used in combination with a concrete type within the same module

@apiraino
Copy link
Contributor

apiraino commented Jun 8, 2023

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 Jun 8, 2023
@lqd
Copy link
Member

lqd commented Jun 8, 2023

Regression in nightly-2022-03-31:

#94081 looks like a juicy target.

@wesleywiser wesleywiser added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 6, 2023
@oli-obk oli-obk self-assigned this Oct 11, 2023
@lcnr lcnr moved this to unblocked in T-types unsound issues Nov 15, 2023
@spastorino spastorino self-assigned this Dec 12, 2023
@spastorino spastorino removed their assignment Feb 2, 2024
@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 17, 2024

The OP example doesn't reproduce on the Playground (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d0a7fd2fc4abe6e4ec732c1b6a28bf71). bisect-rustc says 09bc67b

@rustbot rustbot added S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels May 17, 2024
@lcnr
Copy link
Contributor Author

lcnr commented May 17, 2024

the underlying issue still exists: https://rust.godbolt.org/z/qK375cseW it seems to get covered by some changes when checking that opaque types are well-formed though. We should not have 'erased here. DOwngrading to P-medium however.

@lcnr lcnr added P-medium Medium priority and removed E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-high High priority labels May 17, 2024
@spastorino spastorino self-assigned this May 17, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue May 20, 2024
…ode, r=<try>

 Error for RPIT if they are not defined during MIR borrowck

r? `@lcnr`

Fixes rust-lang#112417

There are some changes in tests that we would need to properly review. I've left some comments on each of them.
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. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Status: unblocked
Development

Successfully merging a pull request may close this issue.

8 participants