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

implied bounds from projections in function signatures can be unsound #129005

Open
lcnr opened this issue Aug 12, 2024 · 0 comments
Open

implied bounds from projections in function signatures can be unsound #129005

lcnr opened this issue Aug 12, 2024 · 0 comments
Labels
A-associated-items Area: Associated items (types, constants & functions) 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

@lcnr
Copy link
Contributor

lcnr commented Aug 12, 2024

Related to #100051, but the underlying reason is slightly different. The full fix will be the same however and is blocked on the new trait solver.

trait ToArg<T> {
    type Arg;
}
impl<T, U> ToArg<T> for U {
    type Arg = T;
}

fn extend_inner<'a, 'b>(x: &'a str) -> <&'b &'a () as ToArg<&'b str>>::Arg { x }
fn extend<'a, 'b>(x: &'a str) -> &'b str  {
    (extend_inner as fn(_) -> _)(x)
}

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

Introduced by #99217. The idea of that PR was that we check that the unnormalized function signature is well-formed when calling a function. We previously only checked the normalized signature, causing another unsoundness #98543 by having an associated type which we're able to normalize in the definition, but not the use. Checking that the unnormalized signature is well-formed, without assuming that it is well-formed, resulted in breaking changes. Because of this, we changed it to both check, and assume, that the unnormalized function signature is well-formed. This would be sound, except that we can first cast the function to a function pointer, discarding its unnormalized signature, and then call it.

We may be able to partially fix this by checking that the unnormalized signature is well-formed when casting function definitions to function pointers. This still leaves us with the general set of unsound higher-ranked implied bounds, cc #100051 #84591 #25860, which can only really be fixed by supporting implications in the type systems. cc @rust-lang/types

@lcnr lcnr added A-associated-items Area: Associated items (types, constants & functions) P-high High priority 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 Aug 12, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 12, 2024
@lcnr lcnr moved this to unblocked in T-types unsound issues Aug 12, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 6, 2024
… r=lcnr

Check WF of source type's signature on fn pointer cast

This PR patches the implied bounds holes slightly for rust-lang#129005, rust-lang#25860.

Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.

### What does this do?

This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:

```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'short, T>(x: &'short T) -> &'static T {
    let f: fn(_, &'short T) -> &'static T = foo;
    f(&&(), x)
}
```

In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.

This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.

The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.

### Further work

The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in rust-lang#115538, to name a few.

One way to make this a bit cleaner is to simply extend rust-lang#115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.

### Crater triage

Done here: rust-lang#129021 (comment)

r? lcnr
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 6, 2024
… r=lcnr

Check WF of source type's signature on fn pointer cast

This PR patches the implied bounds holes slightly for rust-lang#129005, rust-lang#25860.

Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.

### What does this do?

This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:

```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'short, T>(x: &'short T) -> &'static T {
    let f: fn(_, &'short T) -> &'static T = foo;
    f(&&(), x)
}
```

In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.

This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.

The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.

### Further work

The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in rust-lang#115538, to name a few.

One way to make this a bit cleaner is to simply extend rust-lang#115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.

### Crater triage

Done here: rust-lang#129021 (comment)

r? lcnr
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 6, 2024
… r=lcnr

Check WF of source type's signature on fn pointer cast

This PR patches the implied bounds holes slightly for rust-lang#129005, rust-lang#25860.

Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.

### What does this do?

This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:

```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'short, T>(x: &'short T) -> &'static T {
    let f: fn(_, &'short T) -> &'static T = foo;
    f(&&(), x)
}
```

In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.

This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.

The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.

### Further work

The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in rust-lang#115538, to name a few.

One way to make this a bit cleaner is to simply extend rust-lang#115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.

### Crater triage

Done here: rust-lang#129021 (comment)

r? lcnr
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 6, 2024
Rollup merge of rust-lang#129021 - compiler-errors:ptr-cast-outlives, r=lcnr

Check WF of source type's signature on fn pointer cast

This PR patches the implied bounds holes slightly for rust-lang#129005, rust-lang#25860.

Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.

### What does this do?

This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:

```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }

fn bad<'short, T>(x: &'short T) -> &'static T {
    let f: fn(_, &'short T) -> &'static T = foo;
    f(&&(), x)
}
```

In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.

This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.

The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.

### Further work

The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in rust-lang#115538, to name a few.

One way to make this a bit cleaner is to simply extend rust-lang#115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.

### Crater triage

Done here: rust-lang#129021 (comment)

r? lcnr
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items (types, constants & functions) 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
Status: new solver everywhere
Development

No branches or pull requests

3 participants