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

• Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
• Update the changelog (if necessary)

[phip1611]

Suggested change

	if let Some(ptr) = NonNull::new(ptr) {
	NonNull::new(ptr).map(|ptr| NonNull::slice_from_raw_parts(ptr, size)).ok_or(Status::OUT_OF_RESOURCES.into())

[phip1611]

Just making the new API compatible with existing code; shortcut

[nicholasbishop]

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 to ptr.as_mut_ptr(), but it's not yet stable.)

[phip1611]

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)

[nicholasbishop]

Same issue here, this is unsound with uninitialized memory. I don't think we have to use write_volatile like the code here did originally, but we do need to use some pointer function like ptr::write or ptr::write_bytes.

Since so many of the NonNull<[T]>/*mut [T] functions are unstable, I think it's most convenient to cast it back to *mut u8. Then you can do unsafe { ptr.write_bytes(...) }.

Ditto for the docstring examples on boot::allocate_pages/boot::allocate_pool.

[phip1611]

I think the concern you raised is not correct: #1606 (comment)

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};

fn main() {
    let layout = Layout::from_size_align(4096, 4096).unwrap();
    let mut 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 MaybeUninit or a similar construct. The MaybeUninit doc has some examples of instant-UB that don't require a read.

So there's no documentation-level exemption we can give ourselves -- the allocation must be initialized via the pointer API (or &[MaybeUninit<u8>]).

• It seems as the allocator API was created long before the discussions about MaybeUninit came up. After asking the community, Matthieu M. pointed out to me that the allocator API wasn't as thoroughly designed as I expected. Also, the progress on that feature is currently dead.

• The discussion above was whether creating a reference to uninitalized memory creates undefined behavior. This is not yet decided in the Rust reference. It might be okay, it might be not.

  Note that the last point (about pointing to a valid value) remains a subject of some
  debate.

This leaves us (and the community) in an unfortunate situation. Wonderful!

My suggestion:

• close this PR
• let each function return an *mut u8 similar to https://doc.rust-lang.org/alloc/alloc/fn.alloc.html

What do you think? @nicholasbishop

Yes, I'm fine with closing. I think we can leave the existing NonNull<u8> though, rather than switching to *mut u8. There isn't a big difference, right? So might as well avoid API churn.

Leftovers have been moved to #1665