CFI: Strip auto traits off Virtual calls #122879

maurer · 2024-03-22T13:57:43Z

We already use Instance at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well.

The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through Virtual instances.

This is needed for the "separate machinery" version of my approach to the vtable issues (#122573), because we need to respond differently to a Virtual call to the same type as a non-virtual call, specifically stripping auto traits off the receiver's Self because there isn't a separate vtable for Foo vs Foo + Send.

This would also make a more general underlying mechanism that could be used by rcvalle's proposed drop detection / encoding if we end up using his approach, as we could condition out on the def_id in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

rustbot · 2024-03-22T13:57:51Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-22T13:57:53Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

maurer · 2024-03-22T13:58:43Z

r? workingjubilee

workingjubilee · 2024-03-22T14:41:40Z

@bjorn3 The code says cg_clif uses Instance already, yes? Does it also use it for resolving these kinds of calls/invokes, or just the other reasons? Only asking because it doesn't go through cg_ssa.

workingjubilee · 2024-03-22T17:26:56Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+    ///
+    /// You must provide `instance` if it might affect the alias set of the target.
+    /// This will never be the case for direct calls. It is currently only required
+    /// for `Virtual` calls, but providing it when available will avoid complications.


I'm somewhat concerned about this requiring such "high-context" information. Most people have no idea what the hell this means by "alias set". Is there truly no other way to get the relevant Instance?

The only value we have that is a per-instance argument in this function right now is the fn_ptr, and I don't think the backends generally store instances for those because not all of them have one.

I do think your point about it being high-context whether an instance is necessary is reasonable though. Perhaps it would be better to describe it in terms of things you don't need an Instance for? The reworded comment might read:

You must provide instance unless you are making a non-Virtual direct call or calling a raw fn pointer.

sure, that makes more sense.

bors · 2024-03-22T19:44:40Z

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

workingjubilee · 2024-03-22T20:04:54Z

given #122580 adds an Instance arg anyways I withdraw my remarks about that concern and yeah, basically this looks good to me.

workingjubilee · 2024-03-22T20:08:08Z

cc @saethlin the reason the Instance is an Option is because no monomorphization may actually have been done, right? and it has to be forwarded all the time now?

saethlin · 2024-03-22T20:39:19Z

the reason the Instance is an Option is because no monomorphization may actually have been done, right?

Nope. It's an Option because nobody pressed me on this before 🙈 and I didn't really look into it deeply.

At the bottom of codegen_call_terminator, we can have either an Instance or just a backend function pointer. So I thought oh well in some cases we don't have an Instance so it has to be optional. But upon re-reading the code, I think we do always discover an Instance at some point in this function which is the function we're calling. The backend function pointer is derived from this code, which requires that we have found an Instance before:

rust/compiler/rustc_codegen_ssa/src/mir/block.rs

Lines 997 to 1000 in b3df0d7

    
           'make_args: for (i, arg) in first_args.iter().enumerate() { 
        
               let mut op = self.codegen_operand(bx, &arg.node); 
        
               if let (0, Some(ty::InstanceDef::Virtual(_, idx))) = (i, def) {

workingjubilee · 2024-03-22T21:28:27Z

@saethlin Then it would be correct to simply force all callers to pass Instance without leaving them any option about it?

maurer · 2024-03-22T21:37:53Z

the reason the Instance is an Option is because no monomorphization may actually have been done, right?

Nope. It's an Option because nobody pressed me on this before 🙈 and I didn't really look into it deeply.

At the bottom of codegen_call_terminator, we can have either an Instance or just a backend function pointer. So I thought oh well in some cases we don't have an Instance so it has to be optional. But upon re-reading the code, I think we do always discover an Instance at some point in this function which is the function we're calling. The backend function pointer is derived from this code, which requires that we have found an Instance before:

rust/compiler/rustc_codegen_ssa/src/mir/block.rs

Lines 997 to 1000 in b3df0d7

'make_args: for (i, arg) in first_args.iter().enumerate() {

let mut op = self.codegen_operand(bx, &arg.node);

if let (0, Some(ty::InstanceDef::Virtual(_, idx))) = (i, def) {

I belive that the function pointer case

rust/compiler/rustc_codegen_ssa/src/mir/block.rs

Line 825 in b3df0d7

ty::FnPtr(_) => (None, Some(callee.immediate())),

case (non-virtual indirect call) still doesn't have an instance. The block you identified is the only place llfn is assigned to, but its initial definition can have a Some value in the branch I highlighted. Attempting to unwrap the instance at the end also fails empirically.

saethlin · 2024-03-22T21:41:34Z

Ah, yes, I see. I didn't trace def far enough back up through the function. Bummer, looks like this needs to stay optional.

workingjubilee · 2024-03-22T22:22:34Z

alas.

workingjubilee · 2024-03-22T22:23:36Z

@maurer This needs to land a test that would depend on this to prevent it from randomly regressing, then? I think?

rustbot · 2024-03-22T22:54:29Z

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

cc @rust-lang/project-exploit-mitigations, @rcvalle

maurer · 2024-03-22T22:56:42Z

I added my usage of this onto this PR to make it testable. I changed the name of the PR to reflect that.

In addition to feeding the Instance through, it now uses it in the mangling to remove auto traits from virtual calls. This allows &(dyn Foo + Send) receivers to successfully access &self functions on the Foo trait.

workingjubilee · 2024-03-23T00:44:10Z

Nice!

@bors r+

bors · 2024-03-23T00:44:12Z

📌 Commit 042e05b has been approved by workingjubilee

It is now in the queue for this repository.

…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122460 (Rework rmake support library API) - rust-lang#122698 (Cancel `cargo update` job if there's no updates) - rust-lang#122780 (Rename `hir::Local` into `hir::LetStmt`) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122915 (Delay a bug if no RPITITs were found) - rust-lang#122916 (docs(sync): normalize dot in fn summaries) - rust-lang#122922 (-Zprint-type-sizes: print the types of awaitees and unnamed coroutine locals.) - rust-lang#122927 (Change an ICE regression test to use the original reproducer) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-03-23T16:46:30Z

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

We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances.

Additional trait bounds beyond the principal trait and its implications are not possible in the vtable. This means that if a receiver is `&dyn Foo + Send`, the function will only be expecting `&dyn Foo`. This strips those auto traits off before CFI encoding.

maurer · 2024-03-23T18:35:51Z

Conflict resolved, there was a minor use statement ordering conflict.

maurer · 2024-03-23T18:39:31Z

@rustbot ready

workingjubilee · 2024-03-23T21:08:03Z

@bors r+

bors · 2024-03-23T21:08:04Z

📌 Commit f434c27 has been approved by workingjubilee

It is now in the queue for this repository.

…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

…kingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122460 (Rework rmake support library API) - rust-lang#122797 (Fix compile of wasm64-unknown-unknown target) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122895 (add some ice tests 5xxxx to 9xxxx) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#120419 (Expand sys/os for UEFI) - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122762 (fix typo of endianness) - rust-lang#122797 (Fix compile of wasm64-unknown-unknown target) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122969 (Simplify an iterator search in borrowck diag) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122879 - maurer:callsite-instances, r=workingjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

…ngjubilee CFI: Strip auto traits off Virtual calls We already use `Instance` at declaration sites when available to glean additional information about possible abstractions of the type in use. This does the same when possible at callsites as well. The primary purpose of this change is to allow CFI to alter how it generates type information for indirect calls through `Virtual` instances. This is needed for the "separate machinery" version of my approach to the vtable issues (rust-lang#122573), because we need to respond differently to a `Virtual` call to the same type as a non-virtual call, specifically [stripping auto traits off the receiver's `Self`](rust-lang@54b15b0) because there isn't a separate vtable for `Foo` vs `Foo + Send`. This would also make a more general underlying mechanism that could be used by rcvalle's [proposed drop detection / encoding](rust-lang@edcd1e2) if we end up using his approach, as we could condition out on the `def_id` in the CFI code rather than requiring the generating code to explicitly note whether it was calling drop.

…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#120419 (Expand sys/os for UEFI) - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122762 (fix typo of endianness) - rust-lang#122797 (Fix compile of wasm64-unknown-unknown target) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122969 (Simplify an iterator search in borrowck diag) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned Nadrieril Mar 22, 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. labels Mar 22, 2024

rustbot assigned workingjubilee and unassigned Nadrieril Mar 22, 2024

workingjubilee reviewed Mar 22, 2024

View reviewed changes

maurer force-pushed the callsite-instances branch from 250dd9b to 90e363a Compare March 22, 2024 20:40

rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Mar 22, 2024

maurer changed the title ~~CFI: Use Instance at callsites~~ CFI: Strip auto traits off Virtual calls Mar 22, 2024

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 Mar 23, 2024

workingjubilee mentioned this pull request Mar 23, 2024

Rollup of 10 pull requests #122929

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2024

maurer added 2 commits March 23, 2024 18:30

maurer force-pushed the callsite-instances branch from 042e05b to f434c27 Compare March 23, 2024 18:34

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2024

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 Mar 23, 2024

workingjubilee mentioned this pull request Mar 24, 2024

Rollup of 13 pull requests #122967

Closed

workingjubilee mentioned this pull request Mar 24, 2024

Rollup of 14 pull requests #122973

Closed

This was referenced Mar 24, 2024

Rollup of 9 pull requests #122979

Closed

Rollup of 7 pull requests #122980

Merged

bors merged commit 992aa1e into rust-lang:master Mar 24, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFI: Strip auto traits off Virtual calls #122879

CFI: Strip auto traits off Virtual calls #122879

maurer commented Mar 22, 2024

rustbot commented Mar 22, 2024

rustbot commented Mar 22, 2024

maurer commented Mar 22, 2024

workingjubilee commented Mar 22, 2024 •

edited

Loading

workingjubilee Mar 22, 2024

maurer Mar 22, 2024 •

edited

Loading

workingjubilee Mar 22, 2024 •

edited

Loading

bors commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

saethlin commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

maurer commented Mar 22, 2024 •

edited

Loading

saethlin commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

workingjubilee commented Mar 22, 2024 •

edited

Loading

rustbot commented Mar 22, 2024

maurer commented Mar 22, 2024

workingjubilee commented Mar 23, 2024

bors commented Mar 23, 2024

bors commented Mar 23, 2024

maurer commented Mar 23, 2024

maurer commented Mar 23, 2024

workingjubilee commented Mar 23, 2024

bors commented Mar 23, 2024

CFI: Strip auto traits off Virtual calls #122879

CFI: Strip auto traits off Virtual calls #122879

Conversation

maurer commented Mar 22, 2024

rustbot commented Mar 22, 2024

rustbot commented Mar 22, 2024

maurer commented Mar 22, 2024

workingjubilee commented Mar 22, 2024 • edited Loading

workingjubilee Mar 22, 2024

Choose a reason for hiding this comment

maurer Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

workingjubilee Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

saethlin commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

maurer commented Mar 22, 2024 • edited Loading

saethlin commented Mar 22, 2024

workingjubilee commented Mar 22, 2024

workingjubilee commented Mar 22, 2024 • edited Loading

rustbot commented Mar 22, 2024

maurer commented Mar 22, 2024

workingjubilee commented Mar 23, 2024

bors commented Mar 23, 2024

bors commented Mar 23, 2024

maurer commented Mar 23, 2024

maurer commented Mar 23, 2024

workingjubilee commented Mar 23, 2024

bors commented Mar 23, 2024

workingjubilee commented Mar 22, 2024 •

edited

Loading

maurer Mar 22, 2024 •

edited

Loading

workingjubilee Mar 22, 2024 •

edited

Loading

maurer commented Mar 22, 2024 •

edited

Loading

workingjubilee commented Mar 22, 2024 •

edited

Loading