Add API test and improve memory safety in MMIO constructors#54
Add API test and improve memory safety in MMIO constructors#54phip1611 merged 2 commits intorust-osdev:mainfrom
Conversation
Having such tests is useful. Without it, it could happen that `Backend` isn't exported anymore - which would still enable to create instances via the constructors but one can't create bindings anymore. This test therefore protects against that. The test succeeds if the test compiles.
|
After that, I'd ship |
mkroening
left a comment
mkroening
left a comment
There was a problem hiding this comment.
The code looks good to me, but the justification is not quite correct.
The previous
0x1000 as *mut u8cast produced a pointer with unspecified provenance, which is unsound under Rust's abstract machine even if it works in practice.
This is not true. addr as *mut T is fully equivalent to ptr::with_exposed_provenance_mut(addr), according to its documentation. I think it is more about being explicit about making up a provenance.
The whole situation is a bit confusing, to be honest, because for volatile operations on memory that is outside any Rust allocation (such as NULL), the pointer does not have to be valid for the corresponding non-volatile operation and does not need provenance. For details, see the second bullet in ptr::read_volatile as well as rust-osdev/volatile#69.
Make the MMIO constructors take `NonNull<u8>` instead of `*mut u8`. This encodes the non-null invariant in the type system and removes the runtime null check, so `InvalidAddressError::Null` is no longer needed. The current strict pointer provenance APIs and understanding are still shaping. From my understanding, using ptr::with_exposed_provenance_mut() is the correct way to tell Rust: This pointer comes from outside the Rust abstract machine and uses the provenance of that - which is the correct thing for MMIO. This is a breaking change: callers must now pass `NonNull<u8>` to `new_mmio()` and `Uart16550Tty::new_mmio()`.
|
complicated stuff... thanks! |
2 participants
This PR makes two changes to the
uart_16550crate.First, a compile-only API test is added to guard against accidental breakage of public exports. Without it, removing
Backendfrom the public API would go undetected as long as direct constructors still compiled.Second,
new_mmio()on bothUart16550andUart16550Ttynow takesNonNull<u8>instead of*mut u8. The previous0x1000 as *mut u8cast produced a pointer with unspecified provenance, which is unsound under Rust's abstract machine even if it works in practice. The correct approach since Rust 1.84 isptr::with_exposed_provenance_mut(), which makes provenance explicit for addresses pointing outside the Rust abstract machine — exactly the MMIO use case.This is a breaking change: callers must update their pointer construction accordingly. As a side effect, null-pointer validation moves from runtime to the type system, so
InvalidAddressError::Nullhas been removed.Closes #52.