-
Notifications
You must be signed in to change notification settings - Fork 160
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
uefi-raw: Add explicit padding field for MemoryDescriptor #1334
base: main
Are you sure you want to change the base?
Conversation
I'd like to understand this better -- why is this cast being performed? In general we can't make the problems of uninit bytes go away by adding explicit padding fields, because our types often go over FFI boundaries and we don't control the C definition. If you do need a "raw bytes" view of a type that may contain uninit bytes, use |
6b2bd00
to
a86c72e
Compare
I've updated the PR description. Please re-check and let me know what you think. |
I don't get this check. What is its purpose? Why do we have it? @nicholasbishop |
451716a
to
0609ea2
Compare
This allows to use `dbg!` for example. Having this is beneficial in all no_std crates. There are no disadvantages or negative implications.
By convention, structs having a C representation should be explicit with their in-between padding fields. This allows users memory-safe trivial serialization and deserialization by accessing the underlying memory as byte slice. Without this fields being explicit, Miri complains about uninitialized memory accesses, which is UB. uefi-raw: add test for MemoryDescriptor to catch Miri problems The underlying issue is that if someone wants to cast a slice of descriptors to a byte slice, the uninitialized **implicit** padding bytes are UB. Miri complains about that.
The check-raw code assumes nothing is allowed by default, then adds various exceptions for code we do want to allow. Rust is a big language, and we should be very intentional about any new types of code being added to uefi-raw since it's intended to hew closely to the C API. For this particular use case
This is debatable I think. For example, bindgen has an option to enable explicit padding but its not enabled by default.
In general, adding padding bytes can indeed make serialization and deserialization easier, but there are a couple cons to consider:
I think it would really help to have an example of the code where you want this so we can discuss whether alternatives might work acceptably well. For example, perhaps there's something we can do with |
IMHO, having implicit padding bytes in Low-Level types (used for Casting etc) is a bad idea. When you cast a&[MemoryDescriptor]
to&[u8]
, Miri complains about uninitialized bytes.Exporting one (or multiple) public_pad0
fields however is also not nice. So, we should identify all structs where this is relevant and come up with a solution. Perhaps, lets move to constructor-based types with private fields? This is breaking, how-ever.By convention, structs having a C representation should be explicit with their
in-between padding fields. This allows users memory-safe trivial serialization
and deserialization by accessing the underlying memory as byte slice. Without
this fields being explicit, Miri complains about uninitialized memory accesses,
which is UB.
Checklist