Skip to content

Validate more things #477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,26 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> {

const STATIC_KIND: Option<MiriMemoryKind> = Some(MiriMemoryKind::MutStatic);

#[inline(always)]
fn enforce_validity(ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool {
ecx.machine.validate
if !ecx.machine.validate {
return false;
}

// Some functions are whitelisted until we figure out how to fix them.
// We walk up the stack a few frames to also cover their callees.
const WHITELIST: &[&str] = &[
// Uses mem::uninitialized
"std::ptr::read",
];
for frame in ecx.stack().iter()
.rev().take(3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, the last 3 stackframes!?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah well, I had to pick some number. I didn't want to manually whitelist all callees, but also didn't want to walk up the entire stack.

It's a hack to even have a whitelist, so... whatever?^^

{
let name = frame.instance.to_string();
if WHITELIST.iter().any(|white| name.starts_with(white)) {
return false;
}
}
true
}

/// Returns Ok() when the function was handled, fail otherwise
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/cast_box_int_to_fn_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

fn main() {
let b = Box::new(42);
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/cast_int_to_fn_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

fn main() {
let g = unsafe {
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/execute_memory.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

#![feature(box_syntax)]

Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/fn_ptr_offset.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

use std::mem;

Expand Down
8 changes: 1 addition & 7 deletions tests/compile-fail/invalid_bool.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
//ignore-test FIXME: do some basic validation of invariants for all values in flight
//This does currently not get caught becuase it compiles to SwitchInt, which
//has no knowledge about data invariants.

fn main() {
let b = unsafe { std::mem::transmute::<u8, bool>(2) };
if b { unreachable!() } else { unreachable!() } //~ ERROR constant evaluation error
//~^ NOTE invalid boolean value read
let _b = unsafe { std::mem::transmute::<u8, bool>(2) }; //~ ERROR encountered 2, but expected something in the range 0..=1
}
3 changes: 3 additions & 0 deletions tests/compile-fail/invalid_bool2.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

fn main() {
let b = unsafe { std::mem::transmute::<u8, bool>(2) };
let _x = b == true; //~ ERROR invalid boolean value read
Expand Down
8 changes: 8 additions & 0 deletions tests/compile-fail/invalid_char.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
fn main() {
assert!(std::char::from_u32(-1_i32 as u32).is_none());
let _ = match unsafe { std::mem::transmute::<i32, char>(-1) } { //~ ERROR encountered 4294967295, but expected something in the range 0..=1114111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

char has a range? what's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does? It must be a valid unicode codepoint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought char's value ranges had gaps and were generally totally insane. 1114111 seems a little low, too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has gaps. Remember we do first layout-based validation and then type-based validation. This value is so out of range it is already caught by the first.

But if that's too low, our layout code would be wrong and that would be BAD (TM).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maximal value currently seems to be 0x10ffff. In decimal that's 1114111.

Maybe we should make validation print the range in hexadecimal...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah seems fine. Or we could print both. It's not really clear what is more readable...

'a' => {true},
'b' => {false},
_ => {true},
};
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

fn main() {
assert!(std::char::from_u32(-1_i32 as u32).is_none());
let c = unsafe { std::mem::transmute::<i32, char>(-1) };
Expand Down
11 changes: 1 addition & 10 deletions tests/compile-fail/invalid_enum_discriminant.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0

#[repr(C)]
pub enum Foo {
A, B, C, D
}

fn main() {
let f = unsafe { std::mem::transmute::<i32, Foo>(42) };
match f {
Foo::A => {}, //~ ERROR invalid enum discriminant
Foo::B => {},
Foo::C => {},
Foo::D => {},
}
let _f = unsafe { std::mem::transmute::<i32, Foo>(42) }; //~ ERROR encountered invalid enum discriminant 42
}
2 changes: 1 addition & 1 deletion tests/compile-fail/invalid_enum_discriminant2.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Validation makes this fail in the wrong place
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

// error-pattern: invalid enum discriminant

Expand Down
13 changes: 0 additions & 13 deletions tests/compile-fail/match_char.rs

This file was deleted.

2 changes: 1 addition & 1 deletion tests/compile-fail/never_say_never.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// This should fail even without validation
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

#![feature(never_type)]
#![allow(unreachable_code)]
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/never_transmute_humans.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// This should fail even without validation
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

#![feature(never_type)]
#![allow(unreachable_code)]
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/never_transmute_void.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// This should fail even without validation
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

#![feature(never_type)]
#![allow(unreachable_code)]
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-fail/reference_to_packed.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// This should fail even without validation
// compile-flags: -Zmir-emit-validate=0
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

#![allow(dead_code, unused_variables)]

Expand Down
3 changes: 3 additions & 0 deletions tests/compile-fail/storage_dead_dangling.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// This should fail even without validation
// compile-flags: -Zmir-emit-validate=0 -Zmiri-disable-validation

static mut LEAK: usize = 0;

fn fill(v: &mut i32) {
Expand Down
10 changes: 10 additions & 0 deletions tests/compile-fail/validation_cast_fn_ptr1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
fn main() {
// Cast a function pointer such that on a call, the argument gets transmuted
// from raw ptr to reference. This is ABI-compatible, so it's not the call that
// should fail, but validation should.
fn f(_x: &i32) { }

let g: fn(*const i32) = unsafe { std::mem::transmute(f as fn(&i32)) };

g(0usize as *const i32) //~ ERROR encountered 0, but expected something greater or equal to 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet

}
10 changes: 10 additions & 0 deletions tests/compile-fail/validation_cast_fn_ptr2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
fn main() {
// Cast a function pointer such that when returning, the return value gets transmuted
// from raw ptr to reference. This is ABI-compatible, so it's not the call that
// should fail, but validation should.
fn f() -> *const i32 { 0usize as *const i32 }

let g: fn() -> &'static i32 = unsafe { std::mem::transmute(f as fn() -> *const i32) };

let _x = g(); //~ ERROR encountered 0, but expected something greater or equal to 1
}
3 changes: 0 additions & 3 deletions tests/compiletest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ fn compile_fail(sysroot: &Path, path: &str, target: &str, host: &str, need_fullm
flags.push("-Dwarnings -Dunused".to_owned()); // overwrite the -Aunused in compiletest-rs
config.src_base = PathBuf::from(path.to_string());
flags.push("-Zmir-emit-validate=1".to_owned());
flags.push("-Zmiri-disable-validation".to_owned());
config.target_rustcflags = Some(flags.join(" "));
config.target = target.to_owned();
config.host = host.to_owned();
Expand Down Expand Up @@ -103,8 +102,6 @@ fn miri_pass(sysroot: &Path, path: &str, target: &str, host: &str, need_fullmir:
flags.push("-Dwarnings -Dunused".to_owned()); // overwrite the -Aunused in compiletest-rs
if have_fullmir() {
flags.push("-Zmiri-start-fn".to_owned());
// start-fn uses ptr::read, and so fails validation
flags.push("-Zmiri-disable-validation".to_owned());
}
if opt {
flags.push("-Zmir-opt-level=3".to_owned());
Expand Down
3 changes: 0 additions & 3 deletions tests/run-pass/call_drop_through_owned_slice.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
// FIXME validation disabled because ptr::read uses mem::uninitialized
// compile-flags: -Zmiri-disable-validation

struct Bar;

static mut DROP_COUNT: usize = 0;
Expand Down
3 changes: 0 additions & 3 deletions tests/run-pass/issue-29746.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// FIXME validation disabled because ptr::read uses mem::uninitialized
// compile-flags: -Zmiri-disable-validation

// zip!(a1,a2,a3,a4) is equivalent to:
// a1.zip(a2).zip(a3).zip(a4).map(|(((x1,x2),x3),x4)| (x1,x2,x3,x4))
macro_rules! zip {
Expand Down
3 changes: 0 additions & 3 deletions tests/run-pass/sendable-class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// FIXME validation disabled because ptr::read uses mem::uninitialized
// compile-flags: -Zmiri-disable-validation

// Test that a class with only sendable fields can be sent

use std::sync::mpsc::channel;
Expand Down
3 changes: 0 additions & 3 deletions tests/run-pass/unique-send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// FIXME validation disabled because ptr::read uses mem::uninitialized
// compile-flags: -Zmiri-disable-validation

#![feature(box_syntax)]

use std::sync::mpsc::channel;
Expand Down