Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork175
uefi: alloc_pages() and alloc_pool() now return NonNull<[u8]>#1606
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| Ok(NonNull::new(ptr).expect("allocate_pool must not return a null pointer if successful")) | ||
| if let Some(ptr) = NonNull::new(ptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ifletSome(ptr) =NonNull::new(ptr){ | |
| NonNull::new(ptr).map(|ptr|NonNull::slice_from_raw_parts(ptr, size)).ok_or(Status::OUT_OF_RESOURCES.into()) |
| @@ -281,7 +281,7 @@ impl MemoryMapBackingMemory { | |||
| pub(crate) fn new(memory_type: MemoryType) -> crate::Result<Self> { | |||
| let memory_map_meta = boot::memory_map_size(); | |||
| let len = Self::safe_allocation_size_hint(memory_map_meta); | |||
| let ptr = boot::allocate_pool(memory_type, len)?.as_ptr(); | |||
| let ptr = boot::allocate_pool(memory_type, len)?.cast::<u8>().as_ptr(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just making the new API compatible with existing code; shortcut
| @@ -100,7 +102,8 @@ unsafe impl GlobalAlloc for Allocator { | |||
| // `allocate_pool` always provides eight-byte alignment, so we can | |||
| // use `allocate_pool` directly. | |||
| boot::allocate_pool(memory_type, size) | |||
| .map(|ptr| ptr.as_ptr()) | |||
| .map(|mut ptr: NonNull<[u8]>| unsafe { ptr.as_mut() }) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is unsound:as_mut creates a reference, but that's not valid because the allocated memory is uninitialized. I think instead this can be:
boot::allocate_pool(memory_type, size).map(|ptr:NonNull<[u8]>| ptr.as_ptr().cast::<u8>()).unwrap_or(ptr::null_mut())
(Potentially in the future it could be further simplified toptr.as_mut_ptr(), butit's not yet stable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is unsound: as_mut creates a reference, but that's not valid because the allocated memory is uninitialized.
Discussion continues here:#1606 (comment)
| let addr = ptr.as_ptr() as usize; | ||
| assert_eq!(addr % 4096, 0, "Page pointer is not page-aligned"); | ||
| let buffer = unsafe { ptr.as_mut() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same issue here, this is unsound with uninitialized memory. I don't think we have to usewrite_volatile like the code here did originally, but we do need to use some pointer function likeptr::write orptr::write_bytes.
Since so many of theNonNull<[T]>/*mut [T] functions are unstable, I think it's most convenient to cast it back to*mut u8. Then you can dounsafe { ptr.write_bytes(...) }.
Ditto for the docstring examples onboot::allocate_pages/boot::allocate_pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the concern you raised is not correct:#1606 (comment)
This aligns the signature with the Rust allocator API.
This aligns the signature with the Rust allocator API.
phip1611 commentedApr 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think we are safe if we write the following into the documentation of each function:
I think this is valid as Miri verifies my assumptions in this code: #![feature(allocator_api)]use std::alloc::{Allocator,GlobalAlloc,Layout,System};fnmain(){let layout =Layout::from_size_align(4096,4096).unwrap();letmut buffer_ptr =System.allocate(layout).unwrap();let buffer =unsafe{ buffer_ptr.as_mut()}; buffer[1] =42;let _x = buffer[1];let _y = buffer[2];unsafe{System.dealloc(buffer_ptr.as_ptr().cast(), layout);}} What do you think, are we on the same page? |
Miri is a runtime checker, so it can't fully detect all cases of UB. I'm pretty confident that creating a reference to uninitialized memory is always UB, regardless of whether the reference is read from, unless you use So there's no documentation-level exemption we can give ourselves -- the allocation must be initialized via the pointer API (or |
phip1611 commentedMay 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This leaves us (and the community) in an unfortunate situation. Wonderful! My suggestion:
What do you think?@nicholasbishop |
Yes, I'm fine with closing. I think we can leave the existing |
Leftovers have been moved to#1665 |
uefi: alloc_pages() and alloc_pool() now return
NonNull<[u8]>. This aligns the signature with the Rust allocator API and also makes more sense.Checklist