Skip to content

Commit

Permalink
enable number validity checking and ptr::invalid checking by default
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed May 23, 2022
1 parent 31fb32e commit 494d23c
Show file tree
Hide file tree
Showing 27 changed files with 76 additions and 90 deletions.
14 changes: 9 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ in your program, and cannot run all programs:
positives here, so if your program runs fine in Miri right now that is by no
means a guarantee that it is UB-free when these questions get answered.

In particular, Miri does currently not check that integers/floats are
initialized or that references point to valid data.
In particular, Miri does currently not check that references point to valid data.
* If the program relies on unspecified details of how data is laid out, it will
still run fine in Miri -- but might break (including causing UB) on different
compiler versions or different platforms.
Expand Down Expand Up @@ -302,10 +301,15 @@ The remaining flags are for advanced use only, and more likely to change or be r
Some of these are **unsound**, which means they can lead
to Miri failing to detect cases of undefined behavior in a program.

* `-Zmiri-check-number-validity` enables checking of integer and float validity
(e.g., they must be initialized and not carry pointer provenance) as part of
enforcing validity invariants. This has no effect when
* `-Zmiri-allow-uninit-numbers` disables the check to ensure that number types (integer and float
types) always hold initialized data. (They must still be initialized when any actual operation,
such as arithmetic, is performed.) Using this flag is **unsound**. This has no effect when
`-Zmiri-disable-validation` is present.
* `-Zmiri-allow-ptr-int-transmute` makes Miri more accepting of transmutation between pointers and
integers via `mem::transmute` or union/pointer type punning. This has two effects: it disables the
check against integers storing a pointer (i.e., data with provenance), thus allowing
pointer-to-integer transmutation, and it treats integer-to-pointer transmutation as equivalent to
a cast. Using this flag is **unsound**.
* `-Zmiri-disable-abi-check` disables checking [function ABI]. Using this flag
is **unsound**.
* `-Zmiri-disable-alignment-check` disables checking pointer alignment, so you
Expand Down
12 changes: 10 additions & 2 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,16 @@ fn main() {
miri_config.check_alignment = miri::AlignmentCheck::Symbolic;
}
"-Zmiri-check-number-validity" => {
miri_config.check_number_validity = true;
eprintln!(
"WARNING: the flag `-Zmiri-check-number-validity` no longer has any effect \
since it is now enabled by default"
);
}
"-Zmiri-allow-uninit-numbers" => {
miri_config.allow_uninit_numbers = true;
}
"-Zmiri-allow-ptr-int-transmute" => {
miri_config.allow_ptr_int_transmute = true;
}
"-Zmiri-disable-abi-check" => {
miri_config.check_abi = false;
Expand Down Expand Up @@ -386,7 +395,6 @@ fn main() {
"-Zmiri-strict-provenance" => {
miri_config.provenance_mode = ProvenanceMode::Strict;
miri_config.tag_raw = true;
miri_config.check_number_validity = true;
}
"-Zmiri-permissive-provenance" => {
miri_config.provenance_mode = ProvenanceMode::Permissive;
Expand Down
9 changes: 6 additions & 3 deletions src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@ pub struct MiriConfig {
pub stacked_borrows: bool,
/// Controls alignment checking.
pub check_alignment: AlignmentCheck,
/// Controls integer and float validity (e.g., initialization) checking.
pub check_number_validity: bool,
/// Controls integer and float validity initialization checking.
pub allow_uninit_numbers: bool,
/// Controls how we treat ptr2int and int2ptr transmutes.
pub allow_ptr_int_transmute: bool,
/// Controls function [ABI](Abi) checking.
pub check_abi: bool,
/// Action for an op requiring communication with the host.
Expand Down Expand Up @@ -126,7 +128,8 @@ impl Default for MiriConfig {
validate: true,
stacked_borrows: true,
check_alignment: AlignmentCheck::Int,
check_number_validity: false,
allow_uninit_numbers: false,
allow_ptr_int_transmute: false,
check_abi: true,
isolated_op: IsolatedOp::Reject(RejectOpWith::Abort),
ignore_leaks: false,
Expand Down
19 changes: 6 additions & 13 deletions src/intptrcast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,19 +118,12 @@ impl<'mir, 'tcx> GlobalStateInner {
) -> Pointer<Option<Tag>> {
trace!("Transmuting 0x{:x} to a pointer", addr);

let global_state = ecx.machine.intptrcast.borrow();

match global_state.provenance_mode {
ProvenanceMode::Legacy => {
// In legacy mode, we have to support int2ptr transmutes,
// so just pretend they do the same thing as a cast.
Self::ptr_from_addr_cast(ecx, addr)
}
ProvenanceMode::Permissive | ProvenanceMode::Strict => {
// Both of these modes consider transmuted pointers to be "invalid" (`None`
// provenance).
Pointer::new(None, Size::from_bytes(addr))
}
if ecx.machine.allow_ptr_int_transmute {
// When we allow transmutes, treat them like casts.
Self::ptr_from_addr_cast(ecx, addr)
} else {
// We consider transmuted pointers to be "invalid" (`None` provenance).
Pointer::new(None, Size::from_bytes(addr))
}
}

Expand Down
17 changes: 12 additions & 5 deletions src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,13 +255,19 @@ pub struct Evaluator<'mir, 'tcx> {
/// Whether to enforce the validity invariant.
pub(crate) validate: bool,

/// Whether to enforce validity (e.g., initialization) of integers and floats.
pub(crate) enforce_number_validity: bool,
/// Whether to allow uninitialized numbers (integers and floats).
pub(crate) allow_uninit_numbers: bool,

/// Whether to allow ptr2int transmutes, and whether to allow *dereferencing* the result of an
/// int2ptr transmute.
pub(crate) allow_ptr_int_transmute: bool,

/// Whether to enforce [ABI](Abi) of function calls.
pub(crate) enforce_abi: bool,

/// The table of file descriptors.
pub(crate) file_handler: shims::posix::FileHandler,
/// The table of directory descriptors.
pub(crate) dir_handler: shims::posix::DirHandler,

/// The "time anchor" for this machine's monotone clock (for `Instant` simulation).
Expand Down Expand Up @@ -351,7 +357,8 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
tls: TlsData::default(),
isolated_op: config.isolated_op,
validate: config.validate,
enforce_number_validity: config.check_number_validity,
allow_uninit_numbers: config.allow_uninit_numbers,
allow_ptr_int_transmute: config.allow_ptr_int_transmute,
enforce_abi: config.check_abi,
file_handler: FileHandler::new(config.mute_stdout_stderr),
dir_handler: Default::default(),
Expand Down Expand Up @@ -493,12 +500,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {

#[inline(always)]
fn enforce_number_init(ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
ecx.machine.enforce_number_validity
!ecx.machine.allow_uninit_numbers
}

#[inline(always)]
fn enforce_number_no_provenance(ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
ecx.machine.enforce_number_validity
!ecx.machine.allow_ptr_int_transmute
}

#[inline(always)]
Expand Down
3 changes: 1 addition & 2 deletions src/shims/posix/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx

// To catch double-destroys, we de-initialize the mutexattr.
// This is technically not right and might lead to false positives. For example, the below
// code is *likely* sound, even assuming uninit numbers are UB, but miri with
// -Zmiri-check-number-validity complains
// code is *likely* sound, even assuming uninit numbers are UB, but Miri complains.
//
// let mut x: MaybeUninit<libc::pthread_mutexattr_t> = MaybeUninit::zeroed();
// libc::pthread_mutexattr_init(x.as_mut_ptr());
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/provenance/ptr_int_unexposed.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// compile-flags: -Zmiri-permissive-provenance -Zmiri-disable-stacked-borrows
// compile-flags: -Zmiri-permissive-provenance -Zmiri-disable-stacked-borrows -Zmiri-allow-ptr-int-transmute

fn main() {
let x: i32 = 3;
Expand Down
9 changes: 9 additions & 0 deletions tests/compile-fail/provenance/ptr_invalid.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![feature(strict_provenance)]

// Ensure that a `ptr::invalid` ptr is truly invalid.
fn main() {
let x = 42;
let xptr = &x as *const i32;
let xptr_invalid = std::ptr::invalid::<i32>(xptr.expose_addr());
let _val = unsafe { *xptr_invalid }; //~ ERROR is not a valid pointer
}
1 change: 1 addition & 0 deletions tests/compile-fail/stacked_borrows/illegal_read3.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// compile-flags: -Zmiri-allow-ptr-int-transmute
// A callee may not read the destination of our `&mut` without us noticing.
// Thise code got carefully checked to not introduce any reborrows
// that are not explicit in the source. Let's hope the compiler does not break this later!
Expand Down
1 change: 1 addition & 0 deletions tests/compile-fail/transmute-pair-uninit.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// compile-flags: -Zmiri-allow-uninit-numbers
#![feature(core_intrinsics)]

use std::mem;
Expand Down
1 change: 1 addition & 0 deletions tests/compile-fail/uninit_byte_read.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// compile-flags: -Zmiri-allow-uninit-numbers
fn main() {
let v: Vec<u8> = Vec::with_capacity(10);
let undef = unsafe { *v.get_unchecked(5) };
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// compile-flags: -Zmiri-allow-uninit-numbers
#![allow(unused, deprecated, invalid_value)]

#[derive(Copy, Clone)]
Expand Down
2 changes: 0 additions & 2 deletions tests/compile-fail/validity/ptr_integer_array_transmute.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

fn main() {
let r = &mut 42;
let _i: [usize; 1] = unsafe { std::mem::transmute(r) }; //~ ERROR encountered a pointer, but expected plain (non-pointer) bytes
Expand Down
2 changes: 0 additions & 2 deletions tests/compile-fail/validity/ptr_integer_transmute.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

fn main() {
let r = &mut 42;
let _i: usize = unsafe { std::mem::transmute(r) }; //~ ERROR expected plain (non-pointer) bytes
Expand Down
2 changes: 0 additions & 2 deletions tests/compile-fail/validity/uninit_float.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

// This test is adapted from https://github.com/rust-lang/miri/issues/1340#issue-600900312.

fn main() {
Expand Down
2 changes: 0 additions & 2 deletions tests/compile-fail/validity/uninit_integer.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

// This test is from https://github.com/rust-lang/miri/issues/1340#issue-600900312.

fn main() {
Expand Down
2 changes: 0 additions & 2 deletions tests/compile-fail/validity/uninit_integer_signed.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

// This test is adapted from https://github.com/rust-lang/miri/issues/1340#issue-600900312.

fn main() {
Expand Down
36 changes: 0 additions & 36 deletions tests/run-pass/bitop-beyond-alignment.rs

This file was deleted.

2 changes: 1 addition & 1 deletion tests/run-pass/concurrency/libc_pthread_cond.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// ignore-windows: No libc on Windows
// ignore-macos: pthread_condattr_setclock is not supported on MacOS.
// compile-flags: -Zmiri-disable-isolation -Zmiri-check-number-validity
// compile-flags: -Zmiri-disable-isolation

#![feature(rustc_private)]

Expand Down
2 changes: 2 additions & 0 deletions tests/run-pass/intptrcast.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// compile-flags: -Zmiri-allow-ptr-int-transmute

// This returns a miri pointer at type usize, if the argument is a proper pointer
fn transmute_ptr_to_int<T>(x: *const T) -> usize {
unsafe { std::mem::transmute(x) }
Expand Down
10 changes: 5 additions & 5 deletions tests/run-pass/libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,17 @@ fn test_prctl_thread_name() {
use libc::c_long;
unsafe {
let mut buf = [255; 10];
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr() as c_long, 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr(), 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(b"<unnamed>\0", &buf);
let thread_name = CString::new("hello").expect("CString::new failed");
assert_eq!(libc::prctl(libc::PR_SET_NAME, thread_name.as_ptr() as c_long, 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(libc::prctl(libc::PR_SET_NAME, thread_name.as_ptr(), 0 as c_long, 0 as c_long, 0 as c_long), 0);
let mut buf = [255; 6];
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr() as c_long, 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr(), 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(b"hello\0", &buf);
let long_thread_name = CString::new("01234567890123456789").expect("CString::new failed");
assert_eq!(libc::prctl(libc::PR_SET_NAME, long_thread_name.as_ptr() as c_long, 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(libc::prctl(libc::PR_SET_NAME, long_thread_name.as_ptr(), 0 as c_long, 0 as c_long, 0 as c_long), 0);
let mut buf = [255; 16];
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr() as c_long, 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(libc::prctl(libc::PR_GET_NAME, buf.as_mut_ptr(), 0 as c_long, 0 as c_long, 0 as c_long), 0);
assert_eq!(b"012345678901234\0", &buf);
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/run-pass/move-uninit-primval.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// compile-flags: -Zmiri-allow-uninit-numbers
#![allow(deprecated)]

struct Foo {
Expand Down
2 changes: 0 additions & 2 deletions tests/run-pass/partially-uninit.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// compile-flags: -Zmiri-check-number-validity

use std::mem::{self, MaybeUninit};

#[repr(C)]
Expand Down
4 changes: 3 additions & 1 deletion tests/run-pass/ptr_offset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ fn ptr_offset() {
unsafe {
let p = f as fn() -> i32 as usize;
let x = (p as *mut u32).offset(0) as usize;
let f: fn() -> i32 = mem::transmute(x);
// *cast* to ptr, then transmute to fn ptr.
// (transmuting int to [fn]ptr causes trouble.)
let f: fn() -> i32 = mem::transmute(x as *const ());
assert_eq!(f(), 42);
}
}
3 changes: 2 additions & 1 deletion tests/run-pass/tag-align-dyn-u64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ fn mk_rec() -> Rec {
}

fn is_u64_aligned(u: &Tag<u64>) -> bool {
let p: usize = unsafe { mem::transmute(u) };
let p: *const () = unsafe { mem::transmute(u) };
let p = p as usize;
let u64_align = std::mem::align_of::<u64>();
return (p & (u64_align - 1)) == 0;
}
Expand Down
5 changes: 3 additions & 2 deletions tests/run-pass/transmute_fat.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
// Stacked Borrows disallows this becuase the reference is never cast to a raw pointer.
// compile-flags: -Zmiri-disable-stacked-borrows
// compile-flags: -Zmiri-disable-stacked-borrows -Zmiri-allow-ptr-int-transmute

fn main() {
// If we are careful, we can exploit data layout...
let raw = unsafe {
std::mem::transmute::<&[u8], [usize; 2]>(&[42])
};
let ptr = raw[0] + raw[1];
let ptr = ptr as *const u8;
// We transmute both ways, to really test allow-ptr-int-transmute.
let ptr: *const u8 = unsafe { std::mem::transmute(ptr) };
// The pointer is one-past-the end, but we decrement it into bounds before using it
assert_eq!(unsafe { *ptr.offset(-1) }, 42);
}
2 changes: 1 addition & 1 deletion tests/run-pass/uninit_number_ignored.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// compile-flags: -Zmiri-allow-uninit-numbers
// This test is adapted from https://github.com/rust-lang/miri/issues/1340#issue-600900312.
// This test passes because -Zmiri-check-number-validity is not passed.

fn main() {
let _val1 = unsafe { std::mem::MaybeUninit::<usize>::uninit().assume_init() };
Expand Down

0 comments on commit 494d23c

Please sign in to comment.