Skip to content

Commit

Permalink
Rollup merge of #130342 - RalfJung:slice-idx-overflow, r=saethlin
Browse files Browse the repository at this point in the history
interpret, miri: fix dealing with overflow during slice indexing and allocation

This is mostly to fix rust-lang/rust#130284.

I then realized we're using somewhat sketchy arguments for a similar multiplication in `copy`/`copy_nonoverlapping`/`write_bytes`,  so I made them all share the same function that checks exactly the right thing. (The intrinsics would previously fail on allocations larger than `1 << 47` bytes... which are theoretically possible maybe? Anyway it seems conceptually wrong to use any other bound than `isize::MAX` here.)
  • Loading branch information
matthiaskrgr authored Sep 15, 2024
2 parents a785b59 + 1740f8b commit 6a37bae
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 31 deletions.
2 changes: 1 addition & 1 deletion src/concurrency/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// This allocation will be deallocated when the thread dies, so it is not in read-only memory.
alloc.mutability = Mutability::Mut;
// Create a fresh allocation with this content.
let ptr = this.allocate_raw_ptr(alloc, MiriMemoryKind::Tls.into())?;
let ptr = this.insert_allocation(alloc, MiriMemoryKind::Tls.into())?;
this.machine.threads.set_thread_local_alloc(def_id, ptr);
Ok(ptr)
}
Expand Down
17 changes: 2 additions & 15 deletions src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,8 @@
mod atomic;
mod simd;

use std::iter;

use rand::Rng;
use rustc_apfloat::{Float, Round};
use rustc_middle::ty::layout::LayoutOf;
use rustc_middle::{
mir,
ty::{self, FloatTy},
Expand Down Expand Up @@ -119,19 +116,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.copy_op(dest, &place)?;
}

"write_bytes" | "volatile_set_memory" => {
"volatile_set_memory" => {
let [ptr, val_byte, count] = check_arg_count(args)?;
let ty = ptr.layout.ty.builtin_deref(true).unwrap();
let ty_layout = this.layout_of(ty)?;
let val_byte = this.read_scalar(val_byte)?.to_u8()?;
let ptr = this.read_pointer(ptr)?;
let count = this.read_target_usize(count)?;
// `checked_mul` enforces a too small bound (the correct one would probably be target_isize_max),
// but no actual allocation can be big enough for the difference to be noticeable.
let byte_count = ty_layout.size.checked_mul(count, this).ok_or_else(|| {
err_ub_format!("overflow computing total size of `{intrinsic_name}`")
})?;
this.write_bytes_ptr(ptr, iter::repeat(val_byte).take(byte_count.bytes_usize()))?;
this.write_bytes_intrinsic(ptr, val_byte, count, "volatile_set_memory")?;
}

// Memory model / provenance manipulation
Expand Down
48 changes: 36 additions & 12 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
if size == 0 {
throw_ub_format!("creating allocation with size 0");
}
if i128::from(size) > this.tcx.data_layout.pointer_size.signed_int_max() {
if size > this.max_size_of_val().bytes() {
throw_ub_format!("creating an allocation larger than half the address space");
}
if let Err(e) = Align::from_bytes(align) {
Expand Down Expand Up @@ -441,19 +441,34 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'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)?;
this.write_pointer(res, dest)?;
if size <= this.max_size_of_val().bytes() {
let res = this.malloc(size, /*zero_init:*/ false)?;
this.write_pointer(res, dest)?;
} else {
// If this does not fit in an isize, return null and, on Unix, set errno.
if this.target_os_is_unix() {
let einval = this.eval_libc("ENOMEM");
this.set_last_error(einval)?;
}
this.write_null(dest)?;
}
}
"calloc" => {
let [items, len] =
let [items, elem_size] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let items = this.read_target_usize(items)?;
let len = this.read_target_usize(len)?;
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)?;
this.write_pointer(res, dest)?;
let elem_size = this.read_target_usize(elem_size)?;
if let Some(size) = this.compute_size_in_bytes(Size::from_bytes(elem_size), items) {
let res = this.malloc(size.bytes(), /*zero_init:*/ true)?;
this.write_pointer(res, dest)?;
} else {
// On size overflow, return null and, on Unix, set errno.
if this.target_os_is_unix() {
let einval = this.eval_libc("ENOMEM");
this.set_last_error(einval)?;
}
this.write_null(dest)?;
}
}
"free" => {
let [ptr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand All @@ -465,8 +480,17 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
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)?;
this.write_pointer(res, dest)?;
if new_size <= this.max_size_of_val().bytes() {
let res = this.realloc(old_ptr, new_size)?;
this.write_pointer(res, dest)?;
} else {
// If this does not fit in an isize, return null and, on Unix, set errno.
if this.target_os_is_unix() {
let einval = this.eval_libc("ENOMEM");
this.set_last_error(einval)?;
}
this.write_null(dest)?;
}
}

// Rust allocation
Expand Down
4 changes: 2 additions & 2 deletions src/shims/unix/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,14 +363,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
//
// Linux: https://www.unix.com/man-page/linux/3/reallocarray/
// FreeBSD: https://man.freebsd.org/cgi/man.cgi?query=reallocarray
match nmemb.checked_mul(size) {
match this.compute_size_in_bytes(Size::from_bytes(size), nmemb) {
None => {
let einval = this.eval_libc("ENOMEM");
this.set_last_error(einval)?;
this.write_null(dest)?;
}
Some(len) => {
let res = this.realloc(ptr, len)?;
let res = this.realloc(ptr, len.bytes())?;
this.write_pointer(res, dest)?;
}
}
Expand Down
13 changes: 12 additions & 1 deletion tests/pass-dep/libc/libc-mem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ fn test_malloc() {
let slice = slice::from_raw_parts(p3 as *const u8, 20);
assert_eq!(&slice, &[0_u8; 20]);

// new size way too big (so this doesn't actually realloc).
let p_too_big = libc::realloc(p3, usize::MAX);
assert!(p_too_big.is_null());

// old size > new size
let p4 = libc::realloc(p3, 10);
let slice = slice::from_raw_parts(p4 as *const u8, 10);
Expand All @@ -119,9 +123,13 @@ fn test_malloc() {
unsafe {
let p1 = libc::realloc(ptr::null_mut(), 20);
assert!(!p1.is_null());

libc::free(p1);
}

unsafe {
let p_too_big = libc::malloc(usize::MAX);
assert!(p_too_big.is_null());
}
}

fn test_calloc() {
Expand All @@ -143,6 +151,9 @@ fn test_calloc() {
let slice = slice::from_raw_parts(p4 as *const u8, 4 * 8);
assert_eq!(&slice, &[0_u8; 4 * 8]);
libc::free(p4);

let p_too_big = libc::calloc(usize::MAX / 4, 4);
assert!(p_too_big.is_null());
}
}

Expand Down

0 comments on commit 6a37bae

Please sign in to comment.