Safe deconstruct_moving_ptr
#20990
Safe deconstruct_moving_ptr
#20990
Conversation
…t the fields don't overlap. Return aligned pointers from `move_field`. `deconstruct_moving_ptr` no longer works with `repr(packed)` structs, so the field of an aligned type will always be aligned.
chescock
added
A-ECS
crates/bevy_ptr/src/lib.rs
Outdated
| // SAFETY: This closure is never called | ||
| let value = unsafe { ptr.assume_init_mut() }; | ||
| // If a field is mentioned twice, this will cause mutable aliasing and fail to compile. | ||
| let _ = ($(&mut value.$field_index),*); |
There was a problem hiding this comment.
I'd write these into $field_alias and call core::hint::black_box on them instead to ensure that the compiler doesn't try to get smart and shorten their lifetimes.
There was a problem hiding this comment.
I'm hoping the compile-fail test will be enough to catch the compiler shortening the lifetimes. :)
I think making a tuple like this requires that they are all alive together. How would I use black_box there? Is your idea to make all the &muts be local variables, and then consume them one-by-one? Even ordinary drop() should work for that, right?
There was a problem hiding this comment.
| let _ = ($(&mut value.$field_index),*); | |
| $(let $field_alias = &mut value.$field_index;)*; | |
| $(core::hint::black_box($field_alias);)* |
Alternatively, yeah, drop should work as well.
There was a problem hiding this comment.
Suggested change
let _ = ($(&mut value.$field_index),*); $(let $field_alias = &mut value.$field_index;)*; $(core::hint::black_box($field_alias);)* Alternatively, yeah,
dropshould work as well.
The trouble is that we aren't guaranteed to have an identifier here anymore, so we can't bind any locals.
If we want to check that the field list is exhaustive, then we need an escape hatch for users that want to drop some fields, which means we want to support 0: _, and we don't have a spare identifier in that case.
What if we pass the tuple itself to drop? Like
| let _ = ($(&mut value.$field_index),*); | |
| core::mem::drop(($(&mut value.$field_index),*)); |
Now the compiler clearly needs that tuple to exist in order to pass it to drop, which means the borrows in its fields must all be alive concurrently.
There was a problem hiding this comment.
error: call to `std::mem::drop` with a value that does not implement `Drop`. Dropping such a type only extends its contained lifetimes
Thanks, Clippy. core::hint::black_box it is!
|
I was not aware of how much of the Bundle/Spawn* trait APIs were used in the greater ecosystem. I think we need to land this in the 0.17-rc process. I'd rather those cases not need to make otherwise unnecessary unsafe blocks. |
Fix several soundness holes. Ensure that field lists are exhaustive.
|
Alright, I think I plugged the safety holes, but there may have been some scope creep. I can't tell whether the safety is still worth all this complexity. Once we were using destructuring, I thought we might as well check that the list of fields was exhaustive. But I had some trouble with tuples because you can't use the I also found another hole around autoderef, where a |
crates/bevy_ptr/src/lib.rs
Outdated
| // SAFETY: This closure is never called | ||
| let value = unsafe { ptr.assume_init_mut() }; | ||
| // If a field is mentioned twice, this will cause mutable aliasing and fail to compile. | ||
| let _ = ($(&mut value.$field_index),*); |
There was a problem hiding this comment.
| let _ = ($(&mut value.$field_index),*); | |
| $(let $field_alias = &mut value.$field_index;)*; | |
| $(core::hint::black_box($field_alias);)* |
Alternatively, yeah, drop should work as well.
| ({ let tuple { $($field_index:tt: $pattern:pat),* $(,)? } = $ptr:expr ;}) => { | ||
| // Specify the type to make sure the `mem::forget` doesn't forget a mere `&mut MovingPtr` | ||
| let mut ptr: $crate::MovingPtr<_, _> = $ptr; | ||
| let _ = || { | ||
| let value = &mut *ptr; | ||
| // Ensure that each field index exists and is mentioned only once | ||
| // Ensure that the struct is not `repr(packed)` and that we may take references to fields | ||
| let _ = ($(&mut value.$field_index),*); | ||
| // Ensure that `ptr` is a tuple and not something that derefs to it | ||
| // Ensure that the number of patterns matches the number of fields | ||
| fn unreachable<T>(_index: usize) -> T { | ||
| unreachable!() | ||
| } | ||
| *value = ($(unreachable($field_index)),*); | ||
| }; | ||
| // SAFETY: | ||
| // - `f` does a raw pointer offset, which always returns a non-null pointer to a field inside `T` | ||
| // - The struct is not `repr(packed)`, since otherwise the block of code above would fail compilation | ||
| // - `mem::forget` is called on `self` immediately after these calls | ||
| // - Each field is distinct, since otherwise the block of code above would fail compilation | ||
| $(let $pattern = unsafe { ptr.move_field(|f| &raw mut (*f).$field_index) };)* | ||
| core::mem::forget(ptr); | ||
| }; |
There was a problem hiding this comment.
This seems to still be vulnerable to the deref issue since there's nothing checking that MovingPtr points to a tuple nor that $field_index is actually a tuple index.
There was a problem hiding this comment.
This seems to still be vulnerable to the deref issue since there's nothing checking that
MovingPtrpoints to a tuple nor that$field_indexis actually a tuple index.
The assignment should check that. It's a little hard to see through the macro syntax, but it's doing *value = (unreachable(0), unreachable(1));, which is only valid if value is a &mut (_, _) of the right arity.
It turns out that even destructuring wouldn't prevent it, since &mut T can be destructured just like a T, but assignment does require the type to match.
The compile-fail tests for MovingPtr<Box<(usize, usize)>> and MovingPtr<&mut (usize, usize)> should be enough to verify that the check works, right? Or is there another case I'm missing?
...Although I just noticed that I put the , as a separator instead of after every element, which won't quite work for 1-tuples. I'll fix that now, and add some 1-tuple tests.
| ({ let MaybeUninit::<tuple> { $($field_index:tt: $pattern:pat),* $(,)? } = $ptr:expr ;}) => { | ||
| // Specify the type to make sure the `mem::forget` doesn't forget a mere `&mut MovingPtr` | ||
| let mut ptr: $crate::MovingPtr<core::mem::MaybeUninit<_>, _> = $ptr; | ||
| let _ = || { | ||
| // SAFETY: This closure is never called | ||
| let value = unsafe { ptr.assume_init_mut() }; | ||
| // Ensure that each field index exists and is mentioned only once | ||
| // Ensure that the struct is not `repr(packed)` and that we may take references to fields | ||
| let _ = ($(&mut value.$field_index),*); | ||
| // Ensure that `ptr` is a tuple and not something that derefs to it | ||
| // Ensure that the number of patterns matches the number of fields | ||
| fn unreachable<T>(_index: usize) -> T { | ||
| unreachable!() | ||
| } | ||
| *value = ($(unreachable($field_index)),*); | ||
| }; | ||
| // SAFETY: | ||
| // - `f` does a raw pointer offset, which always returns a non-null pointer to a field inside `T` | ||
| // - The struct is not `repr(packed)`, since otherwise the block of code above would fail compilation | ||
| // - `mem::forget` is called on `self` immediately after these calls | ||
| // - Each field is distinct, since otherwise the block of code above would fail compilation | ||
| $(let $pattern = unsafe { ptr.move_maybe_uninit_field(|f| &raw mut (*f).$field_index) };)* | ||
| core::mem::forget(ptr); | ||
| }; |
…oving_ptr # Conflicts: # crates/bevy_ptr/src/lib.rs
… can't end the borrows early.
Add tests for those checks.
alice-i-cecile
added
S-Ready-For-Final-Review
# Objective Make it possible to call `deconstruct_moving_ptr` with safe code. It currently needs to be `unsafe` because you can pass the same field name multiple times and cause mutable aliasing. Fix some other small issues: * Allow the trailing comma to be omitted * Prevent passing a `&mut MovingPtr` or `&MovingPtr`, which would still call `move_field` but would forget the reference instead of the `MovingPtr` * Tighten safety requirements on `move_maybe_uninit_field`, since mutable aliasing is still UB for `MaybeUninit` ## Solution Have the `deconstruct_moving_ptr` macro emit code that borrows all of the provided fields at once from a `&mut T`. If the same field is provided more than once, this will fail with an error of "cannot borrow value as mutable more than once at a time". That code will *also* fail to compile on a `repr(packed)` struct, since we can't borrow fields from that at all. So, drop support for those. That means `move_field` on an aligned pointer can return an aligned pointer, removing the need for `.try_into().debug_checked_unwrap()`. --------- Co-authored-by: François Mockers <[email protected]>
Objective
Make it possible to call
deconstruct_moving_ptrwith safe code. It currently needs to beunsafebecause you can pass the same field name multiple times and cause mutable aliasing.Fix some other small issues:
&mut MovingPtror&MovingPtr, which would still callmove_fieldbut would forget the reference instead of theMovingPtrmove_maybe_uninit_field, since mutable aliasing is still UB forMaybeUninitSolution
Have the
deconstruct_moving_ptrmacro emit code that borrows all of the provided fields at once from a&mut T. If the same field is provided more than once, this will fail with an error of "cannot borrow value as mutable more than once at a time".That code will also fail to compile on a
repr(packed)struct, since we can't borrow fields from that at all. So, drop support for those. That meansmove_fieldon an aligned pointer can return an aligned pointer, removing the need for.try_into().debug_checked_unwrap().