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

CFI: Support self_cell-like recursion #122875

Merged
merged 1 commit into from Mar 24, 2024

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Mar 22, 2024

Current transform_ty attempts to avoid cycles when normalizing #[repr(transparent)] types to their interior, but runs afoul of this pattern used in self_cell:

struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X, and then stop, not attempting to normalize Y any further.

This PR was split off from #121962 as part of fixing the larger vtable compatibility issues.

r? @workingjubilee

@rustbot 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 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2024

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

Comment on lines 894 to 895
let mut parents = parents.to_vec();
parents.push(ty);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so... given this is recursive, this is just gonna be unbounded memory usage, huh...?

I cannot agree we should merge this without someone answering first how much recursion it takes to kill the compiler here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the size of parents only increases when it unwraps a transparent type it hasn't already visited. This means the size of parents is limited to a spine of #[repr(transparent)] types in depth.

I currently have it optimized to avoid any allocations in the common case (no transparents) by taking a &[]. If we want to make it work in a way that's optimal for maximal memory usage, I can make it take a &mut Vec<u8> instead, and .truncate() after this if statement to reset it. (That would make the total memory used by this O(n) instead of O(n^2)).

I just went to double check, and evidently Vec::new() doesn't cost an allocation if you don't ever push to it. Would you like me to switch to that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I may or may not decide the amount of recursion this handles is acceptable, but there's a number and I want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that you want the stack depth that the compiler goes to before dying without this patch, or do you mean you want to know how deeply I can nest transparent structs before the compiler gives up? (I'm guessing they'll be similar, because it'll be based on the stack frame count, not this vector, but I just want to make sure I understand you correctly.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how deep the struct nesting must be, yes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it dies because of sigsegv instead, that's reassuring!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it needs to be pretty deep before it dies. This will stop dying with one layer of alternation removed in today's nightly rustc, and will stop dying with two layers of alternation removed with the parent tracking (slightly bigger stack frame, so that's expected).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

Would it be possible to just generalize it similarly to what we do for recursive repr(transparent)?

@maurer maurer force-pushed the cfi-transparent-termination branch from 5db6e72 to a1ea768 Compare March 22, 2024 16:08
@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

Would it be possible to just generalize it similarly to what we do for recursive repr(transparent)?

That would be cool but:

  1. What type are we generalizing? PhantomData is used in this case, but I don't think it's guaranteed to be wrapped by that in particular. In the pointer generalization case, we're specifically generalizing pointer-to-anything. For this problem to go away, we need to broaden it to fixed-size-type-with-type-parameter-generalization. (The obvious case of managed pointers still bottoms out at PhantomData in all the cases I know of, so I'd have to look around a bit to see if it was possible to have another type not defined in terms of other types that has this property.)
  2. What do we generalize it to? Do we just convert any PhantomData<T> to ()?

I think the first question is the bigger one, since any fixed-size-type that isn't a pointer and has a type parameter would qualify as a potential thing to generalize, and I don't think we want to generalize all of those, do we?

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

1. What type are we generalizing? `PhantomData` is used in this case, but I don't think it's guaranteed to be wrapped by that in particular. In the pointer generalization case,  we're specifically generalizing pointer-to-anything.

I guess it depends on how common is this pattern or how frequently do we expect it to occur.

2. What do we generalize it to? Do we just convert any `PhantomData<T>` to ()`?

Not any, but just in that case i.e., recursive repr(transparent) self_cell).

I think the first question is the bigger one, since any fixed-size-type that isn't a pointer and has a type parameter would qualify as a potential thing to generalize, and I don't think we want to generalize all of those, do we?

Do you know how common is this pattern (i.e., recursive repr(transparent) self_cell)?

@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

1. What type are we generalizing? `PhantomData` is used in this case, but I don't think it's guaranteed to be wrapped by that in particular. In the pointer generalization case,  we're specifically generalizing pointer-to-anything.

I guess it depends on how common is this pattern or how frequently do we expect it to occur.

2. What do we generalize it to? Do we just convert any `PhantomData<T>` to ()`?

Not any, but just in that case i.e., recursive repr(transparent) self_cell).

