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

[Merged by Bors] - Improve soundness of CommandQueue #4863

Closed
wants to merge 5 commits into from

Conversation

SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented May 28, 2022

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 Vecs 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::swapping one), I wonder if it should be documented anyway.

Copy link
Member

@TheRawMeatball TheRawMeatball left a comment

Choose a reason for hiding this comment

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

The changes look good, but the justification for removing the null check should be added as a comment.

@BoxyUwU
Copy link
Member

BoxyUwU commented May 28, 2022

Can you add a test that miri would yell about for the uninit memory case

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels May 28, 2022
@@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@SkiFire13 SkiFire13 May 29, 2022

Choose a reason for hiding this comment

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

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 alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label May 30, 2022
@alice-i-cecile
Copy link
Member

(std::mem::forget is technically a );
I think you missed part of the sentence here.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 30, 2022
@alice-i-cecile alice-i-cecile requested a review from cart May 30, 2022 17:08
@SkiFire13
Copy link
Contributor Author

(std::mem::forget is technically a );
I think you missed part of the sentence here.

Oh right, I was trying to rephrase it and then suddently std::mem::forget about it. Anyway, I fixed it.

@cart
Copy link
Member

cart commented May 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
# 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 bors bot changed the title Improve soundness of CommandQueue [Merged by Bors] - Improve soundness of CommandQueue May 30, 2022
@bors bors bot closed this May 30, 2022
@SkiFire13 SkiFire13 deleted the commandqueue-soundness branch May 31, 2022 05:43
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# 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.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants