Rework std::sys::windows::alloc

[CDirkx]

I came across #76676 (comment), which points out that there was unsound code in the Windows alloc code, creating a &mut to possibly uninitialized memory. I reworked the code so that that particular issue does not occur anymore, and started adding more documentation and safety comments.

Full list of changes:

• moved and documented the relevant Windows Heap API functions
• refactor allocate_with_flags to allocate (and remove the other helper functions), which now takes just a bool if the memory should be zeroed
• add checks for if GetProcessHeap returned null
• add a test that checks if the size and alignment of a Header are indeed <= MIN_ALIGN
• add #![deny(unsafe_op_in_unsafe_fn)] and the necessary unsafe blocks with safety comments

I feel like I may have overdone the documenting, the unsoundness fix is the most important part; I could spit this PR up in separate parts.

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[dtolnay]

Thanks, this is great.

[dtolnay]

Regarding "hHeap is allowed to be any value" -- I didn't get that from the link. What's the specific text that indicates this? Under the "Parameters" section hHeap is documented exactly the same as it is for HeapAlloc, where you've put a more restrictive safety condition: "hHeap must be a non-null handle returned by GetProcessHeap."

[CDirkx]

In the testing I did HeapAlloc and HeapRealloc throw an exception if given a wrong hHeap value, but HeapFree just returns true indicating success. But you're right that the documentation is the same so this maybe shouldn't be relied on.

[CDirkx]

I added the safety condition.

[dtolnay]

Relevant to my comment on HeapFree above -- does this need to check hHeap for null?

[CDirkx]

dealloc gets to make the same assumption as realloc, since ptr has been successfully allocated with this allocator, the default process heap must be available.

[dtolnay]

It's not documented but I could believe this gets to assume GetProcessHeap returns nonnull without a check, because ptr is guaranteed to have been previously allocated from the default process heap, so the process has a default heap. Do you believe we need to add a null check? Is it possible for a process to go from having a heap to not having a heap?

[CDirkx]

I tried looking into the conditions under which GetProcessHeap would fail, but couldn't find any extra information. It does sound like a reasonable assumption, and even if it is wrong it is legal for implemenations of GlobalAlloc to abort (not unwind), which is what HeapRealloc does in practice when given an invalid hHeap.

[Berrysoft]

I don't think the return value need a null check. As we don't specify HEAP_GENERATE_EXCEPTIONS flag, the functions will fail when specified heap is null, and return a null pointer, which is just what we want.

[CDirkx]

HEAP_GENERATE_EXCEPTIONS may be enabled for the default process heap, at least on my machine the following code produces an exception, even with flags set to 0:

unsafe {
    let ptr = HeapAlloc(GetProcessHeap(), 0, 64);
    println!("{:?}", HeapReAlloc(std::ptr::null_mut(), 0, ptr, 128)) // exit code: 0xc0000005, STATUS_ACCESS_VIOLATION
}

[CDirkx]

I added the following to the GetProcessHeap documentation:

// SAFETY: Successful calls to this function within the same process are assumed to
// always return the same handle, which remains valid for the entire lifetime of the process.

Which means we can cache the value, and assume we have a valid handle in dealloc and realloc.

[Berrysoft]

Could we consider cache the return value of GetProcessHeap() in GlobalAlloc? It seems that ucrt is doing that.

[CDirkx]

We can't add a field to the System struct, but we could store it in a static I think.

rust/library/std/src/alloc.rs

Lines 130 to 132 in 673d0db

	#[stable(feature = "alloc_system_type", since = "1.28.0")]
	#[derive(Debug, Default, Copy, Clone)]
	pub struct System;

[Berrysoft]

I think it would be better to cache it somewhere.

[CDirkx]

I added caching of GetProcessHeap: HEAP, which in my isolated testing provided a nice little performance gain. Because we cache the handle for the heap and assume it stays valid for the entire duration of the program, we can indeed remove the null checks in dealloc and realloc.

For now the initialization of HEAP is performed in alloc, but this could be moved to the global runtime initialization in sys::init.

[CDirkx]

I believe this implementation to be sound, even when called simultaneously by different threads, but would like a double check.

[CDirkx]

Just OnceCell is not enough to implement this because it isn't Sync. I could use Once, SyncOnceCell or SyncLazy, but those are all provided by std, and in general it is preferred to not use std types in sys. Is there any other way? @dtolnay what are your thoughts on this?

[Berrysoft]

I looked into the source of OnceCell and found that it is only a UnsafeCell<Option<T>>. Therefore I think it is OK to use AtomicPtr here. As for the possible multiple assignment, I suggest using fetch_update.

[Berrysoft]

For example,

#[inline]
unsafe fn init_or_get_process_heap() -> c::HANDLE {
    HEAP.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |heap| {
        if heap.is_null() {
            let heap = unsafe { GetProcessHeap() };
            if heap.is_null() {
                None
            } else {
                Some(heap)
            }
        } else {
            Some(heap)
        }
    }).unwrap_or(ptr::null_mut())
}

GetProcessHeap may be called multiple times, but assignment will be only once.

[CDirkx]

