From 6147260f38e86b5a2c1601d3b3747a7bd08051dc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 9 Jul 2023 16:21:48 +0200 Subject: [PATCH] C string function shims: consistently treat "invalid" pointers as UB --- src/tools/miri/src/shims/foreign_items.rs | 12 ++++++++++++ .../miri/tests/fail/shims/memchr_null.stderr | 4 ++-- .../miri/tests/fail/shims/memcmp_null.stderr | 4 ++-- src/tools/miri/tests/fail/shims/memcmp_zero.rs | 13 +++++++++++++ .../miri/tests/fail/shims/memcmp_zero.stderr | 15 +++++++++++++++ .../miri/tests/fail/shims/memrchr_null.stderr | 4 ++-- 6 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 src/tools/miri/tests/fail/shims/memcmp_zero.rs create mode 100644 src/tools/miri/tests/fail/shims/memcmp_zero.stderr diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index 99bcce6ab74..7996e72615f 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -690,6 +690,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let right = this.read_pointer(right)?; let n = Size::from_bytes(this.read_target_usize(n)?); + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(left)?; + this.ptr_get_alloc_id(right)?; + let result = { let left_bytes = this.read_bytes_ptr_strip_provenance(left, n)?; let right_bytes = this.read_bytes_ptr_strip_provenance(right, n)?; @@ -714,6 +718,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] let val = val as u8; + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(ptr)?; + if let Some(idx) = this .read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(num))? .iter() @@ -738,6 +745,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] let val = val as u8; + // C requires that this must always be a valid pointer (C18 §7.1.4). + this.ptr_get_alloc_id(ptr)?; + let idx = this .read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(num))? .iter() @@ -752,6 +762,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { "strlen" => { let [ptr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; let ptr = this.read_pointer(ptr)?; + // This reads at least 1 byte, so we are already enforcing that this is a valid pointer. let n = this.read_c_str(ptr)?.len(); this.write_scalar( Scalar::from_target_usize(u64::try_from(n).unwrap(), this), @@ -791,6 +802,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // pointer provenance is preserved by this implementation of `strcpy`. // That is probably overly cautious, but there also is no fundamental // reason to have `strcpy` destroy pointer provenance. + // This reads at least 1 byte, so we are already enforcing that this is a valid pointer. let n = this.read_c_str(ptr_src)?.len().checked_add(1).unwrap(); this.mem_copy( ptr_src, diff --git a/src/tools/miri/tests/fail/shims/memchr_null.stderr b/src/tools/miri/tests/fail/shims/memchr_null.stderr index d48606f34ad..54b58f22c6c 100644 --- a/src/tools/miri/tests/fail/shims/memchr_null.stderr +++ b/src/tools/miri/tests/fail/shims/memchr_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memchr_null.rs:LL:CC | LL | libc::memchr(ptr::null(), 0, 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = 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 diff --git a/src/tools/miri/tests/fail/shims/memcmp_null.stderr b/src/tools/miri/tests/fail/shims/memcmp_null.stderr index 7a09c779894..8b2882fc243 100644 --- a/src/tools/miri/tests/fail/shims/memcmp_null.stderr +++ b/src/tools/miri/tests/fail/shims/memcmp_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memcmp_null.rs:LL:CC | LL | libc::memcmp(ptr::null(), ptr::null(), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = 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 diff --git a/src/tools/miri/tests/fail/shims/memcmp_zero.rs b/src/tools/miri/tests/fail/shims/memcmp_zero.rs new file mode 100644 index 00000000000..f2ddc200563 --- /dev/null +++ b/src/tools/miri/tests/fail/shims/memcmp_zero.rs @@ -0,0 +1,13 @@ +//@ignore-target-windows: No libc on Windows +//@compile-flags: -Zmiri-permissive-provenance + +// C says that passing "invalid" pointers is UB for all string functions. +// It is unclear whether `(int*)42` is "invalid", but there is no actually +// a `char` living at that address, so arguably it cannot be a valid pointer. +// Hence this is UB. +fn main() { + let ptr = 42 as *const u8; + unsafe { + libc::memcmp(ptr.cast(), ptr.cast(), 0); //~ERROR: dangling + } +} diff --git a/src/tools/miri/tests/fail/shims/memcmp_zero.stderr b/src/tools/miri/tests/fail/shims/memcmp_zero.stderr new file mode 100644 index 00000000000..e21b9b06008 --- /dev/null +++ b/src/tools/miri/tests/fail/shims/memcmp_zero.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: out-of-bounds pointer use: 0x2a[noalloc] is a dangling pointer (it has no provenance) + --> $DIR/memcmp_zero.rs:LL:CC + | +LL | libc::memcmp(ptr.cast(), ptr.cast(), 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: 0x2a[noalloc] is a dangling pointer (it has no provenance) + | + = 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/memcmp_zero.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/shims/memrchr_null.stderr b/src/tools/miri/tests/fail/shims/memrchr_null.stderr index b5b7630e7fd..cc11ba89f8f 100644 --- a/src/tools/miri/tests/fail/shims/memrchr_null.stderr +++ b/src/tools/miri/tests/fail/shims/memrchr_null.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: memory access failed: null pointer is a dangling pointer (it has no provenance) +error: Undefined Behavior: out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) --> $DIR/memrchr_null.rs:LL:CC | LL | libc::memrchr(ptr::null(), 0, 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: null pointer is a dangling pointer (it has no provenance) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: null pointer is a dangling pointer (it has no provenance) | = 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