diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 1680b98eca33f..54eb6a3bd6699 100644 --- a/src/tools/miri/src/lib.rs +++ b/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, diff --git a/src/tools/miri/src/shims/alloc.rs b/src/tools/miri/src/shims/alloc.rs index 4eefb8b439b40..1deb9a5654edf 100644 --- a/src/tools/miri/src/shims/alloc.rs +++ b/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: - // 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>> { 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>, - kind: MiriMemoryKind, - ) -> InterpResult<'tcx> { + fn free(&mut self, ptr: Pointer>) -> 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>, new_size: u64, - kind: MiriMemoryKind, ) -> InterpResult<'tcx, Pointer>> { 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()) } diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index 1185d440ec681..d431c28d55a56 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/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)?; } diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index aaf47248af629..08f64e499b558 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/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)?; } } diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index 44b0f25b55c0f..086abf19c5cff 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/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,8 +247,21 @@ 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: + 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] = @@ -257,23 +269,41 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { 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)?; }