From e574c77aa2292ac2a776754a60b1320058bdb071 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 19 Oct 2019 12:28:39 +0200 Subject: [PATCH 01/13] audit our bounds checks --- src/eval.rs | 2 +- src/helpers.rs | 1 + src/shims/foreign_items.rs | 19 ++++++------------- src/shims/intrinsics.rs | 2 ++ 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index bb0bad0ea03d..21a7dd35212c 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -162,7 +162,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( MiriMemoryKind::Env.into(), ); ecx.machine.cmd_line = Some(cmd_ptr); - // Store the UTF-16 string. + // Store the UTF-16 string. We just allocated so we know the bounds are fine. let char_size = Size::from_bytes(2); let cmd_alloc = ecx.memory.get_mut(cmd_ptr.alloc_id)?; let mut cur_ptr = cmd_ptr; diff --git a/src/helpers.rs b/src/helpers.rs index c09d5c823e1b..0c22a0f2e065 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -94,6 +94,7 @@ fn gen_random( } let this = self.eval_context_mut(); + // Don't forget the bounds check. let ptr = this.memory.check_ptr_access( ptr, Size::from_bytes(len as u64), diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index cfbb02f60813..cfd3743089e7 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -50,7 +50,7 @@ fn malloc(&mut self, size: u64, zero_init: bool, kind: MiriMemoryKind) -> Scalar .memory .allocate(Size::from_bytes(size), align, kind.into()); if zero_init { - // We just allocated this, the access cannot fail + // We just allocated this, the access is definitely in-bounds. this.memory .get_mut(ptr.alloc_id) .unwrap() @@ -227,7 +227,7 @@ fn emulate_foreign_item( Align::from_bytes(align).unwrap(), MiriMemoryKind::Rust.into(), ); - // We just allocated this, the access cannot fail + // We just allocated this, the access is definitely in-bounds. this.memory .get_mut(ptr.alloc_id) .unwrap() @@ -643,7 +643,7 @@ fn emulate_foreign_item( // Hook pthread calls that go to the thread-local storage memory subsystem. "pthread_key_create" => { - let key_ptr = this.read_scalar(args[0])?.not_undef()?; + let key_place = this.deref_operand(args[0])?; // Extract the function type out of the signature (that seems easier than constructing it ourselves). let dtor = match this.test_null(this.read_scalar(args[1])?.not_undef()?)? { @@ -668,16 +668,7 @@ fn emulate_foreign_item( throw_unsup!(OutOfTls); } - let key_ptr = this - .memory - .check_ptr_access(key_ptr, key_layout.size, key_layout.align.abi)? - .expect("cannot be a ZST"); - this.memory.get_mut(key_ptr.alloc_id)?.write_scalar( - tcx, - key_ptr, - Scalar::from_uint(key, key_layout.size).into(), - key_layout.size, - )?; + this.write_scalar(Scalar::from_uint(key, key_layout.size), key_place.into())?; // Return success (`0`). this.write_null(dest)?; @@ -856,6 +847,7 @@ fn emulate_foreign_item( let system_info_ptr = this .check_mplace_access(system_info, None)? .expect("cannot be a ZST"); + // We rely on `deref_operand` doing bounds checks for us. // Initialize with `0`. this.memory .get_mut(system_info_ptr.alloc_id)? @@ -992,6 +984,7 @@ fn eval_path_scalar( fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let errno_ptr = this.machine.last_error.unwrap(); + // We allocated this during machine initialziation so the bounds are fine. this.memory.get_mut(errno_ptr.alloc_id)?.write_scalar( &*this.tcx, errno_ptr, diff --git a/src/shims/intrinsics.rs b/src/shims/intrinsics.rs index 4666557e200c..cd7db0973666 100644 --- a/src/shims/intrinsics.rs +++ b/src/shims/intrinsics.rs @@ -359,6 +359,7 @@ fn call_intrinsic( assert!(mplace.meta.is_none()); // not a zst, must be valid pointer let ptr = mplace.ptr.to_ptr()?; + // we know the return place is in-bounds this.memory.get_mut(ptr.alloc_id)?.write_repeat(tcx, ptr, 0, dest.layout.size)?; } } @@ -548,6 +549,7 @@ fn call_intrinsic( let mplace = this.force_allocation(dest)?; assert!(mplace.meta.is_none()); let ptr = mplace.ptr.to_ptr()?; + // We know the return place is in-bounds this.memory .get_mut(ptr.alloc_id)? .mark_definedness(ptr, dest.layout.size, false); From 324fed316f019a24b7a442a7759040fdd650b69f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 19 Oct 2019 16:36:45 +0200 Subject: [PATCH 02/13] print sysroot without any escaping --- miri | 2 +- src/bin/cargo-miri.rs | 12 ++++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/miri b/miri index 6de2391137ac..c3d7ae0280c7 100755 --- a/miri +++ b/miri @@ -54,7 +54,7 @@ build_sysroot() { # Build once, for the user to see. cargo run $CARGO_BUILD_FLAGS --bin cargo-miri -- miri setup "$@" # Call again, to just set env var. - eval $(cargo run $CARGO_BUILD_FLAGS -q --bin cargo-miri -- miri setup --env "$@") + export MIRI_SYSROOT="$(cargo run $CARGO_BUILD_FLAGS -q --bin cargo-miri -- miri setup --print-sysroot "$@")" } # Prepare and set MIRI_SYSROOT. Respects `MIRI_TEST_TARGET` and takes into account diff --git a/src/bin/cargo-miri.rs b/src/bin/cargo-miri.rs index b189dc1f808c..cf513bc1d269 100644 --- a/src/bin/cargo-miri.rs +++ b/src/bin/cargo-miri.rs @@ -310,7 +310,7 @@ fn setup(ask_user: bool) { File::create(dir.join("lib.rs")).unwrap(); // Prepare xargo invocation. let target = get_arg_flag_value("--target"); - let print_env = !ask_user && has_arg_flag("--env"); // whether we just print the necessary environment variable + let print_sysroot = !ask_user && has_arg_flag("--print-sysroot"); // whether we just print the sysroot path let mut command = xargo(); command.arg("build").arg("-q"); command.current_dir(&dir); @@ -339,13 +339,9 @@ fn setup(ask_user: bool) { }; let sysroot = if is_host { dir.join("HOST") } else { PathBuf::from(dir) }; std::env::set_var("MIRI_SYSROOT", &sysroot); // pass the env var to the processes we spawn, which will turn it into "--sysroot" flags - if print_env { - // Escape an arbitrary string for the shell: by wrapping it in `'`, the only special - // character we have to worry about is `'` itself. Everything else is taken literally - // in these strings. `'` is encoded as `'"'"'`: the outer `'` end and being a - // `'`-quoted string, respectively; the `"'"` in the middle represents a single `'`. - // (We could use `'\''` instead of `'"'"'` if we wanted but let's avoid backslashes.) - println!("MIRI_SYSROOT='{}'", sysroot.display().to_string().replace('\'', r#"'"'"'"#)); + if print_sysroot { + // Print just the sysroot and nothing else; this way we do not need any escaping. + println!("{}", sysroot.display()); } else if !ask_user { println!("A libstd for Miri is now available in `{}`.", sysroot.display()); } From 88c88530ec5788ada08bcf23d6c7b149337f0713 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 20 Oct 2019 12:20:48 +0200 Subject: [PATCH 03/13] use expect_none and unwrap_none where it makes sense --- src/eval.rs | 5 +---- src/lib.rs | 1 + src/machine.rs | 5 +---- src/shims/foreign_items.rs | 5 +---- src/shims/fs.rs | 7 ++++--- src/shims/intrinsics.rs | 4 ++-- src/shims/tls.rs | 2 +- src/stacked_borrows.rs | 4 ++-- 8 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 21a7dd35212c..6fb1cd25b159 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -177,10 +177,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( } } - assert!( - args.next().is_none(), - "start lang item has more arguments than expected" - ); + args.next().expect_none("start lang item has more arguments than expected"); // Set the last_error to 0 let errno_layout = ecx.layout_of(ecx.tcx.types.u32)?; diff --git a/src/lib.rs b/src/lib.rs index 06ec33a914bf..59acff358678 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ #![feature(rustc_private)] +#![feature(option_expect_none, option_unwrap_none)] #![warn(rust_2018_idioms)] #![allow(clippy::cast_lossless)] diff --git a/src/machine.rs b/src/machine.rs index d20346ba468a..3878a0860538 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -244,10 +244,7 @@ fn box_alloc( ecx.write_scalar(Scalar::from_uint(align, arg.layout.size), arg)?; // No more arguments. - assert!( - args.next().is_none(), - "`exchange_malloc` lang item has more arguments than expected" - ); + args.next().expect_none("`exchange_malloc` lang item has more arguments than expected"); Ok(()) } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index cfd3743089e7..b28e361b1377 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -349,10 +349,7 @@ fn emulate_foreign_item( let arg_dest = this.local_place(arg_local)?; this.write_scalar(data, arg_dest)?; - assert!( - args.next().is_none(), - "__rust_maybe_catch_panic argument has more arguments than expected" - ); + args.next().expect_none("__rust_maybe_catch_panic argument has more arguments than expected"); // We ourselves will return `0`, eventually (because we will not return if we paniced). this.write_null(dest)?; diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 891474bc3bdb..ed2465cd1f97 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -7,6 +7,7 @@ use crate::stacked_borrows::Tag; use crate::*; +#[derive(Debug)] pub struct FileHandle { file: File, } @@ -103,7 +104,7 @@ fn open( let fd = options.open(path).map(|file| { let mut fh = &mut this.machine.file_handler; fh.low += 1; - fh.handles.insert(fh.low, FileHandle { file }); + fh.handles.insert(fh.low, FileHandle { file }).unwrap_none(); fh.low }); @@ -175,7 +176,7 @@ fn read( .map(|buffer| handle.file.read(buffer)) }); // Reinsert the file handle - this.machine.file_handler.handles.insert(fd, handle); + this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); this.consume_result(bytes?.map(|bytes| bytes as i64)) }) } @@ -204,7 +205,7 @@ fn write( .get_bytes(&*this.tcx, buf, Size::from_bytes(count)) .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) }); - this.machine.file_handler.handles.insert(fd, handle); + this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); this.consume_result(bytes?) }) } diff --git a/src/shims/intrinsics.rs b/src/shims/intrinsics.rs index cd7db0973666..46658760cc12 100644 --- a/src/shims/intrinsics.rs +++ b/src/shims/intrinsics.rs @@ -356,7 +356,7 @@ fn call_intrinsic( _ => { // Do it in memory let mplace = this.force_allocation(dest)?; - assert!(mplace.meta.is_none()); + mplace.meta.unwrap_none(); // not a zst, must be valid pointer let ptr = mplace.ptr.to_ptr()?; // we know the return place is in-bounds @@ -547,7 +547,7 @@ fn call_intrinsic( _ => { // Do it in memory let mplace = this.force_allocation(dest)?; - assert!(mplace.meta.is_none()); + mplace.meta.unwrap_none(); let ptr = mplace.ptr.to_ptr()?; // We know the return place is in-bounds this.memory diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 44bedbd44d2d..b6aadd31a5be 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -53,7 +53,7 @@ pub fn create_tls_key( data: None, dtor, }, - ); + ).unwrap_none(); trace!("New TLS key allocated: {} with dtor {:?}", new_key, dtor); new_key } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index d219c1c75834..2188b9d5394a 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -172,7 +172,7 @@ fn new_ptr(&mut self) -> PtrId { pub fn new_call(&mut self) -> CallId { let id = self.next_call_id; trace!("new_call: Assigning ID {}", id); - self.active_calls.insert(id); + assert!(self.active_calls.insert(id)); self.next_call_id = NonZeroU64::new(id.get() + 1).unwrap(); id } @@ -189,7 +189,7 @@ pub fn static_base_ptr(&mut self, id: AllocId) -> Tag { self.base_ptr_ids.get(&id).copied().unwrap_or_else(|| { let tag = Tag::Tagged(self.new_ptr()); trace!("New allocation {:?} has base tag {:?}", id, tag); - self.base_ptr_ids.insert(id, tag); + self.base_ptr_ids.insert(id, tag).unwrap_none(); tag }) } From 4232939319448244bacd8376049a17325225eadb Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 12 Oct 2019 20:44:45 -0500 Subject: [PATCH 04/13] Move last error functions to helpers --- src/helpers.rs | 35 +++++++++++++++++++++++++++++++++++ src/shims/env.rs | 4 ++-- src/shims/foreign_items.rs | 28 ---------------------------- src/shims/fs.rs | 2 +- 4 files changed, 38 insertions(+), 31 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 0c22a0f2e065..36091d923555 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -345,4 +345,39 @@ fn check_no_isolation(&mut self, name: &str) -> InterpResult<'tcx> { } Ok(()) } + + /// Sets the last error variable + fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let tcx = &{ this.tcx.tcx }; + let errno_ptr = this.machine.last_error.unwrap(); + this.memory.get_mut(errno_ptr.alloc_id)?.write_scalar( + tcx, + errno_ptr, + scalar.into(), + Size::from_bits(32), + ) + } + + /// Gets the last error variable + fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + let tcx = &{ this.tcx.tcx }; + let errno_ptr = this.machine.last_error.unwrap(); + this.memory + .get(errno_ptr.alloc_id)? + .read_scalar(tcx, errno_ptr, Size::from_bits(32))? + .not_undef() + } + + /// Sets the last error variable using a `std::io::Error`. It fails if the error cannot be + /// transformed to a raw os error succesfully + fn set_last_error_from_io_error(&mut self, e: std::io::Error) -> InterpResult<'tcx> { + self.eval_context_mut().set_last_error(Scalar::from_int( + e.raw_os_error().ok_or_else(|| { + err_unsup_format!("The {} error cannot be transformed into a raw os error", e) + })?, + Size::from_bits(32), + )) + } } diff --git a/src/shims/env.rs b/src/shims/env.rs index 6078ca26e269..661e8bf209b1 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -146,7 +146,7 @@ fn getcwd( let erange = this.eval_libc("ERANGE")?; this.set_last_error(erange)?; } - Err(e) => this.consume_io_error(e)?, + Err(e) => this.set_last_error_from_io_error(e)?, } Ok(Scalar::ptr_null(&*this.tcx)) } @@ -168,7 +168,7 @@ fn chdir(&mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { match env::set_current_dir(path) { Ok(()) => Ok(0), Err(e) => { - this.consume_io_error(e)?; + this.set_last_error_from_io_error(e)?; Ok(-1) } } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index b28e361b1377..5d2c3648b43a 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -977,34 +977,6 @@ fn eval_path_scalar( } return Ok(None); } - - fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - let errno_ptr = this.machine.last_error.unwrap(); - // We allocated this during machine initialziation so the bounds are fine. - this.memory.get_mut(errno_ptr.alloc_id)?.write_scalar( - &*this.tcx, - errno_ptr, - scalar.into(), - Size::from_bits(32), - ) - } - - fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { - let this = self.eval_context_mut(); - let errno_ptr = this.machine.last_error.unwrap(); - this.memory - .get(errno_ptr.alloc_id)? - .read_scalar(&*this.tcx, errno_ptr, Size::from_bits(32))? - .not_undef() - } - - fn consume_io_error(&mut self, e: std::io::Error) -> InterpResult<'tcx> { - self.eval_context_mut().set_last_error(Scalar::from_int( - e.raw_os_error().unwrap(), - Size::from_bits(32), - )) - } } // Shims the linux 'getrandom()' syscall. diff --git a/src/shims/fs.rs b/src/shims/fs.rs index ed2465cd1f97..8fcd5c8b1e3e 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -285,7 +285,7 @@ fn consume_result>( match result { Ok(ok) => Ok(ok), Err(e) => { - self.eval_context_mut().consume_io_error(e)?; + self.eval_context_mut().set_last_error_from_io_error(e)?; Ok((-1).into()) } } From ed776f67ba73380926d987bb8ba6618fa0cc55f3 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 12 Oct 2019 20:58:02 -0500 Subject: [PATCH 05/13] Change last_error to a place --- src/eval.rs | 3 +-- src/helpers.rs | 18 ++++-------------- src/machine.rs | 4 ++-- src/shims/foreign_items.rs | 3 ++- 4 files changed, 9 insertions(+), 19 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 6fb1cd25b159..f4a8d176172d 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -183,8 +183,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>( let errno_layout = ecx.layout_of(ecx.tcx.types.u32)?; let errno_place = ecx.allocate(errno_layout, MiriMemoryKind::Static.into()); ecx.write_scalar(Scalar::from_u32(0), errno_place.into())?; - let errno_ptr = ecx.check_mplace_access(errno_place.into(), Some(Size::from_bits(32)))?; - ecx.machine.last_error = errno_ptr; + ecx.machine.last_error = Some(errno_place); Ok(ecx) } diff --git a/src/helpers.rs b/src/helpers.rs index 36091d923555..1b80166b2fe7 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -349,25 +349,15 @@ fn check_no_isolation(&mut self, name: &str) -> InterpResult<'tcx> { /// Sets the last error variable fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let tcx = &{ this.tcx.tcx }; - let errno_ptr = this.machine.last_error.unwrap(); - this.memory.get_mut(errno_ptr.alloc_id)?.write_scalar( - tcx, - errno_ptr, - scalar.into(), - Size::from_bits(32), - ) + let errno_place = this.machine.last_error.unwrap(); + this.write_scalar(scalar, errno_place.into()) } /// Gets the last error variable fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let tcx = &{ this.tcx.tcx }; - let errno_ptr = this.machine.last_error.unwrap(); - this.memory - .get(errno_ptr.alloc_id)? - .read_scalar(tcx, errno_ptr, Size::from_bits(32))? - .not_undef() + let errno_place = this.machine.last_error.unwrap(); + this.read_scalar(errno_place.into())?.not_undef() } /// Sets the last error variable using a `std::io::Error`. It fails if the error cannot be diff --git a/src/machine.rs b/src/machine.rs index 3878a0860538..3714aa2d799e 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -91,8 +91,8 @@ pub struct Evaluator<'tcx> { pub(crate) argv: Option>, pub(crate) cmd_line: Option>, - /// Last OS error. - pub(crate) last_error: Option>, + /// Last OS error location in memory. It is a 32 bits integer (unsigned for Windows) + pub(crate) last_error: Option>, /// TLS state. pub(crate) tls: TlsData<'tcx>, diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 5d2c3648b43a..51be7ea5bcd3 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -414,7 +414,8 @@ fn emulate_foreign_item( } "__errno_location" | "__error" => { - let errno_scalar: Scalar = this.machine.last_error.unwrap().into(); + let errno_place = this.machine.last_error.unwrap(); + let errno_scalar: Scalar = this.check_mplace_access(errno_place.into(), Some(Size::from_bits(32)))?.unwrap().into(); this.write_scalar(errno_scalar, dest)?; } From 338e51aa48fbf9d5f3407c3872e46facfab53ca3 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 16 Oct 2019 21:37:35 -0500 Subject: [PATCH 06/13] Rename consume_result --- src/helpers.rs | 19 +++++++++++++++++++ src/shims/fs.rs | 29 +++++------------------------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 1b80166b2fe7..465fca554c10 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -370,4 +370,23 @@ fn set_last_error_from_io_error(&mut self, e: std::io::Error) -> InterpResult<'t Size::from_bits(32), )) } + + /// Helper function that consumes an `std::io::Result` and returns an + /// `InterpResult<'tcx,T>::Ok` instead. It is expected that the result can be converted to an + /// OS error using `std::io::Error::raw_os_error`. + /// + /// This function uses `T: From` instead of `i32` directly because some IO related + /// functions return different integer types (like `read`, that returns an `i64`) + fn set_last_error_from_io_result>( + &mut self, + result: std::io::Result, + ) -> InterpResult<'tcx, T> { + match result { + Ok(ok) => Ok(ok), + Err(e) => { + self.eval_context_mut().set_last_error_from_io_error(e)?; + Ok((-1).into()) + } + } + } } diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 8fcd5c8b1e3e..c8d1eb29562d 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -108,7 +108,7 @@ fn open( fh.low }); - this.consume_result(fd) + this.set_last_error_from_io_result(fd) } fn fcntl( @@ -144,7 +144,7 @@ fn close(&mut self, fd_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let fd = this.read_scalar(fd_op)?.to_i32()?; this.remove_handle_and(fd, |handle, this| { - this.consume_result(handle.file.sync_all().map(|_| 0i32)) + this.set_last_error_from_io_result(handle.file.sync_all().map(|_| 0i32)) }) } @@ -177,7 +177,7 @@ fn read( }); // Reinsert the file handle this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.consume_result(bytes?.map(|bytes| bytes as i64)) + this.set_last_error_from_io_result(bytes?.map(|bytes| bytes as i64)) }) } @@ -206,7 +206,7 @@ fn write( .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) }); this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.consume_result(bytes?) + this.set_last_error_from_io_result(bytes?) }) } @@ -223,7 +223,7 @@ fn unlink( &mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let result = remove_file(path).map(|_| 0); - this.consume_result(result) + this.set_last_error_from_io_result(result) } /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it @@ -271,23 +271,4 @@ fn remove_handle_and>(&mut self, fd: i32, mut f: F) -> InterpRes Ok((-1).into()) } } - - /// Helper function that consumes an `std::io::Result` and returns an - /// `InterpResult<'tcx,T>::Ok` instead. It is expected that the result can be converted to an - /// OS error using `std::io::Error::raw_os_error`. - /// - /// This function uses `T: From` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`) - fn consume_result>( - &mut self, - result: std::io::Result, - ) -> InterpResult<'tcx, T> { - match result { - Ok(ok) => Ok(ok), - Err(e) => { - self.eval_context_mut().set_last_error_from_io_error(e)?; - Ok((-1).into()) - } - } - } } From 5c3c738c4b82a00471cffe67e44a22173404bd4f Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 17 Oct 2019 20:29:30 -0500 Subject: [PATCH 07/13] Make transformation to OS error explicit --- src/helpers.rs | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 465fca554c10..7d68d05cdb78 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -361,14 +361,34 @@ fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { } /// Sets the last error variable using a `std::io::Error`. It fails if the error cannot be - /// transformed to a raw os error succesfully + /// transformed to a raw os error succesfully. fn set_last_error_from_io_error(&mut self, e: std::io::Error) -> InterpResult<'tcx> { - self.eval_context_mut().set_last_error(Scalar::from_int( - e.raw_os_error().ok_or_else(|| { - err_unsup_format!("The {} error cannot be transformed into a raw os error", e) - })?, - Size::from_bits(32), - )) + use std::io::ErrorKind::*; + let this = self.eval_context_mut(); + let target = &this.tcx.tcx.sess.target.target; + let last_error = if target.options.target_family == Some("unix".to_owned()) { + this.eval_libc(match e.kind() { + ConnectionRefused => "ECONNREFUSED", + ConnectionReset => "ECONNRESET", + PermissionDenied => "EPERM", + BrokenPipe => "EPIPE", + NotConnected => "ENOTCONN", + ConnectionAborted => "ECONNABORTED", + AddrNotAvailable => "EADDRNOTAVAIL", + AddrInUse => "EADDRINUSE", + NotFound => "ENOENT", + Interrupted => "EINTR", + InvalidInput => "EINVAL", + TimedOut => "ETIMEDOUT", + AlreadyExists => "EEXIST", + WouldBlock => "EWOULDBLOCK", + _ => throw_unsup_format!("The {} error cannot be transformed into a raw os error", e) + })? + } else { + // FIXME: we have to implement the windows' equivalent of this. + throw_unsup_format!("Setting the last OS error from an io::Error is unsupported for {}.", target.target_os) + }; + this.set_last_error(last_error) } /// Helper function that consumes an `std::io::Result` and returns an From 619ccf3834d18eeef1aea4030e5c2bcf673e5688 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 18 Oct 2019 14:33:25 -0500 Subject: [PATCH 08/13] Rename set_last_error_from_io_result --- src/helpers.rs | 10 +++++----- src/machine.rs | 2 +- src/shims/fs.rs | 11 +++++------ 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 7d68d05cdb78..616de8378791 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -360,8 +360,8 @@ fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { this.read_scalar(errno_place.into())?.not_undef() } - /// Sets the last error variable using a `std::io::Error`. It fails if the error cannot be - /// transformed to a raw os error succesfully. + /// Sets the last OS error using a `std::io::Error`. This function tries to produce the most + /// similar OS error from the `std::io::ErrorKind` and sets it as the last OS error. fn set_last_error_from_io_error(&mut self, e: std::io::Error) -> InterpResult<'tcx> { use std::io::ErrorKind::*; let this = self.eval_context_mut(); @@ -392,12 +392,12 @@ fn set_last_error_from_io_error(&mut self, e: std::io::Error) -> InterpResult<'t } /// Helper function that consumes an `std::io::Result` and returns an - /// `InterpResult<'tcx,T>::Ok` instead. It is expected that the result can be converted to an - /// OS error using `std::io::Error::raw_os_error`. + /// `InterpResult<'tcx,T>::Ok` instead. In case the result is an error, this function returns + /// `Ok(-1)` and sets the last OS error accordingly. /// /// This function uses `T: From` instead of `i32` directly because some IO related /// functions return different integer types (like `read`, that returns an `i64`) - fn set_last_error_from_io_result>( + fn try_unwrap_io_result>( &mut self, result: std::io::Result, ) -> InterpResult<'tcx, T> { diff --git a/src/machine.rs b/src/machine.rs index 3714aa2d799e..315e9c1a35a9 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -91,7 +91,7 @@ pub struct Evaluator<'tcx> { pub(crate) argv: Option>, pub(crate) cmd_line: Option>, - /// Last OS error location in memory. It is a 32 bits integer (unsigned for Windows) + /// Last OS error location in memory. It is a 32 bit integer (unsigned for Windows) pub(crate) last_error: Option>, /// TLS state. diff --git a/src/shims/fs.rs b/src/shims/fs.rs index c8d1eb29562d..ffcfab10081a 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -108,7 +108,7 @@ fn open( fh.low }); - this.set_last_error_from_io_result(fd) + this.try_unwrap_io_result(fd) } fn fcntl( @@ -144,7 +144,7 @@ fn close(&mut self, fd_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let fd = this.read_scalar(fd_op)?.to_i32()?; this.remove_handle_and(fd, |handle, this| { - this.set_last_error_from_io_result(handle.file.sync_all().map(|_| 0i32)) + this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32)) }) } @@ -175,9 +175,8 @@ fn read( .get_bytes_mut(&*this.tcx, buf, Size::from_bytes(count)) .map(|buffer| handle.file.read(buffer)) }); - // Reinsert the file handle this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.set_last_error_from_io_result(bytes?.map(|bytes| bytes as i64)) + this.try_unwrap_io_result(bytes?.map(|bytes| bytes as i64)) }) } @@ -206,7 +205,7 @@ fn write( .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) }); this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.set_last_error_from_io_result(bytes?) + this.try_unwrap_io_result(bytes?) }) } @@ -223,7 +222,7 @@ fn unlink( &mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let result = remove_file(path).map(|_| 0); - this.set_last_error_from_io_result(result) + this.try_unwrap_io_result(result) } /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it From 8a8fa53a5d28fa3e849e26e907480dd28b8f2aa3 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 18 Oct 2019 14:44:48 -0500 Subject: [PATCH 09/13] Transform the last error place to an immediate instead --- src/shims/foreign_items.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 51be7ea5bcd3..1933aee1151d 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -415,8 +415,7 @@ fn emulate_foreign_item( "__errno_location" | "__error" => { let errno_place = this.machine.last_error.unwrap(); - let errno_scalar: Scalar = this.check_mplace_access(errno_place.into(), Some(Size::from_bits(32)))?.unwrap().into(); - this.write_scalar(errno_scalar, dest)?; + this.write_scalar(errno_place.to_ref().to_scalar()?, dest)?; } "getenv" => { From 9d50c5e75806bc27fc0b144be92b895c2f2a7339 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 19 Oct 2019 14:00:44 -0500 Subject: [PATCH 10/13] Small corrections to docs --- src/helpers.rs | 4 ++-- src/machine.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 616de8378791..16091bb242cd 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -346,14 +346,14 @@ fn check_no_isolation(&mut self, name: &str) -> InterpResult<'tcx> { Ok(()) } - /// Sets the last error variable + /// Sets the last error variable. fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let errno_place = this.machine.last_error.unwrap(); this.write_scalar(scalar, errno_place.into()) } - /// Gets the last error variable + /// Gets the last error variable. fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); let errno_place = this.machine.last_error.unwrap(); diff --git a/src/machine.rs b/src/machine.rs index 315e9c1a35a9..50f0ecf59093 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -91,7 +91,7 @@ pub struct Evaluator<'tcx> { pub(crate) argv: Option>, pub(crate) cmd_line: Option>, - /// Last OS error location in memory. It is a 32 bit integer (unsigned for Windows) + /// Last OS error location in memory. It is a 32-bit integer pub(crate) last_error: Option>, /// TLS state. From ebdb6d4df73c244b4b6dae761c900db896263a89 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 21 Oct 2019 10:25:47 +0200 Subject: [PATCH 11/13] when xargo is manually specified, don't try to upgrade it --- src/bin/cargo-miri.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bin/cargo-miri.rs b/src/bin/cargo-miri.rs index cf513bc1d269..b889ce52f386 100644 --- a/src/bin/cargo-miri.rs +++ b/src/bin/cargo-miri.rs @@ -259,6 +259,10 @@ fn setup(ask_user: bool) { // First, we need xargo. if xargo_version().map_or(true, |v| v < (0, 3, 16)) { + if std::env::var("XARGO").is_ok() { + // The user manually gave us a xargo binary; don't do anything automatically. + show_error(format!("Your xargo is too old; please upgrade to the latest version")) + } let mut cmd = cargo(); cmd.args(&["install", "xargo", "-f"]); ask_to_run(cmd, ask_user, "install a recent enough xargo"); From d9aa20fb3135436706ba8b9dd387edfde9127ddb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 21 Oct 2019 13:24:56 +0200 Subject: [PATCH 12/13] add some missing trailing full stops that slipped through review --- src/machine.rs | 2 +- src/shims/fs.rs | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/machine.rs b/src/machine.rs index 50f0ecf59093..7904e1cc123b 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -91,7 +91,7 @@ pub struct Evaluator<'tcx> { pub(crate) argv: Option>, pub(crate) cmd_line: Option>, - /// Last OS error location in memory. It is a 32-bit integer + /// Last OS error location in memory. It is a 32-bit integer. pub(crate) last_error: Option>, /// TLS state. diff --git a/src/shims/fs.rs b/src/shims/fs.rs index ffcfab10081a..902f5f609d18 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -21,7 +21,7 @@ impl Default for FileHandler { fn default() -> Self { FileHandler { handles: Default::default(), - // 0, 1 and 2 are reserved for stdin, stdout and stderr + // 0, 1 and 2 are reserved for stdin, stdout and stderr. low: 3, } } @@ -123,7 +123,7 @@ fn fcntl( let fd = this.read_scalar(fd_op)?.to_i32()?; let cmd = this.read_scalar(cmd_op)?.to_i32()?; - // We only support getting the flags for a descriptor + // We only support getting the flags for a descriptor. if cmd == this.eval_libc_i32("F_GETFD")? { // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the // `FD_CLOEXEC` value without checking if the flag is set for the file because `std` @@ -159,16 +159,16 @@ fn read( this.check_no_isolation("read")?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - // Reading zero bytes should not change `buf` + // Reading zero bytes should not change `buf`. if count == 0 { return Ok(0); } let fd = this.read_scalar(fd_op)?.to_i32()?; let buf_scalar = this.read_scalar(buf_op)?.not_undef()?; - // Remove the file handle to avoid borrowing issues + // Remove the file handle to avoid borrowing issues. this.remove_handle_and(fd, |mut handle, this| { - // Don't use `?` to avoid returning before reinserting the handle + // Don't use `?` to avoid returning before reinserting the handle. let bytes = this.force_ptr(buf_scalar).and_then(|buf| { this.memory .get_mut(buf.alloc_id)? @@ -191,7 +191,7 @@ fn write( this.check_no_isolation("write")?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - // Writing zero bytes should not change `buf` + // Writing zero bytes should not change `buf`. if count == 0 { return Ok(0); } @@ -232,7 +232,7 @@ fn unlink( &mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). /// /// This function uses `T: From` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`) + /// functions return different integer types (like `read`, that returns an `i64`). fn get_handle_and>(&mut self, fd: i32, f: F) -> InterpResult<'tcx, T> where F: Fn(&FileHandle) -> InterpResult<'tcx, T>, @@ -256,7 +256,7 @@ fn get_handle_and>(&mut self, fd: i32, f: F) -> InterpResult<'tc /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). /// /// This function uses `T: From` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`) + /// functions return different integer types (like `read`, that returns an `i64`). fn remove_handle_and>(&mut self, fd: i32, mut f: F) -> InterpResult<'tcx, T> where F: FnMut(FileHandle, &mut MiriEvalContext<'mir, 'tcx>) -> InterpResult<'tcx, T>, From 789eeee6a5eed7926d3ced5684b624071d9bbf5d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 21 Oct 2019 13:31:38 +0200 Subject: [PATCH 13/13] bump Rust (no changes needed) --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 0f59c13723c7..8e2cace33850 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -fa0f7d0080d8e7e9eb20aa9cbf8013f96c81287f +7979016aff545f7b41cc517031026020b340989d