I think the first question is the bigger one, since any fixed-size-type that isn't a pointer and has a type parameter would qualify as a potential thing to generalize, and I don't think we want to generalize all of those, do we?

Do you know how common is this pattern (i.e., recursive repr(transparent) self_cell)?

This came up because of the crate self_cell which uses it, which has 3.5 million downloads, and is transitively depended on by rustc (which is how I found this issue in the first place).

@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

Pointer generalization doesn't solve it - I modified the test-case to avoid PhantomData and use a pointer instead, and it still happens. This is because the problem is in the self type substitution, not the fixed-size-type-parameter. (My earlier comment was an incorrect analysis.)

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

This came up because of the crate self_cell which uses it, which has 3.5 million downloads, and is transitively depended on by rustc (which is how I found this issue in the first place).

If we're sure this is a common pattern, would it be possible to implement it by looking the type(s) ahead (instead of keeping track of the visited types)?

It would be nice to add a new test file like this one: https://github.com/rust-lang/rust/blob/master/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-repr-transparent-types.rs for recursive repr(transparent) self_cell so we can also document the expected encoding.

@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

This came up because of the crate self_cell which uses it, which has 3.5 million downloads, and is transitively depended on by rustc (which is how I found this issue in the first place).

If we're sure this is a common pattern, would it be possible to implement it by looking the type(s) ahead (instead of keeping track of the visited types)?

Wouldn't this require O(n^2) work instead of O(n) work when peeling transparent structs? As I peel a transparent struct, I'd have to walk down the remainder of the struct looking for myself to figure out if I need to do some kind of early stop. If there are N nested transparent structs, this will result in N checks, each requiring that I walk depth N looking for the type currently being transformed.

It would be nice to add a new test file like this one: https://github.com/rust-lang/rust/blob/master/tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-repr-transparent-types.rs for recursive repr(transparent) self_cell so we can also document the expected encoding.

I was intentionally avoiding this to leave this definition loose - I don't think that we want to canonicalize the encoding of a self-referential type like this. This would leave you free to adjust it to whatever you want, whether it's some kind of generalization, an early termination like this, etc. as long as it still builds and runs.

I think it makes a lot of sense to do that sort of explicit checking at types that could appear on language boundaries, because there's a defined standard. For types that are only possible internally (especially if you might want to change this to an alternate generalization approach some day, given that this currently is needed for pointer types too, not just PhantomData), I don't see a strong reason to be codifying the actual tag in a test, only that the tags match at icall sites, which this test checks.

That said, if I get feedback from the project that such a test would be beneficial, I can definitely add it to this PR.

@workingjubilee
Copy link
Contributor

workingjubilee commented Mar 22, 2024

I think for types that only appear in Rust to Rust calls, for their encoding, they should be categorized and tested separately, because it is always okay to breaking-change the encoding, and the test should be in an "auto-blessable" form, so that contributors do not need to know about the vagaries of CFI to fix the test.

@rcvalle If you wish such a test is added, I have no objection per se, but I would prefer we open an issue for setting up the requisite infra for blessable CFI encoding tests and deferring it to a later PR, because that infra should land first.

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

I mean, it would just to document the corner case and what it's currently being encoded to, like we do for recursive repr(transparent), but I don't have any objections if it's not added now.

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

Wouldn't this require O(n^2) work instead of O(n) work when peeling transparent structs? As I peel a transparent struct, I'd have to walk down the remainder of the struct looking for myself to figure out if I need to do some kind of early stop. If there are N nested transparent structs, this will result in N checks, each requiring that I walk depth N looking for the type currently being transformed.

Couldn't we use ty.contains()? Would it be that bad? I'm asking because we use it already in a few places (and the Rust compiler too).

@workingjubilee
Copy link
Contributor

Hmm. Well, the repr(transparent) stuff is important because it ought to remain correct in Rust-to-C FFI.... hmmm.

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

@maurer Sorry, I inadvertently edited your comment instead of replying, but copied the original comment back. GitHub is hard 😅

@workingjubilee
Copy link
Contributor

@maurer to be clear, is it possible for self_cell! types to produce things that are valid for passing to C FFI?

@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

