Ensure that #[repr(packed)] cannot be used on #[pin_projectable] structs #34
Conversation
|
cc @RalfJung: I've not 100% sure that my reasoning about 'no-op |
|
It looks like CI started using a newer version of Clippy, which is causing lints to trigger for code that was passing before. |
As described in issue taiki-e#32, it's possible to bypass the #[repr(packed)] check via the use of an additional procedural macro. However, we need to be able to forbid using #[repr(packed) in order to make #[pin_projectable] sound To enforce this, we can make use of the fact that taking references to the fields of a #[repr(packed)] struct is unsafe. Given a #[repr(packed)] struct Foo, we can generate code like this: ```rust fn check_Foo(val: Foo) { &val.field1; &val.field2; ... &val.fieldn; } ``` If `Foo` turns out to be #[repr(packed)], the compiler will generate an error for us, since none of the field references are in an `unsafe` block. Unfortunately, this check requires that at least one of the struct's fields have an alignment greater than 1. If the struct is composed entirely of fields of alignment 1 (e.g. 'u8'), it will be safe to take a reference to any of the fields, preventing an error from being emiited. However, I believe that pin projecting such a struct is actually sound. If all fields have an alignment 1, #[repr(packed)] is effectively a no-op, as struct will already have no padding. I've added a test to verify that the compiler does not copy the fields of such a struct when it is dropped.
This lint is triggering on existing code, completely unrelated to this PR
Aaron1011
force-pushed
the
fix/packed-bypass
branch
from
8d7b45a to
799aeb0
Compare
|
AFAIK currently the compiler both makes taking a reference to a 1-aligned packed field unsafe and moves them for If you want to be really sure, you could even add something like that as a ui compile-fail test to rustc. Then add a comment in that test explaining that if we ever start accepting this, we should also change the drop glue for packed structs to not copy 1-aligned fields. Link to this issue for why that is important. |
Wait and this passes? That's odd. I will have to find the code in rustc that does the moving, I don't recall an exception for already "sufficiently aligned" fields. But that would be good news for you. I guess. |
|
Found it. And this is not very recent. Interesting. Given this, the compiler could also stop moving things entirely e.g. in a I am not aware of a similar exception for the error (and it would be much harder there in general as that's pre-monomorphization). But better add some tests as described above. |
move of packed fields might or might not occur when they actually are sufficiently aligned See taiki-e/pin-project#34, where it was pointed out that we actually don't move fields of 1-aligned types when dropping a packed struct -- but e.g. in a `packed(2)` struct, we don't do something similar for 2-aligned types. The code for that is [here](https://github.com/rust-lang/rust/blob/db7c773a6be2f050d1d1504763819ea3916f5428/src/librustc_mir/util/alignment.rs#L7).
move of packed fields might or might not occur when they actually are sufficiently aligned See taiki-e/pin-project#34, where it was pointed out that we actually don't move fields of 1-aligned types when dropping a packed struct -- but e.g. in a `packed(2)` struct, we don't do something similar for 2-aligned types. The code for that is [here](https://github.com/rust-lang/rust/blob/db7c773a6be2f050d1d1504763819ea3916f5428/src/librustc_mir/util/alignment.rs#L7).
|
Would it make sense for Rust to have an officially supported way of checking for One one hand, this is a very niche use case. On the other hand, it's not currently possible to build a completely safe pin projection abstraction, despite the fact that the compiler has all the information that it needs to allow us to do so. |
|
Maybe? It seems worth asking by putting some pre-RFC onto internals.
What is wrong with your idea of having a "test function" that tries to take references? |
I have a few concerns with my current implementation:
Having a |
You could ask the compiler team. IMO it would be reasonable to demand that for any field that will be moved by the drop glue, there also will be an error when trying to create a reference. Basically, the error shouldn't be smarter than the drop glue.
I don't think so. That text was added because of the implicit move, and I was not aware that there are exceptions to the move for 1-aligned types.
|
|
bors r+ |
34: Ensure that #[repr(packed)] cannot be used on #[pin_projectable] structs r=taiki-e a=Aaron1011 As described in issue #32, it's possible to bypass the #[repr(packed)] check via the use of an additional procedural macro. However, we need to be able to forbid using #[repr(packed) in order to make #[pin_projectable] sound To enforce this, we can make use of the fact that taking references to the fields of a #[repr(packed)] struct is unsafe. Given a #[repr(packed)] struct Foo, we can generate code like this: ```rust fn check_Foo(val: Foo) { &val.field1; &val.field2; ... &val.fieldn; } ``` If `Foo` turns out to be #[repr(packed)], the compiler will generate an error for us, since none of the field references are in an `unsafe` block. Unfortunately, this check requires that at least one of the struct's fields have an alignment greater than 1. If the struct is composed entirely of fields of alignment 1 (e.g. 'u8'), it will be safe to take a reference to any of the fields, preventing an error from being emiited. However, I believe that pin projecting such a struct is actually sound. If all fields have an alignment 1, #[repr(packed)] is effectively a no-op, as struct will already have no padding. I've added a test to verify that the compiler does not copy the fields of such a struct when it is dropped. Note: I've removed the tests for `#[repr(packed)]` being denied on enums - `#[repr(packed)]` isn't even allowed on enums in the first place, so there's no need to do anything. Co-authored-by: Aaron Hill <[email protected]> Co-authored-by: Taiki Endo <[email protected]>
|
(I applied with those suggestions myself.) |
Build succeeded
|
83: Improve error message for #[repr(packed(N))] r=taiki-e a=taiki-e #34 can [already detect this](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=7331036de27faeb8354e367ef514c6c6), but this will generate better error messages. This also adds some tests related to `#[repr(packed)]`. Co-authored-by: Taiki Endo <[email protected]>
|
Seems this logic can be bypassed using
pin-project v0.4.3 or later (#135, btw, v0.4.0-0.4.2 are already yanked for another reason) is internally proc-macro-derive, so they are not affected by the problem that the struct definition is rewritten by another macro after the However, pin-project-lite is affected by this issue. The current pin-project-lite generates almost the same code as pin-project, but does not have the equivalent of the |
55: Prepare for removal of safe_packed_borrows lint r=taiki-e a=taiki-e pin-project-lite is using `safe_packed_borrows` lint for checking `repr(packed)` types. (See taiki-e/pin-project#34 for more) As described in #26, lint-based tricks aren't perfect, but they're much better than nothing. `safe_packed_borrows` is planned to be removed in favor of `unaligned_references` (rust-lang/rust#82525), so I would like to switch to `unaligned_references`. (However, actually, enable both lint as `unaligned_references` does not exist in older compilers.) Co-authored-by: Taiki Endo <[email protected]>
As described in issue #32,
it's possible to bypass the #[repr(packed)] check via the use of an
additional procedural macro. However, we need to be able to forbid
using #[repr(packed) in order to make #[pin_projectable] sound
To enforce this, we can make use of the fact that taking references to
the fields of a #[repr(packed)] struct is unsafe. Given a #[repr(packed)]
struct Foo, we can generate code like this:
If
Footurns out to be #[repr(packed)], the compiler will generate anerror for us, since none of the field references are in an
unsafeblock.
Unfortunately, this check requires that at least one of the struct's
fields have an alignment greater than 1. If the struct is composed
entirely of fields of alignment 1 (e.g. 'u8'), it will be safe to take a
reference to any of the fields, preventing an error from being emiited.
However, I believe that pin projecting such a struct is actually sound.
If all fields have an alignment 1, #[repr(packed)] is effectively a
no-op, as struct will already have no padding. I've added a test to
verify that the compiler does not copy the fields of such a struct when
it is dropped.
Note: I've removed the tests for
#[repr(packed)]being denied on enums -#[repr(packed)]isn't even allowed on enums in the first place, so there's no need to do anything.