Audit C enum for safety and use integer newtypes where needed #55
Conversation
src/table/boot.rs
Outdated
| @@ -443,6 +447,9 @@ pub enum MemoryType { | |||
| PalCode, | |||
| /// Memory region which is usable and is also non-volatile. | |||
| PersistentMemory, | |||
| // SAFETY: UEFI defines a MaxMemoryType, therefore adding new memory types | |||
There was a problem hiding this comment.
Unfortunately, quoting from the spec:
MemoryType values in the range
0x70000000..0x7FFFFFFF are reserved for OEM use.
MemoryType values in the range 0x80000000..0xFFFFFFFF are reserved for use by
UEFI OS loaders that are provided by operating system vendors.
There was a problem hiding this comment.
Ah, so UEFI max values can lie... Sad, sad... Let's review the whole thing in this new light then!
There was a problem hiding this comment.
Fixed in the latest commit. I checked the GOP case again, and the spec is more insistent on the fact that "Valid EFI_GRAPHICS_PIXEL_FORMAT enum values are less than this value", so I still feel confident leaving it alone. Of course, there would also be a consistency argument in favor of making every enum used in FFI a newtype_enum now that these are becoming quite common...
HadrienG2
force-pushed
the
safe-enums
branch
from
36e46b1
to
d198935
Compare
As discussed in #53, there are two more UEFI enums which should not be modeled as Rust enums in the UEFI codebase besides Status, namely ScanCode and ProcessorArch.
I added safety notes to the code of all other UEFI enums that we have modeled as Rust enums, which explains why I think that design is safe for them.
While interfacing ProcessorArch like this, I noticed in the logs that the Debug representation of newtype-style enum FFI was pretty bad with the current code: the numerical value was displayed instead of the variant name. Addressing this required going for a more invasive variant generation macro which must have full knowledge of all enum variants at declaration time. However, I think this is a fair price to pay for keeping logs readable.
Fixes #53.