Upon reconsideration, I think it does make sense to have a codegen test for this. I haven't seen someone use this pattern in a way they expected to use for C FFI, but it does seem that you can represent C-compatible types like this. I'll add it to the repr-transparent test.

@workingjubilee
Copy link
Contributor

hokay!

@workingjubilee
Copy link
Contributor

Wouldn't this require O(n^2) work instead of O(n) work when peeling transparent structs? As I peel a transparent struct, I'd have to walk down the remainder of the struct looking for myself to figure out if I need to do some kind of early stop. If there are N nested transparent structs, this will result in N checks, each requiring that I walk depth N looking for the type currently being transformed.

Couldn't we use ty.contains()? Would it be that bad? I'm asking because we use it already in a few places (and the Rust compiler too).

I may not understand what you're on about but that seems like it would be answering the wrong question since it can recurse through the type to hit the leaves.

@rcvalle
Copy link
Member

rcvalle commented Mar 22, 2024

I may not understand what you're on about but that seems like it would be answering the wrong question since it can recurse through the type to hit the leaves.

You're right. That would be useful if we wanted to generalize it. I'm not sure, but I feel we could just look ahead one ty for the self_cell and (shallow?) compare with current ty?

@workingjubilee
Copy link
Contributor

Perhaps! I have run out of opinions about this implementation. We should have a test, and try to avoid doing anything blatantly O(n^2) where we can avoid it, but beyond that I do not have a strong feeling on lookahead vs. caching.

@maurer maurer force-pushed the cfi-transparent-termination branch from a1ea768 to 8b5aba7 Compare March 22, 2024 18:48
@rustbot
Copy link
Collaborator

rustbot commented Mar 22, 2024

Some changes occurred in tests/codegen/sanitizer

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

@workingjubilee
Copy link
Contributor

@maurer merge conflicts somehow?

Current `transform_ty` attempts to avoid cycles when normalizing
`#[repr(transparent)]` types to their interior, but runs afoul of this
pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By
using a types-visited list, this will instead get expanded exactly
one layer deep to X<Y>, and then stop, not attempting to normalize `Y`
any further.
@maurer maurer force-pushed the cfi-transparent-termination branch from 8b5aba7 to dec36c3 Compare March 22, 2024 23:05
@maurer
Copy link
Contributor Author

maurer commented Mar 22, 2024

It was a conflict with #122852 , which touched all raw pointers including some in the typeid code. I've resolved it.

@workingjubilee
Copy link
Contributor

ahhh spicy.

@workingjubilee
Copy link
Contributor

If a more elegant refactor is found that seems fine, but currently this looks okay.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 23, 2024

📌 Commit dec36c3 has been approved by workingjubilee

It is now in the queue for this repository.

@bors 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 added a commit to workingjubilee/rustc that referenced this pull request Mar 23, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? `@workingjubilee`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? ``@workingjubilee``
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - 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#122915 (Delay a bug if no RPITITs were found)
 - rust-lang#122916 (docs(sync): normalize dot in fn summaries)
 - rust-lang#122921 (Enable more mir-opt tests in debug builds)
 - 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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? ```@workingjubilee```
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
…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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? ````@workingjubilee````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 24, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? `````@workingjubilee`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
…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
@bors bors merged commit b9b65f8 into rust-lang:master Mar 24, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2024
Rollup merge of rust-lang#122875 - maurer:cfi-transparent-termination, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? ``````@workingjubilee``````
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
…, r=workingjubilee

CFI: Support self_cell-like recursion

Current `transform_ty` attempts to avoid cycles when normalizing `#[repr(transparent)]` types to their interior, but runs afoul of this pattern used in `self_cell`:

```
struct X<T> {
  x: u8,
  p: PhantomData<T>,
}

 #[repr(transparent)]
struct Y(X<Y>);
```

When attempting to normalize Y, it will still cycle indefinitely. By using a types-visited list, this will instead get expanded exactly one layer deep to X<Y>, and then stop, not attempting to normalize `Y` any further.

This PR was split off from rust-lang#121962 as part of fixing the larger vtable compatibility issues.

r? ``````@workingjubilee``````
RenjiSann pushed a commit to RenjiSann/rust that referenced this pull request Mar 25, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants