CFI: Handle dyn with no principal #123003

maurer · 2024-03-24T15:27:28Z

In user-facing Rust, dyn always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and dyn Sync is, for example, legal, this results in us having dyn types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a dyn type which are relevant to its alias set.

Fixes #122998

r? workingjubilee

rustbot · 2024-03-24T15:27:36Z

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

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

Some changes occurred in tests/ui/sanitizer

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

compiler-errors · 2024-03-24T16:05:48Z

compiler/rustc_middle/src/ty/predicate.rs

@@ -290,7 +290,7 @@ impl<'tcx> ty::List<ty::PolyExistentialPredicate<'tcx>> {
    /// have a "trivial" vtable consisting of just the size, alignment,
    /// and destructor.
    pub fn principal(&self) -> Option<ty::Binder<'tcx, ExistentialTraitRef<'tcx>>> {
-        self[0]
+        self.get(0)?


I don't like changing this invariant in rustc_middle just because CFI makes bad types 😞

No other place in the compiler expects or constructs dyn types with no predicates.

Since later on in the type encoding process, this type will be examined under normalization, I can't just guard against calling principal in the calling code. Given your comment, that probably means I need to avoid constructing 0-predicate dyn

Do you have suggestions for predicates or representations that would make sense here? The case in question is that I have a dyn Send and dyn Sync - they share a vtable. This vtable has one method, drop_in_place. For purposes of checking the alias set, do you have opinions on what it should be encoded as? The only thing I will know about this object at the method site is that it is a dyn with no principal.

Three possible encodings that come to mind:

dyn Drop - I don't like this because these types may not implement Drop, and methods which require fn drop shouldn't go here, but it is descriptive.

dyn Sized - I'm considering this specifically because you can't normally have a dyn Sized object, and so it can't accidentally match anything else.

Contract the entire receiver to *mut () - this technically discards the alias set restriction that someone made a dyn out of this type at some point in the program, but should otherwise be fine. Note - switching from *dyn to *` is safe in this particular situation because this is only being used to encode vtable calls, which are already using thin-dyn pointers.

I'm currently adjusting my code to use the *mut () approach, but figured I'd lay out my thoughts and see if you have a different opinion.

You could perhaps use dyn Destruct, which every type currently implements (and approximately matches the fact that all we know is that it has a drop_in_place)?

*mut () also works I guess.

compiler-errors · 2024-03-24T16:06:50Z

compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs

@@ -760,7 +760,11 @@ fn transform_predicates<'tcx>(
            ty::ExistentialPredicate::AutoTrait(..) => Some(predicate),
        })
        .collect();
-    tcx.mk_poly_existential_predicates(&predicates)
+    if predicates.len() == 0 {


Above comment is in conflict w/ this hack, for example.

Map it to dyn Drop or something if you need a dummy principal trait to put in there.

compiler-errors · 2024-03-24T16:17:14Z

I'd like to review this and the other CFI PR you put up since they're actually kind of related to the type system

r? compiler-errors

workingjubilee · 2024-03-24T16:47:52Z

I indeed should not be the primary reviewer on anything that is basically just rustc_middle action. I specifically gazed into the ABIss.

In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a `dyn` type which are relevant to its alias set. Fixes rust-lang#122998

compiler-errors

This is fine for now!

compiler-errors · 2024-03-24T17:21:09Z

@bors r+ rollup

bors · 2024-03-24T17:21:12Z

📌 Commit ea45185 has been approved by compiler-errors

It is now in the queue for this repository.

CFI: Handle dyn with no principal In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a `dyn` type which are relevant to its alias set. Fixes rust-lang#122998 r? workingjubilee

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#120557 (Add rust-lldb pretty printing for Path and PathBuf) - rust-lang#121051 (Introduce infrastructure for generating target docs) - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring) - rust-lang#122896 (Update stdarch submodule) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122970 (Use `chunk_by` when building `ReverseSccGraph`) - rust-lang#123003 (CFI: Handle dyn with no principal) r? `@ghost` `@rustbot` modify labels: rollup

CFI: Handle dyn with no principal In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a `dyn` type which are relevant to its alias set. Fixes rust-lang#122998 r? workingjubilee

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#122802 (Provide structured suggestion for unconstrained generic constant) - rust-lang#122858 (Tweak `parse_dot_suffix_expr`) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122990 (Clarify transmute example) - rust-lang#122995 (Clean up unnecessary headers/flags in coverage mir-opt tests) - rust-lang#123003 (CFI: Handle dyn with no principal) - rust-lang#123005 (CFI: Support complex receivers) - rust-lang#123020 (Temporarily remove nnethercote from the review rotation.) r? `@ghost` `@rustbot` modify labels: rollup

CFI: Handle dyn with no principal In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a `dyn` type which are relevant to its alias set. Fixes rust-lang#122998 r? workingjubilee

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#120557 (Add rust-lldb pretty printing for Path and PathBuf) - rust-lang#122802 (Provide structured suggestion for unconstrained generic constant) - rust-lang#122858 (Tweak `parse_dot_suffix_expr`) - rust-lang#122990 (Clarify transmute example) - rust-lang#122995 (Clean up unnecessary headers/flags in coverage mir-opt tests) - rust-lang#123003 (CFI: Handle dyn with no principal) - rust-lang#123005 (CFI: Support complex receivers) - rust-lang#123020 (Temporarily remove nnethercote from the review rotation.) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#122858 (Tweak `parse_dot_suffix_expr`) - rust-lang#122982 (Add more comments to the bootstrap code that handles `tests/coverage`) - rust-lang#122990 (Clarify transmute example) - rust-lang#122995 (Clean up unnecessary headers/flags in coverage mir-opt tests) - rust-lang#123003 (CFI: Handle dyn with no principal) - rust-lang#123005 (CFI: Support complex receivers) - rust-lang#123020 (Temporarily remove nnethercote from the review rotation.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123003 - maurer:dyn-empty, r=compiler-errors CFI: Handle dyn with no principal In user-facing Rust, `dyn` always has at least one predicate following it. Unfortunately, because we filter out marker traits from receivers at callsites and `dyn Sync` is, for example, legal, this results in us having `dyn` types with no predicates on occasion in our alias set encoding. This patch handles cases where there are no predicates in a `dyn` type which are relevant to its alias set. Fixes rust-lang#122998 r? workingjubilee

rustbot assigned workingjubilee Mar 24, 2024

rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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 24, 2024

compiler-errors reviewed Mar 24, 2024

View reviewed changes

rustbot assigned compiler-errors and unassigned workingjubilee Mar 24, 2024

maurer force-pushed the dyn-empty branch from e0ef3ef to 6f4dc71 Compare March 24, 2024 16:52

maurer force-pushed the dyn-empty branch from 6f4dc71 to ea45185 Compare March 24, 2024 16:59

compiler-errors approved these changes Mar 24, 2024

View reviewed changes

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 24, 2024

matthiaskrgr mentioned this pull request Mar 24, 2024

Rollup of 7 pull requests #123010

Closed

matthiaskrgr mentioned this pull request Mar 25, 2024

Rollup of 8 pull requests #123023

Closed

matthiaskrgr mentioned this pull request Mar 25, 2024

Rollup of 8 pull requests #123027

Closed

matthiaskrgr mentioned this pull request Mar 25, 2024

Rollup of 7 pull requests #123029

Merged

bors merged commit 84ec66e into rust-lang:master Mar 25, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFI: Handle dyn with no principal #123003

CFI: Handle dyn with no principal #123003

maurer commented Mar 24, 2024

rustbot commented Mar 24, 2024

compiler-errors Mar 24, 2024

compiler-errors Mar 24, 2024

maurer Mar 24, 2024 •

edited

Loading

compiler-errors Mar 24, 2024

compiler-errors Mar 24, 2024

compiler-errors Mar 24, 2024

compiler-errors commented Mar 24, 2024

workingjubilee commented Mar 24, 2024

compiler-errors left a comment

compiler-errors commented Mar 24, 2024

bors commented Mar 24, 2024

CFI: Handle dyn with no principal #123003

CFI: Handle dyn with no principal #123003

Conversation

maurer commented Mar 24, 2024

rustbot commented Mar 24, 2024

compiler-errors Mar 24, 2024

Choose a reason for hiding this comment

compiler-errors Mar 24, 2024

Choose a reason for hiding this comment

maurer Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

compiler-errors Mar 24, 2024

Choose a reason for hiding this comment

compiler-errors Mar 24, 2024

Choose a reason for hiding this comment

compiler-errors Mar 24, 2024

Choose a reason for hiding this comment

compiler-errors commented Mar 24, 2024

workingjubilee commented Mar 24, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors commented Mar 24, 2024

bors commented Mar 24, 2024

maurer Mar 24, 2024 •

edited

Loading