One thing I'm concerned about is the performance. At least from the benchmarks I did, using a mutex or fetch_update is slower than just calling GetProcessHeap every time:

Benchmarks

#![feature(test)]
#![feature(atomic_fetch_update)]

extern crate test;

use std::sync::atomic::{AtomicPtr, Ordering};
use std::ffi::c_void;
use core::ptr;
use std::sync::{Mutex, Once};

pub type HANDLE = LPVOID;
pub type LPVOID = *mut std::ffi::c_void;

extern "system" {
    pub fn GetProcessHeap() -> HANDLE;
}

#[inline]
fn bench<F: Fn() -> HANDLE>(bencher: &mut test::bench::Bencher, f: F) {
    bencher.iter(|| {
        for _ in 0..1000 {
            let heap = f();
            assert!(!heap.is_null());
            test::black_box(heap);
        }
    })
}

#[bench]
fn syscall(bencher: &mut test::bench::Bencher) {
    bench(bencher, || {
        unsafe { GetProcessHeap() }
    })
}

#[bench]
fn once(bencher: &mut test::bench::Bencher) {
    static INIT: Once = Once::new();
    static HEAP: AtomicPtr<c_void> = AtomicPtr::new(ptr::null_mut());

    bench(bencher, || {
        INIT.call_once(|| { HEAP.store(unsafe { GetProcessHeap() }, Ordering::Relaxed) });
        HEAP.load(Ordering::Relaxed)
    })
}

#[bench]
fn load_store(bencher: &mut test::bench::Bencher) {
    static HEAP: AtomicPtr<c_void> = AtomicPtr::new(ptr::null_mut());

    bench(bencher, || {
        let heap = HEAP.load(Ordering::Relaxed);
        if heap.is_null() {
            let heap = unsafe { GetProcessHeap() };
            if !heap.is_null() {
                HEAP.store(heap, Ordering::Relaxed);
                heap
            } else {
                ptr::null_mut()
            }
        } else {
            heap
        }
    })
}

#[bench]
fn fetch_update(bencher: &mut test::bench::Bencher) {
    static HEAP: AtomicPtr<c_void> = AtomicPtr::new(ptr::null_mut());

    bench(bencher, || {
        let _old = HEAP.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |heap| {
            if heap.is_null() {
                let heap = unsafe { GetProcessHeap() };
                if heap.is_null() {
                    None
                } else {
                    Some(heap)
                }
            } else {
                Some(heap)
            }
        });

        HEAP.load(Ordering::Acquire)
    })
}

#[bench]
fn mutex(bencher: &mut test::bench::Bencher) {
    let mutex: Mutex<()> = Mutex::new(()); // would use a static sys_common::mutex::StaticMutex
    static mut HEAP: HANDLE = ptr::null_mut();

    bench(bencher, || {
        unsafe {
            let _lock = mutex.lock();
            if HEAP.is_null() {
                HEAP = GetProcessHeap();
            }

            HEAP
        }
    })
}

Results:

test fetch_update ... bench:      10,205 ns/iter (+/- 930)
test load_store   ... bench:         270 ns/iter (+/- 27)
test mutex        ... bench:      12,145 ns/iter (+/- 788)
test once         ... bench:         495 ns/iter (+/- 24)
test syscall      ... bench:       2,018 ns/iter (+/- 221)

[Berrysoft]

OK, that's fair.

[CDirkx]

I believe std is already guaranteed to have AtomicPtr available.

[Berrysoft]

Well, I think it would be better to use OnceCell, but it is a nightly API, and I don't know if the std lib allows using that.

[CDirkx]

Oh that might be better. I initially thought OnceCell was provided by std, and sys shouldn't depend on the rest of std, but now I see it is provided by core, so it should be fine to use it, even if it is unstable.

[dtolnay]

I'm not clear why this is unsafe to call.

[CDirkx]

I was thinking the unsafety of GetProcessHeap and the role it plays in correctly initializing HEAP which the realloc and dealloc code relies on.

[dtolnay]

That isn't what unsafe to call means; unsafe to call means the caller is responsible for establishing some precondition which is not enforced by the type system. What is the precondition the caller needs in order for a call to init_or_get_process_heap to be correct? That would need to be documented.

[CDirkx]

I added get_process_heap and updated some safety comments: get_process_heap is unsafe because it assumes HEAP has already been initialized, init_or_get_process_heap is not unsafe as there are no preconditions.

[dtolnay]

This looks unsound to me. If an allocation is performed on one thread and shortly deallocated on a different thread, why would the relaxed store in the first thread be guaranteed to be visible to the relaxed load in the second thread? It seems like the second thread could read null, which would violate HeapFree's documented contract.

[CDirkx]

That's true, would making the store Release and the loads in realloc and dealloc Aquire be enough?

[CDirkx]

The documentation of GlobalAlloc has the point:

• It's undefined behavior if global allocators unwind. This restriction may be lifted in the future, but currently a panic from any of these functions may lead to memory unsafety.

So I removed the debug_assert

[dtolnay]

Nice! Looks good to me.

[dtolnay]

@bors r+

[bors]

📌 Commit db1d003 has been approved by dtolnay

[Berrysoft]

Should the ordering here be Acquire?