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

PersistentGpuBufferable probably shouldn't be an unsafe trait #12727

Closed
james7132 opened this issue Mar 26, 2024 · 2 comments · Fixed by #12744
Closed

PersistentGpuBufferable probably shouldn't be an unsafe trait #12727

james7132 opened this issue Mar 26, 2024 · 2 comments · Fixed by #12744
Labels
A-Rendering Drawing game state to the screen P-Unsound A bug that results in undefined compiler behavior
Milestone

Comments

@james7132
Copy link
Member

A good example of an unsafe trait is TrustedLen or WorldQuery, where the correct implementation of the trait is required to satisfy the safety invariants in its application, and currently PersistentGpuBufferable is not used in such a way. It is not used in any unsafe context CPU-side, and as far as I know, wgpu's validation should prevent any improperly uploading invalid inputs to the GPU.

Of the two implementations of the trait, the Arc<[u8]> implementation does not actually satisfy the safety invariants requiring proper alignment of the contents (it's min alignment is 1 and wgpu::COPY_BUFFER_ALGINMENT is currently 4 and this is not enforced in it's implementation), and both implementations do not document how they're satisfying the safety invariants.

Potential solutions here:

  • Make the trait not unsafe.
  • Remove the Arc<[u8]> implementation or enforce its safety invariants.
  • Document the safety invariants and enforce them properly.
@james7132 james7132 added A-Rendering Drawing game state to the screen P-Unsound A bug that results in undefined compiler behavior labels Mar 26, 2024
@james7132 james7132 added this to the 0.14 milestone Mar 26, 2024
@JMS55
Copy link
Contributor

JMS55 commented Mar 26, 2024

Arc<[u8]> is used for vertex data which is always in multiples of 48 bytes, so it satisfies the alignment.

@james7132
Copy link
Member Author

james7132 commented Mar 26, 2024

However, that's not enforced as a part of the implementation of the trait for Arc<[u8]>, which means the trait implementation itself is unsound according to the safety invariants of the trait.

IMO we should just replace the unsafe here with an assertion to validate the input, as it's pretty clear that this isn't a memory safety issue, as far as the Rust compiler is concerned.

github-merge-queue bot pushed a commit that referenced this issue Mar 29, 2024
# Objective
Fixes #12727. All parts that `PersistentGpuBuffer` interact with should
be 100% safe both on the CPU and the GPU: `Queue::write_buffer_with`
zeroes out the slice being written to and when uploading to the GPU, and
all slice writes are bounds checked on the CPU side.

## Solution
Make `PersistentGpuBufferable` a safe trait. Enforce it's correct
implementation via assertions. Re-enable `forbid(unsafe_code)` on
`bevy_pbr`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants