Auto merge of rust-lang#3598 - RalfJung:heap, r=RalfJung

alloc: update comments around malloc() alignment

Also separate the C heap shims form the Windows heap shims; their guarantees aren't quite the same.

src/tools/miri/src/lib.rs

@@ -12,6 +12,7 @@
 #![feature(let_chains)]
 #![feature(lint_reasons)]
 #![feature(trait_upcasting)]
+#![feature(strict_overflow_ops)]
 // Configure clippy and other lints
 #![allow(
     clippy::collapsible_else_if,

src/tools/miri/src/shims/alloc.rs

@@ -19,23 +19,34 @@ pub(super) fn check_alloc_request<'tcx>(size: u64, align: u64) -> InterpResult<'
 
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
-    /// Returns the minimum alignment for the target architecture for allocations of the given size.
-    fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align {
+    /// Returns the alignment that `malloc` would guarantee for requests of the given size.
+    fn malloc_align(&self, size: u64) -> Align {
         let this = self.eval_context_ref();
-        // List taken from `library/std/src/sys/pal/common/alloc.rs`.
-        // This list should be kept in sync with the one from libstd.
-        let min_align = match this.tcx.sess.target.arch.as_ref() {
+        // The C standard says: "The pointer returned if the allocation succeeds is suitably aligned
+        // so that it may be assigned to a pointer to any type of object with a fundamental
+        // alignment requirement and size less than or equal to the size requested."
+        // So first we need to figure out what the limits are for "fundamental alignment".
+        // This is given by `alignof(max_align_t)`. The following list is taken from
+        // `library/std/src/sys/pal/common/alloc.rs` (where this is called `MIN_ALIGN`) and should
+        // be kept in sync.
+        let max_fundamental_align = match this.tcx.sess.target.arch.as_ref() {
             "x86" | "arm" | "mips" | "mips32r6" | "powerpc" | "powerpc64" | "wasm32" => 8,
             "x86_64" | "aarch64" | "mips64" | "mips64r6" | "s390x" | "sparc64" | "loongarch64" =>
                 16,
             arch => bug!("unsupported target architecture for malloc: `{}`", arch),
         };
-        // Windows always aligns, even small allocations.
-        // Source: <https://support.microsoft.com/en-us/help/286470/how-to-use-pageheap-exe-in-windows-xp-windows-2000-and-windows-server>
-        // But jemalloc does not, so for the C heap we only align if the allocation is sufficiently big.
-        if kind == MiriMemoryKind::WinHeap || size >= min_align {
-            return Align::from_bytes(min_align).unwrap();
+        // The C standard only requires sufficient alignment for any *type* with size less than or
+        // equal to the size requested. Types one can define in standard C seem to never have an alignment
+        // bigger than their size. So if the size is 2, then only alignment 2 is guaranteed, even if
+        // `max_fundamental_align` is bigger.
+        // This matches what some real-world implementations do, see e.g.
+        // - https://github.com/jemalloc/jemalloc/issues/1533
+        // - https://github.com/llvm/llvm-project/issues/53540
+        // - https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm
+        if size >= max_fundamental_align {
+            return Align::from_bytes(max_fundamental_align).unwrap();
         }
+        // C doesn't have zero-sized types, so presumably nothing is guaranteed here.
         if size == 0 {
             return Align::ONE;
         }
@@ -85,11 +96,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         &mut self,
         size: u64,
         zero_init: bool,
-        kind: MiriMemoryKind,
     ) -> InterpResult<'tcx, Pointer<Option<Provenance>>> {
         let this = self.eval_context_mut();
-        let align = this.min_align(size, kind);
-        let ptr = this.allocate_ptr(Size::from_bytes(size), align, kind.into())?;
+        let align = this.malloc_align(size);
+        let ptr = this.allocate_ptr(Size::from_bytes(size), align, MiriMemoryKind::C.into())?;
         if zero_init {
             // We just allocated this, the access is definitely in-bounds and fits into our address space.
             this.write_bytes_ptr(
@@ -101,14 +111,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         Ok(ptr.into())
     }
 
-    fn free(
-        &mut self,
-        ptr: Pointer<Option<Provenance>>,
-        kind: MiriMemoryKind,
-    ) -> InterpResult<'tcx> {
+    fn free(&mut self, ptr: Pointer<Option<Provenance>>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         if !this.ptr_is_null(ptr)? {
-            this.deallocate_ptr(ptr, None, kind.into())?;
+            this.deallocate_ptr(ptr, None, MiriMemoryKind::C.into())?;
         }
         Ok(())
     }
@@ -117,13 +123,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         &mut self,
         old_ptr: Pointer<Option<Provenance>>,
         new_size: u64,
-        kind: MiriMemoryKind,
     ) -> InterpResult<'tcx, Pointer<Option<Provenance>>> {
         let this = self.eval_context_mut();
-        let new_align = this.min_align(new_size, kind);
+        let new_align = this.malloc_align(new_size);
         if this.ptr_is_null(old_ptr)? {
             // Here we must behave like `malloc`.
-            self.malloc(new_size, /*zero_init*/ false, kind)
+            self.malloc(new_size, /*zero_init*/ false)
         } else {
             if new_size == 0 {
                 // C, in their infinite wisdom, made this UB.
@@ -135,7 +140,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     None,
                     Size::from_bytes(new_size),
                     new_align,
-                    kind.into(),
+                    MiriMemoryKind::C.into(),
                 )?;
                 Ok(new_ptr.into())
             }

src/tools/miri/src/shims/foreign_items.rs

@@ -421,7 +421,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             "malloc" => {
                 let [size] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 let size = this.read_target_usize(size)?;
-                let res = this.malloc(size, /*zero_init:*/ false, MiriMemoryKind::C)?;
+                let res = this.malloc(size, /*zero_init:*/ false)?;
                 this.write_pointer(res, dest)?;
             }
             "calloc" => {
@@ -432,20 +432,20 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let size = items
                     .checked_mul(len)
                     .ok_or_else(|| err_ub_format!("overflow during calloc size computation"))?;
-                let res = this.malloc(size, /*zero_init:*/ true, MiriMemoryKind::C)?;
+                let res = this.malloc(size, /*zero_init:*/ true)?;
                 this.write_pointer(res, dest)?;
             }
             "free" => {
                 let [ptr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 let ptr = this.read_pointer(ptr)?;
-                this.free(ptr, MiriMemoryKind::C)?;
+                this.free(ptr)?;
             }
             "realloc" => {
                 let [old_ptr, new_size] =
                     this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 let old_ptr = this.read_pointer(old_ptr)?;
                 let new_size = this.read_target_usize(new_size)?;
-                let res = this.realloc(old_ptr, new_size, MiriMemoryKind::C)?;
+                let res = this.realloc(old_ptr, new_size)?;
                 this.write_pointer(res, dest)?;
             }
 

src/tools/miri/src/shims/unix/foreign_items.rs

@@ -310,7 +310,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                         this.write_null(dest)?;
                     }
                     Some(len) => {
-                        let res = this.realloc(ptr, len, MiriMemoryKind::C)?;
+                        let res = this.realloc(ptr, len)?;
                         this.write_pointer(res, dest)?;
                     }
                 }

src/tools/miri/src/shims/windows/foreign_items.rs

@@ -5,10 +5,9 @@ use std::path::{self, Path, PathBuf};
 use std::str;
 
 use rustc_span::Symbol;
-use rustc_target::abi::Size;
+use rustc_target::abi::{Align, Size};
 use rustc_target::spec::abi::Abi;
 
-use crate::shims::alloc::EvalContextExt as _;
 use crate::shims::os_str::bytes_to_os_str;
 use crate::shims::windows::*;
 use crate::*;
@@ -248,32 +247,63 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let size = this.read_target_usize(size)?;
                 let heap_zero_memory = 0x00000008; // HEAP_ZERO_MEMORY
                 let zero_init = (flags & heap_zero_memory) == heap_zero_memory;
-                let res = this.malloc(size, zero_init, MiriMemoryKind::WinHeap)?;
-                this.write_pointer(res, dest)?;
+                // Alignment is twice the pointer size.
+                // Source: <https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapalloc>
+                let align = this.tcx.pointer_size().bytes().strict_mul(2);
+                let ptr = this.allocate_ptr(
+                    Size::from_bytes(size),
+                    Align::from_bytes(align).unwrap(),
+                    MiriMemoryKind::WinHeap.into(),
+                )?;
+                if zero_init {
+                    this.write_bytes_ptr(
+                        ptr.into(),
+                        iter::repeat(0u8).take(usize::try_from(size).unwrap()),
+                    )?;
+                }
+                this.write_pointer(ptr, dest)?;
             }
             "HeapFree" => {
                 let [handle, flags, ptr] =
                     this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
                 this.read_target_isize(handle)?;
                 this.read_scalar(flags)?.to_u32()?;
                 let ptr = this.read_pointer(ptr)?;
-                this.free(ptr, MiriMemoryKind::WinHeap)?;
+                // "This pointer can be NULL." It doesn't say what happens then, but presumably nothing.
+                // (https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapfree)
+                if !this.ptr_is_null(ptr)? {
+                    this.deallocate_ptr(ptr, None, MiriMemoryKind::WinHeap.into())?;
+                }
                 this.write_scalar(Scalar::from_i32(1), dest)?;
             }
             "HeapReAlloc" => {
-                let [handle, flags, ptr, size] =
+                let [handle, flags, old_ptr, size] =
                     this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
                 this.read_target_isize(handle)?;
                 this.read_scalar(flags)?.to_u32()?;
-                let ptr = this.read_pointer(ptr)?;
+                let old_ptr = this.read_pointer(old_ptr)?;
                 let size = this.read_target_usize(size)?;
-                let res = this.realloc(ptr, size, MiriMemoryKind::WinHeap)?;
-                this.write_pointer(res, dest)?;
+                let align = this.tcx.pointer_size().bytes().strict_mul(2); // same as above
+                // The docs say that `old_ptr` must come from an earlier HeapAlloc or HeapReAlloc,
+                // so unlike C `realloc` we do *not* allow a NULL here.
+                // (https://learn.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heaprealloc)
+                let new_ptr = this.reallocate_ptr(
+                    old_ptr,
+                    None,
+                    Size::from_bytes(size),
+                    Align::from_bytes(align).unwrap(),
+                    MiriMemoryKind::WinHeap.into(),
+                )?;
+                this.write_pointer(new_ptr, dest)?;
             }
             "LocalFree" => {
                 let [ptr] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
                 let ptr = this.read_pointer(ptr)?;
-                this.free(ptr, MiriMemoryKind::WinLocal)?;
+                // "If the hMem parameter is NULL, LocalFree ignores the parameter and returns NULL."
+                // (https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-localfree)
+                if !this.ptr_is_null(ptr)? {
+                    this.deallocate_ptr(ptr, None, MiriMemoryKind::WinLocal.into())?;
+                }
                 this.write_null(dest)?;
             }