allocator::c::{,Aligned}Malloc: fix ZST (re)allocs by replacing them with alloc(...)?; free(...);

relates to #32

Author: MaulingMonkey

src/allocator/c/aligned_malloc.rs

@@ -78,10 +78,23 @@ impl Meta for AlignedMalloc {
 
     const MAX_SIZE  : usize     = usize::MAX;
 
-    /// | Platform          | Behavior |
-    /// | ------------------| ---------|
-    /// | Linux             | Succeeds?
-    /// | Windows           | Fails?  [`_aligned_malloc`] explicitly documents "If \[...\] `size` is zero, this function invokes the invalid parameter handler, as described in [Parameter validation](https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation). If execution is allowed to continue, this function returns `NULL` and sets `errno` to `EINVAL`."
+    /// ### Platform: OS X
+    /// Zero sized allocs succeed.
+    /// Reallocs are untested.
+    ///
+    /// ### Platform: Linux
+    /// Zero sized allocs succeed.
+    /// Reallocs are untested.
+    ///
+    /// ### Platform: Windows
+    ///
+    /// [`_aligned_malloc`] is documented as failing when size = 0.
+    /// This is a lie: it succeeds, at least on 10.0.19045.5737.
+    /// OTOH, it may or may not trigger debug checks on other versions of windows or the CRT.
+    ///
+    /// [`_aligned_realloc`], on the other hand, is documented as *freeing* when size = 0.
+    /// Tests seem to confirm it, resulting in heap corruption checks if the buffers are sufficiently used afterwards.
+    /// As such, `*::Realloc` will skip it in favor of [`_aligned_malloc`] and [`_aligned_free`] when size = 0.
     ///
     #[doc = include_str!("_refs.md")]
     const ZST_SUPPORTED : bool  = false;
@@ -174,21 +187,33 @@ unsafe impl fat::Free for AlignedMalloc {
 // SAFETY: per above
 unsafe impl fat::Realloc for AlignedMalloc {
     #[cfg(target_env = "msvc")]
-    #[track_caller] unsafe fn realloc_uninit(&self, ptr: AllocNN, _old_layout: Layout, new_layout: Layout) -> Result<AllocNN, Self::Error> {
+    #[track_caller] unsafe fn realloc_uninit(&self, ptr: AllocNN, old_layout: Layout, new_layout: Layout) -> Result<AllocNN, Self::Error> {
         let new_layout = Self::fix_layout(new_layout)?;
-        // SAFETY: ✔️ `ptr` belongs to `self` per [`fat::Realloc::realloc_uninit`]'s documented safety preconditions
-        // SAFETY: ✔️ `new_layout` has been checked/fixed for platform validity
-        let alloc = unsafe { ffi::_aligned_realloc(ptr.as_ptr().cast(), new_layout.size(), new_layout.align()) };
-        NonNull::new(alloc.cast()).ok_or(())
+        if new_layout.size() == 0 {
+            let alloc = fat::Alloc::alloc_uninit(self, new_layout)?;
+            unsafe { fat::Free::free(self, ptr, old_layout) };
+            Ok(alloc)
+        } else {
+            // SAFETY: ✔️ `ptr` belongs to `self` per [`fat::Realloc::realloc_uninit`]'s documented safety preconditions
+            // SAFETY: ✔️ `new_layout` has been checked/fixed for platform validity
+            let alloc = unsafe { ffi::_aligned_realloc(ptr.as_ptr().cast(), new_layout.size(), new_layout.align()) };
+            NonNull::new(alloc.cast()).ok_or(())
+        }
     }
 
     #[cfg(target_env = "msvc")]
-    unsafe fn realloc_zeroed(&self, ptr: AllocNN, _old_layout: Layout, new_layout: Layout) -> Result<AllocNN, Self::Error> {
+    unsafe fn realloc_zeroed(&self, ptr: AllocNN, old_layout: Layout, new_layout: Layout) -> Result<AllocNN, Self::Error> {
         let new_layout = Self::fix_layout(new_layout)?;
-        // SAFETY: ✔️ `ptr` belongs to `self` per [`fat::Realloc::realloc_zeroed`]'s documented safety preconditions
-        // SAFETY: ✔️ `new_layout` has been checked/fixed for platform validity
-        let alloc = unsafe { ffi::_aligned_recalloc(ptr.as_ptr().cast(), 1, new_layout.size(), new_layout.align()) };
-        NonNull::new(alloc.cast()).ok_or(())
+        if new_layout.size() == 0 {
+            let alloc = fat::Alloc::alloc_zeroed(self, new_layout)?;
+            unsafe { fat::Free::free(self, ptr, old_layout) };
+            Ok(alloc.cast())
+        } else {
+            // SAFETY: ✔️ `ptr` belongs to `self` per [`fat::Realloc::realloc_zeroed`]'s documented safety preconditions
+            // SAFETY: ✔️ `new_layout` has been checked/fixed for platform validity
+            let alloc = unsafe { ffi::_aligned_recalloc(ptr.as_ptr().cast(), 1, new_layout.size(), new_layout.align()) };
+            NonNull::new(alloc.cast()).ok_or(())
+        }
     }
 }
 
@@ -267,5 +292,7 @@ mod ffi {
 #[test] fn fat_alignment()              { fat::test::alignment(AlignedMalloc) }
 #[test] fn fat_edge_case_sizes()        { fat::test::edge_case_sizes(AlignedMalloc) }
 #[test] fn fat_uninit()                 { if !ALIGNED_ALLOC_ZERO_INITS { unsafe { fat::test::uninit_alloc_unsound(AlignedMalloc) } } }
+#[test] fn fat_uninit_realloc()         { fat::test::uninit_realloc(AlignedMalloc) }
 #[test] fn fat_zeroed()                 { fat::test::zeroed_alloc(AlignedMalloc) }
+#[test] fn fat_zeroed_realloc()         { fat::test::zeroed_realloc(AlignedMalloc) }
 #[test] fn fat_zst_support()            { fat::test::zst_supported_conservative(AlignedMalloc) }

src/allocator/c/malloc.rs

@@ -48,16 +48,24 @@ impl Meta for Malloc {
 
     const MAX_SIZE : usize = usize::MAX; // *slightly* less in practice
 
-    /// "If the size of the space requested is zero, the behavior is implementation defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object."
-    /// C89 § 7.20.3 ¶ 1
+    /// `malloc(0)` is implementation defined at best (C89 § 7.20.3 ¶ 1), but `realloc(ptr, 0)` is [straight up undefined behavior as of C23](https://en.cppreference.com/w/c/memory/realloc#:~:text=the%20behavior%20is%20undefined)?
+    /// This wrapper works around that problem by invoking `malloc(0)` then `free(ptr)`.
     ///
-    /// Null pointers will be translated into an [`Err`], so this is at least defined behavior, but consider using an [`adapt`](crate::allocator::adapt) allocator for ZST support.
+    /// Consider using an [`adapt`](crate::allocator::adapt) allocator for ZST support.
+    ///
+    /// ### Platform: OS X
+    /// -   `malloc(0)` *untested?*
+    /// -   `realloc(ptr, 0)` *untested?*
+    ///
+    /// ### Platform: Linux
+    /// -   `malloc(0)` allocates
+    /// -   `realloc(ptr, 0)` *untested?*
+    ///
+    /// ### Platform: Windows
+    /// -   `malloc(0)` allocates
+    /// -   `_msize(zst)` returns `0`
+    /// -   `realloc(ptr, 0)` **frees** - to avoid this causing problems, this wrapper replaces it with `malloc(0)`, `free(ptr)`.
     ///
-    /// | Platform          | Behavior  |
-    /// | ------------------| ----------|
-    /// | Linux             | Allocate
-    /// | OS X              | ???
-    /// | Windows           | Allocate
     const ZST_SUPPORTED : bool = false;
 }
 
@@ -130,12 +138,24 @@ unsafe impl thin::Realloc for Malloc {
     const CAN_REALLOC_ZEROED : bool = cfg!(target_env = "msvc");
 
     #[track_caller] unsafe fn realloc_uninit(&self, ptr: AllocNN, new_size: usize) -> Result<AllocNN, Self::Error> {
-        // SAFETY: ✔️ `ptr` belongs to `self` per thin::Realloc's documented safety preconditions, and thus was allocated with one of `malloc`, `calloc`, `realloc, or `_recalloc` - all of which should be safe to `realloc`.
-        let alloc = unsafe { realloc(ptr.as_ptr().cast(), new_size) };
-        NonNull::new(alloc.cast()).ok_or(())
+        if new_size == 0 { // see <Malloc as Meta>::ZST_SUPPORTED rant above
+            let alloc = thin::Alloc::alloc_uninit(self, new_size)?;
+            unsafe { thin::Free::free(self, ptr) };
+            Ok(alloc)
+        } else {
+            // SAFETY: ✔️ `ptr` belongs to `self` per thin::Realloc's documented safety preconditions, and thus was allocated with one of `malloc`, `calloc`, `realloc, or `_recalloc` - all of which should be safe to `realloc`.
+            let alloc = unsafe { realloc(ptr.as_ptr().cast(), new_size) };
+            NonNull::new(alloc.cast()).ok_or(())
+        }
     }
 
     #[track_caller] unsafe fn realloc_zeroed(&self, ptr: AllocNN, new_size: usize) -> Result<AllocNN, Self::Error> {
+        if Self::CAN_REALLOC_ZEROED && new_size == 0 { // see <Malloc as Meta>::ZST_SUPPORTED rant above
+            let alloc = thin::Alloc::alloc_zeroed(self, new_size)?;
+            unsafe { thin::Free::free(self, ptr) };
+            return Ok(alloc.cast());
+        }
+
         #[cfg(target_env = "msvc")] {
             // https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/recalloc
             extern "C" { fn _recalloc(memblock: *mut c_void, num: size_t, size: size_t) -> *mut c_void; }
@@ -195,17 +215,22 @@ unsafe impl thin::SizeOfDebug for Malloc {
 #[test] fn thin_alignment()             { thin::test::alignment(Malloc) }
 #[test] fn thin_edge_case_sizes()       { thin::test::edge_case_sizes(Malloc) }
 #[test] fn thin_nullable()              { thin::test::nullable(Malloc) }
-#[test] fn thin_size()                  { thin::test::size_over_alloc(Malloc) }
+#[cfg(    target_env = "msvc" )] #[test] fn thin_size_msvc()    { thin::test::size_exact_alloc_except_zsts(Malloc) }
+#[cfg(not(target_env = "msvc"))] #[test] fn thin_size()         { thin::test::size_exact_alloc(Malloc) }
 #[test] fn thin_uninit()                { if !MALLOC_ZERO_INITS { unsafe { thin::test::uninit_alloc_unsound(Malloc) } } }
+#[test] fn thin_uninit_realloc()        { thin::test::uninit_realloc(Malloc) }
 #[test] fn thin_zeroed()                { thin::test::zeroed_alloc(Malloc) }
+#[test] fn thin_zeroed_realloc()        { thin::test::zeroed_realloc(Malloc) }
 #[test] fn thin_zst_support()           { thin::test::zst_supported_conservative(Malloc) }
 #[test] fn thin_zst_support_dangle()    { thin::test::zst_supported_conservative(crate::allocator::adapt::DangleZst(Malloc)) }
 #[test] fn thin_zst_support_alloc()     { thin::test::zst_supported_conservative(crate::allocator::adapt::AllocZst(Malloc)) }
 
 #[test] fn fat_alignment()              { fat::test::alignment(Malloc) }
 #[test] fn fat_edge_case_sizes()        { fat::test::edge_case_sizes(Malloc) }
 #[test] fn fat_uninit()                 { if !MALLOC_ZERO_INITS { unsafe { fat::test::uninit_alloc_unsound(Malloc) } } }
+#[test] fn fat_uninit_realloc()         { fat::test::uninit_realloc(Malloc) }
 #[test] fn fat_zeroed()                 { fat::test::zeroed_alloc(Malloc) }
+#[test] fn fat_zeroed_realloc()         { fat::test::zeroed_realloc(Malloc) }
 #[test] fn fat_zst_support()            { fat::test::zst_supported_conservative(Malloc) }
 #[test] fn fat_zst_support_dangle()     { fat::test::zst_supported_conservative(crate::allocator::adapt::DangleZst(Malloc)) }
 #[test] fn fat_zst_support_alloc()      { fat::test::zst_supported_conservative(crate::allocator::adapt::AllocZst(Malloc)) }

src/traits/thin.rs

@@ -352,6 +352,16 @@ pub mod test {
         }
     }
 
+    /// Assert that `allocator` always reports an exact allocation size, except for ZSTs
+    pub fn size_exact_alloc_except_zsts<A: Alloc + Free + SizeOfDebug>(allocator: A) {
+        for size in [0, 1, 3, 7, 15, 31, 63, 127] {
+            let Ok(alloc) = TTB::try_new_uninit(&allocator, size) else { continue };
+            // SAFETY: ✔️ `alloc` belongs to `allocator`
+            let query_size = unsafe { allocator.size_of_debug(alloc.as_nonnull()) }.unwrap_or(size);
+            assert_eq!(size.max(1), query_size, "allocator returns oversized allocs, use thin::test::size_over_alloc instead");
+        }
+    }
+
     /// Assert that `allocator` sometimes reports a larger-than-requested allocation size
     pub fn size_over_alloc<A: Alloc + Free + SizeOfDebug>(allocator: A) {
         let mut any_sized = false;