-
Notifications
You must be signed in to change notification settings - Fork 187
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
Reading on uninitialized memory may cause UB ( util::read_spv()
)
#354
Comments
Unfortunately the safe yet naive fix of zero-initializing the newly allocated buffer incurs runtime overhead 😢 |
Yikes, it looks like rustsec/advisory-db#680 managed to get merged just yesterday (after sitting for half a year...). I'm inclined to just remove this function altogether or ban it away to a separately-published util crate, it has nothing to do with Ash's immediate goal of wrapping Vulkan. For parsing SPIR-V one can always resort to |
Fixes #354 `io::Read::read_exact` does not receive `MaybeUninit` memory and a trait implementation can possibly read from our uninitialized vector without `unsafe`, which is UB. As there is no proper solution to this problem yet (see linked issue), our safest bet is to just take the perf-hit and zero-initialize this vector.
Fixes #354 `io::Read::read_exact` does not receive `MaybeUninit` memory and a trait implementation can possibly read from our uninitialized vector without `unsafe`, which is UB. As there is no proper solution to this problem yet (see linked issue), our safest bet is to just take the perf-hit and zero-initialize this vector.
…470) Fixes #354 `io::Read::read_exact` does not receive `MaybeUninit` memory and a trait implementation can possibly read from our uninitialized vector without `unsafe`, which is UB. As there is no proper solution to this problem yet (see linked issue), our safest bet is to just take the perf-hit and zero-initialize this vector.
|
I've misread the function initially; we always use a compiler that emits SPIR-V in host-endianness (what |
Handling endianness isn't actually the main benefit, though it's nice. Vulkan requires that SPIR-V be passed in as a valid
Note that what you actually want here is target endianness, though this only differs when cross compiling. |
This is fairly trivial too, not "surprisingly tricky to get right" - this function could have been safe if it didn't juggle endianness around.
I was referring to the "host" that runs our Vulkan program, fair to call that a target. We compile the few shaders that we have on that target machine at runtime, so it's always the same on our end. Whatever, we're bikeshedding now 😬 |
The number of times I've had to explain to people that it's necessary at all (see e.g. #245) begs to differ, at least :P
The endianness flip is already 100% safe. The unsafety is required to treat an array of |
Implicitly saying that, if we took the penalty for copies, we can still change endianness (in a 100% safe way) but we wouldn't be here discussing this in the first place. The flip itself isn't the problem, the way it is implemented is (or was). |
Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.
Issue Description
https://github.com/MaikKlein/ash/blob/7fa182cc4330da8da98a7c023049162a4979d0bf/ash/src/util.rs#L116-L124
util::read_spv()
method creates an uninitialized buffer and passes it to user-providedRead
implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).This part from the
Read
trait documentation explains the issue:Suggested Fix
It is safe to zero-initialize the newly allocated
u32
buffer beforeread()
, in order to prevent user-providedRead
from accessing old contents of the newly allocated heap memory.Also, there are two nightly features for handling such cases.
Thank you for checking out this issue 👍
The text was updated successfully, but these errors were encountered: