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

function type params are not checked for well-formedness #104005

Closed
aliemjay opened this issue Nov 5, 2022 · 8 comments · Fixed by #120019
Closed

function type params are not checked for well-formedness #104005

aliemjay opened this issue Nov 5, 2022 · 8 comments · Fixed by #120019
Assignees
Labels
A-lifetimes Area: Lifetimes / regions 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 regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@aliemjay
Copy link
Member

aliemjay commented Nov 5, 2022

This unsound program shouldn't compile, but it does: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=2e0af084bc8b2ec9f440a2c5dab7992f

use std::fmt::Display;

trait Displayable {
    fn display(self) -> Box<dyn Display>;
}

impl<T: Display> Displayable for (T, Option<&'static T>) {
    fn display(self) -> Box<dyn Display> {
        Box::new(self.0)
    }
}

fn extend_lt<T, U>(val: T) -> Box<dyn Display>
where
    (T, Option<U>): Displayable,
{
    Displayable::display((val, None))
}

fn main() {
    // The type parameter `U = &'static &'temporary str` is ill-formed
    // because it does not enforce `'temporary: 'static` ...
    let val = extend_lt(&String::from("blah blah blah"));
    println!("{}", val);
}

I was shocked discovering that. I thought WellFormed(U) is part of the caller bounds of extend_lt.

@rustbot label C-bug T-types I-unsound A-lifetimes

@rustbot rustbot added A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. 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. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 5, 2022
@aliemjay
Copy link
Member Author

aliemjay commented Nov 5, 2022

So this is a MIR borrowck bug because it regressed in v1.36 when it was enabled on 2015 edition.
@rustbot label regression-from-stable-to-stable

Remarkably adding type annotations to the call site

-    let val = extend_lt(&String::from("blah blah blah"));
+    let val = extend_lt::<_, &_>(&String::from("blah blah blah"));

makes it not compile because user annotations are always checked for well-formedness.

@rustbot rustbot added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Nov 5, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Nov 5, 2022

cc @lcnr

@aliemjay
Copy link
Member Author

aliemjay commented Nov 5, 2022

A case that regressed with full NLL in v1.63 - it was rejected by the HIR borrowck:

// ... same as above
fn test<'a>() {
    let _: fn(&'a str) -> _ = extend_lt;
}

@aliemjay
Copy link
Member Author

aliemjay commented Nov 5, 2022

I'd argue that this should be rejected as well:

trait Trait<A>: Sized {
    fn call(self) {}
}

impl<T> Trait<&'static T> for T {}

fn main() {
    Trait::call(&String::new());
}

This is necessary to fix #98852 in a sound way. At this point we should basically WF-check all fn item substs.

@rustbot claim

@jackh726 jackh726 added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Nov 6, 2022
@jackh726
Copy link
Member

jackh726 commented Nov 6, 2022

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: This bug is tracked inside the repo by a `known-bug` test. label Apr 25, 2023
@DianQK
Copy link
Member

DianQK commented Aug 15, 2023

Hi, @aliemjay.
Are you still working on this issue? I would like to get started with MIR with this issue.

@aliemjay
Copy link
Member Author

Hi, @aliemjay.
Are you still working on this issue? I would like to get started with MIR with this issue.

Hi, You can claim this. There is #104098, which I was too busy to finish. You can take it over to rebase, address review comments and push for a crater run.

@DianQK
Copy link
Member

DianQK commented Aug 15, 2023

Hi, @aliemjay.
Are you still working on this issue? I would like to get started with MIR with this issue.

Hi, You can claim this. There is #104098, which I was too busy to finish. You can take it over to rebase, address review comments and push for a crater run.

Thanks, I'll give it a try. I added a corresponding question at https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/lifetime-issue-104005 as well.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2023
fix fn item implied bounds and wf check

These are two distinct changes:
1. Wf-check all fn item substs.
Fixes rust-lang#104005

2. Use implied bounds from impl header.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

cc `@lcnr`
r? types
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 9, 2023
fix fn item implied bounds and wf check

These are two distinct changes:
1. Wf-check all fn item substs.
Fixes rust-lang#104005

2. Use implied bounds from impl header.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

cc `@lcnr`
r? types
@lcnr lcnr moved this to new solver in coherence in T-types unsound issues Nov 15, 2023
@lcnr lcnr moved this from new solver in coherence to unblocked in T-types unsound issues Nov 15, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 20, 2023
fix fn/const items implied bounds and wf check

These are two distinct changes (edit: actually three, see below):
1. Wf-check all fn item args. This is a soundness fix.
Fixes rust-lang#104005

2. Use implied bounds from impl header in borrowck of associated functions/consts. This strictly accepts more code and helps to mitigate the impact of other breaking changes.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have a big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

The third change is to to check WF of user type annotations before normalizing them (fixes rust-lang#104764, fixes rust-lang#104763). It is mutually dependent on the second change above: an attempt to land it separately in rust-lang#104746 caused several crater regressions that can all be mitigated by using the implied from the impl header. It is also necessary for the soundness of associated consts that use the implied bounds of impl header. See rust-lang#104763 and how the third commit fixes the soundness issue in `tests/ui/wf/wf-associated-const.rs` that was introduces by the previous commit.

cc `@lcnr`
r? types
@bors bors closed this as completed in 6bf600b Jan 17, 2024
@github-project-automation github-project-automation bot moved this from unblocked to new solver everywhere in T-types unsound issues Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions 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 regression-from-stable-to-stable Performance or correctness regression from one stable version to another. S-bug-has-test Status: This bug is tracked inside the repo by a `known-bug` test. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
5 participants