From 90970e31999466f06d45b4842ddc2392edcf1a32 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 17 Jun 2024 11:41:18 +0200 Subject: [PATCH] show proper UB when making a too large allocation request --- src/alloc_bytes.rs | 24 ++++++++++++++---------- src/shims/alloc.rs | 12 ------------ src/shims/foreign_items.rs | 24 ++++++++++++++++++++---- tests/fail/alloc/too_large.rs | 10 ++++++++++ tests/fail/alloc/too_large.stderr | 15 +++++++++++++++ 5 files changed, 59 insertions(+), 26 deletions(-) create mode 100644 tests/fail/alloc/too_large.rs create mode 100644 tests/fail/alloc/too_large.stderr diff --git a/src/alloc_bytes.rs b/src/alloc_bytes.rs index 97841a05cd..8757929300 100644 --- a/src/alloc_bytes.rs +++ b/src/alloc_bytes.rs @@ -64,17 +64,19 @@ impl MiriAllocBytes { /// If `size == 0` we allocate using a different `alloc_layout` with `size = 1`, to ensure each allocation has a unique address. /// Returns `Err(alloc_layout)` if the allocation function returns a `ptr` where `ptr.is_null()`. fn alloc_with( - size: usize, - align: usize, + size: u64, + align: u64, alloc_fn: impl FnOnce(Layout) -> *mut u8, - ) -> Result { - let layout = Layout::from_size_align(size, align).unwrap(); + ) -> Result { + let size = usize::try_from(size).map_err(|_| ())?; + let align = usize::try_from(align).map_err(|_| ())?; + let layout = Layout::from_size_align(size, align).map_err(|_| ())?; // When size is 0 we allocate 1 byte anyway, to ensure each allocation has a unique address. let alloc_layout = if size == 0 { Layout::from_size_align(1, align).unwrap() } else { layout }; let ptr = alloc_fn(alloc_layout); if ptr.is_null() { - Err(alloc_layout) + Err(()) } else { // SAFETY: All `MiriAllocBytes` invariants are fulfilled. Ok(Self { ptr, layout }) @@ -86,11 +88,13 @@ impl AllocBytes for MiriAllocBytes { fn from_bytes<'a>(slice: impl Into>, align: Align) -> Self { let slice = slice.into(); let size = slice.len(); - let align = align.bytes_usize(); + let align = align.bytes(); // SAFETY: `alloc_fn` will only be used with `size != 0`. let alloc_fn = |layout| unsafe { alloc::alloc(layout) }; - let alloc_bytes = MiriAllocBytes::alloc_with(size, align, alloc_fn) - .unwrap_or_else(|layout| alloc::handle_alloc_error(layout)); + let alloc_bytes = MiriAllocBytes::alloc_with(size.try_into().unwrap(), align, alloc_fn) + .unwrap_or_else(|()| { + panic!("Miri ran out of memory: cannot create allocation of {size} bytes") + }); // SAFETY: `alloc_bytes.ptr` and `slice.as_ptr()` are non-null, properly aligned // and valid for the `size`-many bytes to be copied. unsafe { alloc_bytes.ptr.copy_from(slice.as_ptr(), size) }; @@ -98,8 +102,8 @@ impl AllocBytes for MiriAllocBytes { } fn zeroed(size: Size, align: Align) -> Option { - let size = size.bytes_usize(); - let align = align.bytes_usize(); + let size = size.bytes(); + let align = align.bytes(); // SAFETY: `alloc_fn` will only be used with `size != 0`. let alloc_fn = |layout| unsafe { alloc::alloc_zeroed(layout) }; MiriAllocBytes::alloc_with(size, align, alloc_fn).ok() diff --git a/src/shims/alloc.rs b/src/shims/alloc.rs index 7b0c54d763..a33657d33a 100644 --- a/src/shims/alloc.rs +++ b/src/shims/alloc.rs @@ -5,18 +5,6 @@ use rustc_target::abi::{Align, Size}; use crate::*; -/// Check some basic requirements for this allocation request: -/// non-zero size, power-of-two alignment. -pub(super) fn check_alloc_request<'tcx>(size: u64, align: u64) -> InterpResult<'tcx> { - if size == 0 { - throw_ub_format!("creating allocation with size 0"); - } - if !align.is_power_of_two() { - throw_ub_format!("creating allocation with non-power-of-two alignment {}", align); - } - Ok(()) -} - impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// Returns the alignment that `malloc` would guarantee for requests of the given size. diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 898fc111fd..b8d85a1950 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -12,7 +12,7 @@ use rustc_target::{ spec::abi::Abi, }; -use super::alloc::{check_alloc_request, EvalContextExt as _}; +use super::alloc::EvalContextExt as _; use super::backtrace::EvalContextExt as _; use crate::*; use helpers::{ToHost, ToSoft}; @@ -204,6 +204,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { impl<'tcx> EvalContextExtPriv<'tcx> for crate::MiriInterpCx<'tcx> {} trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { + /// Check some basic requirements for this allocation request: + /// non-zero size, power-of-two alignment. + fn check_rustc_alloc_request(&self, size: u64, align: u64) -> InterpResult<'tcx> { + let this = self.eval_context_ref(); + if size == 0 { + throw_ub_format!("creating allocation with size 0"); + } + if i128::from(size) > this.tcx.data_layout.pointer_size.signed_int_max() { + throw_ub_format!("creating an allocation larger than half the address space"); + } + if !align.is_power_of_two() { + throw_ub_format!("creating allocation with non-power-of-two alignment {}", align); + } + Ok(()) + } + fn emulate_foreign_item_inner( &mut self, link_name: Symbol, @@ -462,7 +478,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { let size = this.read_target_usize(size)?; let align = this.read_target_usize(align)?; - check_alloc_request(size, align)?; + this.check_rustc_alloc_request(size, align)?; let memory_kind = match link_name.as_str() { "__rust_alloc" => MiriMemoryKind::Rust, @@ -496,7 +512,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { let size = this.read_target_usize(size)?; let align = this.read_target_usize(align)?; - check_alloc_request(size, align)?; + this.check_rustc_alloc_request(size, align)?; let ptr = this.allocate_ptr( Size::from_bytes(size), @@ -560,7 +576,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> { let new_size = this.read_target_usize(new_size)?; // No need to check old_size; we anyway check that they match the allocation. - check_alloc_request(new_size, align)?; + this.check_rustc_alloc_request(new_size, align)?; let align = Align::from_bytes(align).unwrap(); let new_ptr = this.reallocate_ptr( diff --git a/tests/fail/alloc/too_large.rs b/tests/fail/alloc/too_large.rs new file mode 100644 index 0000000000..4e28d2401d --- /dev/null +++ b/tests/fail/alloc/too_large.rs @@ -0,0 +1,10 @@ +extern "Rust" { + fn __rust_alloc(size: usize, align: usize) -> *mut u8; +} + +fn main() { + let bytes = isize::MAX as usize + 1; + unsafe { + __rust_alloc(bytes, 1); //~ERROR: larger than half the address space + } +} diff --git a/tests/fail/alloc/too_large.stderr b/tests/fail/alloc/too_large.stderr new file mode 100644 index 0000000000..77dcf91d84 --- /dev/null +++ b/tests/fail/alloc/too_large.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: creating an allocation larger than half the address space + --> $DIR/too_large.rs:LL:CC + | +LL | __rust_alloc(bytes, 1); + | ^^^^^^^^^^^^^^^^^^^^^^ creating an allocation larger than half the address space + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/too_large.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +