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

compiler-errors · 2024-08-12T17:59:34Z

This PR patches the implied bounds holes slightly for #129005, #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:

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.
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 #115538, to name a few.

One way to make this a bit cleaner is to simply extend #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: #129021 (comment)

r? lcnr

compiler-errors · 2024-08-12T17:59:40Z

@bors try

…=<try> Check WF of source type's signature on fn pointer cast TODO: description r? lcnr

bors · 2024-08-12T18:00:50Z

⌛ Trying commit aa3574c with merge f781a78...

compiler-errors · 2024-08-12T18:06:43Z

tests/ui/implied-bounds/implied-bounds-on-nested-references-plus-variance.rs

@@ -1,15 +1,10 @@
-//@ check-pass
-//@ known-bug: #25860


I guess I should probably rewrite this test to continue being a known-bug?

Just make it higher-ranked in one more arg or sth...

yes, and also add "regression test for X"

bors · 2024-08-12T19:57:26Z

☀️ Try build successful - checks-actions
Build commit: f781a78 (f781a78d815e6a304bf6061b8a8fa69d7ce8dfd5)

compiler-errors · 2024-08-12T21:02:53Z

@craterbot check

craterbot · 2024-08-12T21:03:01Z

👌 Experiment pr-129021 created and queued.
🤖 Automatically detected try build f781a78
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rustbot · 2024-08-13T21:37:18Z

changes to the core type system

cc @compiler-errors, @lcnr

compiler/rustc_trait_selection/src/traits/wf.rs

bors · 2024-08-14T17:43:51Z

