CFI: Support self_cell-like recursion by maurer · Pull Request #122875 · rust-la...

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

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

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?

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)?

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).

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.)

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.

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.

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.

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.

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).

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

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

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

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.

hokay!

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.

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?

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.

Some changes occurred in tests/codegen/sanitizer

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

@maurer merge conflicts somehow?

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

ahhh spicy.

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

@bors r+ rollup

📌 Commit dec36c3 has been approved by workingjubilee

It is now in the queue for this repository.