[Merged by Bors] - Improve soundness of CommandQueue
#4863
Conversation
|
Can you add a test that miri would yell about for the uninit memory case |
alice-i-cecile
added
A-ECS
| @@ -81,27 +85,11 @@ impl CommandQueue { | |||
| // unnecessary allocations. | |||
| unsafe { self.bytes.set_len(0) }; | |||
|
|
|||
| let byte_ptr = if self.bytes.as_mut_ptr().is_null() { | |||
There was a problem hiding this comment.
I don't believe that this is technically sound (in the sense that the std docs aren't clear enough on this case) and instead relying on implementation details of Vec - the docs for Vec guarantee that:
Most fundamentally, Vec is and always will be a (pointer, capacity, length) triplet. No more, no less. The order of these fields is completely unspecified, and you should use the appropriate methods to modify these. The pointer will never be null, so this type is null-pointer-optimized.
However, the pointer might not actually point to allocated memory
However, that makes no guarantee that the ptr returned from as_mut_ptr isn't null. That's documented to:
Returns an unsafe mutable pointer to the vector’s buffer.
If the vector doesn't have a buffer then the documentation's assumed preconditions are not met. Therefore, any (non-UB having) implementation is permitted. This means that the pointer is the necessarily the same as the one above which is null-pointer optimised, therefore this could be a null pointer with an adverserial std implementation within the current docs.
That being said, I'm happy accepting this anyway - it's clear that std intends to give this guarantee, and the fix for this is not to defensively program around it, but to engage with alloc to get these docs clarified.
There was a problem hiding this comment.
Going by what the docs say I don't think we can program against them with is_null, as_mut_ptr could also return a freed pointer, which according to https://doc.rust-lang.org/stable/std/ptr/index.html#safety are also not valid for zero-sized reads. I guess the correct way to do this would be checking if the capacity is 0 rather than if the pointer is null. Anyway, I opened a PR to change Vec::as_ptr and Vec::as_mut_ptr's docs, so hopefully this get clarified in the stdlib.
alice-i-cecile
added
the
P-Unsound
|
alice-i-cecile
added
the
S-Ready-For-Final-Review
Oh right, I was trying to rephrase it and then suddently |
|
bors r+ |
# Objective This PR aims to improve the soundness of `CommandQueue`. In particular it aims to: - make it sound to store commands that contain padding or uninitialized bytes; - avoid uses of commands after moving them in the queue's buffer (`std::mem::forget` is technically a use of its argument); - remove useless checks: `self.bytes.as_mut_ptr().is_null()` is always `false` because even `Vec`s that haven't allocated use a dangling pointer. Moreover the same pointer was used to write the command, so it ought to be valid for reads if it was for writes. ## Solution - To soundly store padding or uninitialized bytes `CommandQueue` was changed to contain a `Vec<MaybeUninit<u8>>` instead of `Vec<u8>`; - To avoid uses of the command through `std::mem::forget`, `ManuallyDrop` was used. ## Other observations While writing this PR I noticed that `CommandQueue` doesn't seem to drop the commands that weren't applied. While this is a pretty niche case (you would have to be manually using `CommandQueue`/`std::mem::swap`ping one), I wonder if it should be documented anyway.
bors
bot
changed the title
CommandQueueCommandQueue
# Objective This PR aims to improve the soundness of `CommandQueue`. In particular it aims to: - make it sound to store commands that contain padding or uninitialized bytes; - avoid uses of commands after moving them in the queue's buffer (`std::mem::forget` is technically a use of its argument); - remove useless checks: `self.bytes.as_mut_ptr().is_null()` is always `false` because even `Vec`s that haven't allocated use a dangling pointer. Moreover the same pointer was used to write the command, so it ought to be valid for reads if it was for writes. ## Solution - To soundly store padding or uninitialized bytes `CommandQueue` was changed to contain a `Vec<MaybeUninit<u8>>` instead of `Vec<u8>`; - To avoid uses of the command through `std::mem::forget`, `ManuallyDrop` was used. ## Other observations While writing this PR I noticed that `CommandQueue` doesn't seem to drop the commands that weren't applied. While this is a pretty niche case (you would have to be manually using `CommandQueue`/`std::mem::swap`ping one), I wonder if it should be documented anyway.
# Objective This PR aims to improve the soundness of `CommandQueue`. In particular it aims to: - make it sound to store commands that contain padding or uninitialized bytes; - avoid uses of commands after moving them in the queue's buffer (`std::mem::forget` is technically a use of its argument); - remove useless checks: `self.bytes.as_mut_ptr().is_null()` is always `false` because even `Vec`s that haven't allocated use a dangling pointer. Moreover the same pointer was used to write the command, so it ought to be valid for reads if it was for writes. ## Solution - To soundly store padding or uninitialized bytes `CommandQueue` was changed to contain a `Vec<MaybeUninit<u8>>` instead of `Vec<u8>`; - To avoid uses of the command through `std::mem::forget`, `ManuallyDrop` was used. ## Other observations While writing this PR I noticed that `CommandQueue` doesn't seem to drop the commands that weren't applied. While this is a pretty niche case (you would have to be manually using `CommandQueue`/`std::mem::swap`ping one), I wonder if it should be documented anyway.
Objective
This PR aims to improve the soundness of
CommandQueue. In particular it aims to:std::mem::forgetis technically a use of its argument);self.bytes.as_mut_ptr().is_null()is alwaysfalsebecause evenVecs that haven't allocated use a dangling pointer. Moreover the same pointer was used to write the command, so it ought to be valid for reads if it was for writes.Solution
CommandQueuewas changed to contain aVec<MaybeUninit<u8>>instead ofVec<u8>;std::mem::forget,ManuallyDropwas used.Other observations
While writing this PR I noticed that
CommandQueuedoesn't seem to drop the commands that weren't applied. While this is a pretty niche case (you would have to be manually usingCommandQueue/std::mem::swapping one), I wonder if it should be documented anyway.