☔ The latest upstream changes (presumably #129092) made this pull request unmergeable. Please resolve the merge conflicts.

craterbot · 2024-08-15T16:24:58Z

🚧 Experiment pr-129021 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-08-17T20:21:25Z

🎉 Experiment pr-129021 is completed!
📊 24 regressed and 4 fixed (499461 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2024-08-19T16:30:19Z

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-129021/retry-regressed-list.txt p=1

There seems to be no legitimate regressions, just a bunch of unsound copies of cve-rs. Lots of "no more space on disk" and segfaults that seem normal with crater, but let's give this another pass just to shake those off.

craterbot · 2024-08-19T16:30:24Z

👌 Experiment pr-129021-1 created and queued.
🤖 Automatically detected try build f781a78
⚠️ Try build based on commit aa3574c, but latest commit is 86a3123. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-08-19T19:32:16Z

🚧 Experiment pr-129021-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler/rustc_borrowck/src/type_check/mod.rs

lcnr

r=me after FCP

rfcbot · 2024-09-05T19:32:45Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

compiler-errors · 2024-09-05T19:34:00Z

@bors r=lcnr

bors · 2024-09-05T19:34:03Z

📌 Commit 67804c5 has been approved by lcnr

It is now in the queue for this repository.

… 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

…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#128919 (Add an internal lint that warns when accessing untracked data) - rust-lang#129021 (Check WF of source type's signature on fn pointer cast) - rust-lang#129472 (fix ICE when `asm_const` and `const_refs_to_static` are combined) - rust-lang#129653 (clarify that addr_of creates read-only pointers) - rust-lang#129775 (bootstrap: Try to track down why `initial_libdir` sometimes fails) - rust-lang#129781 (Make `./x.py <cmd> compiler/<crate>` aware of the crate's features) - rust-lang#129939 (explain why Rvalue::Len still exists) - rust-lang#129942 (copy rustc rustlib artifacts from ci-rustc) - rust-lang#129944 (Add compat note for trait solver change) - rust-lang#129947 (Add digit separators in `Duration` examples) - rust-lang#129955 (Temporarily remove fmease from the review rotation) - rust-lang#129957 (forward linker option to lint-docs) - rust-lang#129969 (Make `Ty::boxed_ty` return an `Option`) - rust-lang#129995 (Remove wasm32-wasip2's tier 2 status from release notes) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#129021 (Check WF of source type's signature on fn pointer cast) - rust-lang#129781 (Make `./x.py <cmd> compiler/<crate>` aware of the crate's features) - rust-lang#129963 (Inaccurate `{Path,OsStr}::to_string_lossy()` documentation) - rust-lang#129969 (Make `Ty::boxed_ty` return an `Option`) - rust-lang#129995 (Remove wasm32-wasip2's tier 2 status from release notes) - rust-lang#130013 (coverage: Count await when the Future is immediately ready ) r? `@ghost` `@rustbot` modify labels: rollup

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

`cve-rs` does not compile in nightly due to rust-lang/rust#129021 and this change will be landed in Rust 1.83. This PR fixes it.

rustbot assigned lcnr Aug 12, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Aug 12, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2024

Auto merge of rust-lang#129021 - compiler-errors:ptr-cast-outlives, r…

f781a78

…=<try> Check WF of source type's signature on fn pointer cast TODO: description r? lcnr

compiler-errors commented Aug 12, 2024

View reviewed changes

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2024

compiler-errors force-pushed the ptr-cast-outlives branch from aa3574c to 042df19 Compare August 13, 2024 21:37

compiler-errors commented Aug 13, 2024

View reviewed changes

compiler/rustc_trait_selection/src/traits/wf.rs Outdated Show resolved Hide resolved

compiler-errors force-pushed the ptr-cast-outlives branch from 042df19 to 86a3123 Compare August 13, 2024 21:43

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 17, 2024

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2024

compiler-errors force-pushed the ptr-cast-outlives branch from 86a3123 to 512ae20 Compare August 19, 2024 16:33

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the ptr-cast-outlives branch from 512ae20 to b9acb52 Compare August 19, 2024 17:38

lcnr reviewed Sep 5, 2024

View reviewed changes

compiler/rustc_borrowck/src/type_check/mod.rs Outdated Show resolved Hide resolved

lcnr reviewed Sep 5, 2024

View reviewed changes

compiler-errors force-pushed the ptr-cast-outlives branch from 96da3ab to 6729b8d Compare September 5, 2024 10:36

compiler-errors added 2 commits September 5, 2024 06:37

Check unnormalized signature on pointer cast

e8472e8

Adjust tests

67804c5

compiler-errors force-pushed the ptr-cast-outlives branch from 6729b8d to 67804c5 Compare September 5, 2024 10:37

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 5, 2024

rustbot mentioned this pull request Sep 5, 2024

Tracking issue for release notes of #129021: Check WF of source type's signature on fn pointer cast #130000

Open

3 tasks

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2024

workingjubilee mentioned this pull request Sep 6, 2024

Rollup of 15 pull requests #130007

Closed

workingjubilee mentioned this pull request Sep 6, 2024

Rollup of 14 pull requests #130010

Closed

matthiaskrgr mentioned this pull request Sep 6, 2024

Rollup of 6 pull requests #130016

Merged

bors merged commit e903b29 into rust-lang:master Sep 6, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 6, 2024

usamoi mentioned this pull request Sep 7, 2024

fix expand & expand_mut in nightly Speykious/cve-rs#42

Merged

This was referenced Sep 19, 2024

regression: type annotations needed #130576

Closed

Use equality when relating formal and expected type in arg checking #129317

Merged

apiraino removed the to-announce Announce this issue on triage meeting label Sep 26, 2024

Bright-Shard added a commit to Speykious/cve-rs that referenced this pull request Oct 22, 2024

fix expand & expand_mut in nightly (#42)

80d3a0e

`cve-rs` does not compile in nightly due to rust-lang/rust#129021 and this change will be landed in Rust 1.83. This PR fixes it.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

compiler-errors commented Aug 12, 2024 •

edited

Loading

compiler-errors commented Aug 12, 2024

bors commented Aug 12, 2024

compiler-errors Aug 12, 2024

compiler-errors Aug 12, 2024

lcnr Aug 25, 2024

bors commented Aug 12, 2024

compiler-errors commented Aug 12, 2024

craterbot commented Aug 12, 2024

rustbot commented Aug 13, 2024

bors commented Aug 14, 2024

craterbot commented Aug 15, 2024

craterbot commented Aug 17, 2024

compiler-errors commented Aug 19, 2024 •

edited

Loading

craterbot commented Aug 19, 2024

This comment has been minimized.

craterbot commented Aug 19, 2024

lcnr left a comment

rfcbot commented Sep 5, 2024

compiler-errors commented Sep 5, 2024

bors commented Sep 5, 2024

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

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

Conversation

compiler-errors commented Aug 12, 2024 • edited Loading

What does this do?

Further work

Crater triage

compiler-errors commented Aug 12, 2024

bors commented Aug 12, 2024

compiler-errors Aug 12, 2024

Choose a reason for hiding this comment

compiler-errors Aug 12, 2024

Choose a reason for hiding this comment

lcnr Aug 25, 2024

Choose a reason for hiding this comment

bors commented Aug 12, 2024

compiler-errors commented Aug 12, 2024

craterbot commented Aug 12, 2024

rustbot commented Aug 13, 2024

bors commented Aug 14, 2024

craterbot commented Aug 15, 2024

craterbot commented Aug 17, 2024

compiler-errors commented Aug 19, 2024 • edited Loading

craterbot commented Aug 19, 2024

This comment has been minimized.

craterbot commented Aug 19, 2024

lcnr left a comment

Choose a reason for hiding this comment

rfcbot commented Sep 5, 2024

compiler-errors commented Sep 5, 2024

bors commented Sep 5, 2024

compiler-errors commented Aug 12, 2024 •

edited

Loading

compiler-errors commented Aug 19, 2024 •

edited

Loading