From 91f0b28ecc780f291ed716c9126719b0f3110f92 Mon Sep 17 00:00:00 2001 From: Wim Looman Date: Fri, 30 Nov 2018 17:05:37 +0100 Subject: [PATCH 1/7] Skip testing targets that don't ship libstd --- tests/compiletest.rs | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/tests/compiletest.rs b/tests/compiletest.rs index 7aa55ef6634..859890aba9f 100644 --- a/tests/compiletest.rs +++ b/tests/compiletest.rs @@ -124,16 +124,33 @@ fn is_target_dir>(path: P) -> bool { path.metadata().map(|m| m.is_dir()).unwrap_or(false) } -fn for_all_targets(sysroot: &Path, mut f: F) { +fn target_has_std>(path: P) -> bool { + let mut path = path.into(); + path.push("lib"); + std::fs::read_dir(path) + .expect("invalid target") + .map(|entry| entry.unwrap()) + .filter(|entry| entry.file_type().unwrap().is_file()) + .filter_map(|entry| entry.file_name().into_string().ok()) + .any(|file_name| file_name.starts_with("libstd") && file_name.ends_with(".rlib")) +} + + +fn for_all_targets(sysroot: &Path, f: F) { let target_dir = sysroot.join("lib").join("rustlib"); - for entry in std::fs::read_dir(target_dir).expect("invalid sysroot") { - let entry = entry.unwrap(); - if !is_target_dir(entry.path()) { - continue; - } - let target = entry.file_name().into_string().unwrap(); - f(target); + let mut targets = std::fs::read_dir(target_dir) + .expect("invalid sysroot") + .map(|entry| entry.unwrap()) + .filter(|entry| is_target_dir(entry.path())) + .filter(|entry| target_has_std(entry.path())) + .map(|entry| entry.file_name().into_string().unwrap()) + .peekable(); + + if targets.peek().is_none() { + panic!("No valid targets found"); } + + targets.for_each(f); } fn get_sysroot() -> PathBuf { From e12d4bc70c018e366e0ac896f60b456019e63986 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 2 Dec 2018 14:03:29 +0100 Subject: [PATCH 2/7] build libstd with minimal features --- src/bin/cargo-miri.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/bin/cargo-miri.rs b/src/bin/cargo-miri.rs index 4c83c5bd3dc..2dc6eee5de9 100644 --- a/src/bin/cargo-miri.rs +++ b/src/bin/cargo-miri.rs @@ -155,6 +155,9 @@ fn setup(ask_user: bool) { File::create(dir.join("Xargo.toml")).unwrap() .write_all(br#" [dependencies.std] +default_features = false +# We need the `panic_unwind` feature because we use the `unwind` panic strategy. +# Using `abort` works for libstd, but then libtest will not compile. features = ["panic_unwind"] [dependencies.test] From 6df89de68a21f538173fa833f5965ac4d5da7f23 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 2 Dec 2018 17:18:25 +0100 Subject: [PATCH 3/7] we don't need no whitelist --- src/lib.rs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 9739a7a95b6..ec4e621a24a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -310,26 +310,9 @@ impl<'a, 'mir, 'tcx> Machine<'a, 'mir, 'tcx> for Evaluator<'tcx> { const STATIC_KIND: Option = Some(MiriMemoryKind::MutStatic); + #[inline(always)] fn enforce_validity(ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool { - 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, &str)] = &[ - // Uses mem::uninitialized - ("std::sys::windows::mutex::Mutex::", ""), - ]; - for frame in ecx.stack().iter() - .rev().take(3) - { - let name = frame.instance.to_string(); - if WHITELIST.iter().any(|(prefix, suffix)| name.starts_with(prefix) && name.ends_with(suffix)) { - return false; - } - } - true + ecx.machine.validate } /// Returns Ok() when the function was handled, fail otherwise From 0e44876a2d37dda4dc57a6eb20588654e5e92562 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 22 Nov 2018 16:26:06 +0100 Subject: [PATCH 4/7] fix mutability gap: do not allow shared mutation when creating frozen reference --- src/stacked_borrows.rs | 37 +++++++++++-------- .../stacked_borrows/illegal_write3.rs | 4 +- .../stacked_borrows/pass_invalid_shr.rs | 2 +- .../stacked_borrows/shr_frozen_violation1.rs | 16 ++++++++ tests/run-pass/refcell.rs | 23 ++++++++++++ tests/run-pass/stacked-borrows.rs | 31 +++++++++------- 6 files changed, 82 insertions(+), 31 deletions(-) create mode 100644 tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 31f80fe2f6c..a90be317705 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -303,14 +303,30 @@ impl<'tcx> Stack { /// is met: We cannot push `Uniq` onto frozen stacks. /// `kind` indicates which kind of reference is being created. fn create(&mut self, bor: Borrow, kind: RefKind) { - if self.frozen_since.is_some() { - // A frozen location? Possible if we create a barrier, then push again. - assert!(bor.is_shared(), "We should never try creating a unique borrow for a frozen stack"); - trace!("create: Not doing anything on frozen location"); + // When creating a frozen reference, freeze. This ensures F1. + // We also do *not* push anything else to the stack, making sure that no nother kind + // of access (like writing through raw pointers) is permitted. + if kind == RefKind::Frozen { + let bor_t = match bor { + Borrow::Shr(Some(t)) => t, + _ => bug!("Creating illegal borrow {:?} for frozen ref", bor), + }; + // It is possible that we already are frozen (e.g. if we just pushed a barrier, + // the redundancy check would not have kicked in). + match self.frozen_since { + Some(loc_t) => assert!(loc_t <= bor_t, "Trying to freeze location for longer than it was already frozen"), + None => { + trace!("create: Freezing"); + self.frozen_since = Some(bor_t); + } + } return; } - // First, push. We do this even if we will later freeze, because we - // will allow mutation of shared data at the expense of unfreezing. + if self.frozen_since.is_some() { + bug!("Trying to create non-frozen reference to frozen location"); + } + + // Push new item to the stack. let itm = match bor { Borrow::Uniq(t) => BorStackItem::Uniq(t), Borrow::Shr(_) => BorStackItem::Shr, @@ -325,15 +341,6 @@ impl<'tcx> Stack { trace!("create: Pushing {:?}", itm); self.borrows.push(itm); } - // Then, maybe freeze. This is part 2 of ensuring F1. - if kind == RefKind::Frozen { - let bor_t = match bor { - Borrow::Shr(Some(t)) => t, - _ => bug!("Creating illegal borrow {:?} for frozen ref", bor), - }; - trace!("create: Freezing"); - self.frozen_since = Some(bor_t); - } } /// Add a barrier diff --git a/tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs b/tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs index 01559af21e7..a653aa5003f 100644 --- a/tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs +++ b/tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs @@ -3,6 +3,6 @@ fn main() { // Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref. let r#ref = ⌖ // freeze let ptr = r#ref as *const _ as *mut _; // raw ptr, with raw tag - unsafe { *ptr = 42; } - let _val = *r#ref; //~ ERROR is not frozen + unsafe { *ptr = 42; } //~ ERROR does not exist on the stack + let _val = *r#ref; } diff --git a/tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs b/tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs index 67bbc88e40f..22a80e27103 100644 --- a/tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs +++ b/tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs @@ -3,7 +3,7 @@ fn foo(_: &i32) {} fn main() { let x = &mut 42; - let xraw = &*x as *const _ as *mut _; + let xraw = x as *mut _; let xref = unsafe { &*xraw }; unsafe { *xraw = 42 }; // unfreeze foo(xref); //~ ERROR is not frozen diff --git a/tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs b/tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs new file mode 100644 index 00000000000..560c9dfb665 --- /dev/null +++ b/tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs @@ -0,0 +1,16 @@ +fn foo(x: &mut i32) -> i32 { + *x = 5; + unknown_code(&*x); + *x // must return 5 +} + +fn main() { + println!("{}", foo(&mut 0)); +} + +// If we replace the `*const` by `&`, my current dev version of miri +// *does* find the problem, but not for a good reason: It finds it because +// of barriers, and we shouldn't rely on unknown code using barriers. +fn unknown_code(x: *const i32) { + unsafe { *(x as *mut i32) = 7; } //~ ERROR barrier +} diff --git a/tests/run-pass/refcell.rs b/tests/run-pass/refcell.rs index 5f2f3523b96..0bc8b15c5f2 100644 --- a/tests/run-pass/refcell.rs +++ b/tests/run-pass/refcell.rs @@ -39,6 +39,13 @@ fn aliasing_mut_and_shr() { *aliasing += 4; let _shr = &*rc; *aliasing += 4; + // also turning this into a frozen ref now must work + let aliasing = &*aliasing; + let _val = *aliasing; + let _escape_to_raw = rc as *const _; // this must NOT unfreeze + let _val = *aliasing; + let _shr = &*rc; // this must NOT unfreeze + let _val = *aliasing; } let rc = RefCell::new(23); @@ -48,7 +55,23 @@ fn aliasing_mut_and_shr() { assert_eq!(*rc.borrow(), 23+12); } +fn aliasing_frz_and_shr() { + fn inner(rc: &RefCell, aliasing: &i32) { + let _val = *aliasing; + let _escape_to_raw = rc as *const _; // this must NOT unfreeze + let _val = *aliasing; + let _shr = &*rc; // this must NOT unfreeze + let _val = *aliasing; + } + + let rc = RefCell::new(23); + let bshr = rc.borrow(); + inner(&rc, &*bshr); + assert_eq!(*rc.borrow(), 23); +} + fn main() { lots_of_funny_borrows(); aliasing_mut_and_shr(); + aliasing_frz_and_shr(); } diff --git a/tests/run-pass/stacked-borrows.rs b/tests/run-pass/stacked-borrows.rs index 93bdf5ffbf3..388765c29ea 100644 --- a/tests/run-pass/stacked-borrows.rs +++ b/tests/run-pass/stacked-borrows.rs @@ -4,10 +4,11 @@ fn main() { read_does_not_invalidate1(); read_does_not_invalidate2(); ref_raw_int_raw(); - mut_shr_raw(); mut_raw_then_mut_shr(); + mut_shr_then_mut_raw(); mut_raw_mut(); partially_invalidate_mut(); + drop_after_sharing(); } // Deref a raw ptr to access a field of a large struct, where the field @@ -53,18 +54,6 @@ fn ref_raw_int_raw() { assert_eq!(unsafe { *xraw }, 3); } -// Creating a raw from a `&mut` through an `&` works, even if we -// write through that raw. -fn mut_shr_raw() { - let mut x = 2; - { - let xref = &mut x; - let xraw = &*xref as *const i32 as *mut i32; - unsafe { *xraw = 4; } - } - assert_eq!(x, 4); -} - // Escape a mut to raw, then share the same mut and use the share, then the raw. // That should work. fn mut_raw_then_mut_shr() { @@ -77,6 +66,16 @@ fn mut_raw_then_mut_shr() { assert_eq!(x, 4); } +// Create first a shared reference and then a raw pointer from a `&mut` +// should permit mutation through that raw pointer. +fn mut_shr_then_mut_raw() { + let xref = &mut 2; + let _xshr = &*xref; + let xraw = xref as *mut _; + unsafe { *xraw = 3; } + assert_eq!(*xref, 3); +} + // Ensure that if we derive from a mut a raw, and then from that a mut, // and then read through the original mut, that does not invalidate the raw. // This shows that the read-exception for `&mut` applies even if the `Shr` item @@ -107,3 +106,9 @@ fn partially_invalidate_mut() { *shard += 1; // so we can still use `shard`. assert_eq!(*data, (1, 1)); } + +// Make sure that we can handle the situation where a loaction is frozen when being dropped. +fn drop_after_sharing() { + let x = String::from("hello!"); + let _len = x.len(); +} From 9d0c1dd676ee542af15bff3c4cd34c5c8cacaad0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 2 Dec 2018 23:24:08 +0100 Subject: [PATCH 5/7] disable VecDeque test until the fix lands in rustc --- tests/run-pass-fullmir/vecdeque.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/run-pass-fullmir/vecdeque.rs b/tests/run-pass-fullmir/vecdeque.rs index 381169505ec..d92cff0b084 100644 --- a/tests/run-pass-fullmir/vecdeque.rs +++ b/tests/run-pass-fullmir/vecdeque.rs @@ -1,3 +1,6 @@ +// FIXME: Validation disabled until https://github.com/rust-lang/rust/pull/56161 lands +// compile-flags: -Zmiri-disable-validation + use std::collections::VecDeque; fn main() { From 04057432c1045564bf8b917cacde931df4894d41 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 3 Dec 2018 09:15:00 +0100 Subject: [PATCH 6/7] bump nightly --- rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-version b/rust-version index 6fd720c5c78..087b7c66427 100644 --- a/rust-version +++ b/rust-version @@ -1 +1 @@ -nightly-2018-11-30 +nightly-2018-12-03 From d11a6766ad388c97e3a95b26a9350ff4ddefd004 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 3 Dec 2018 10:26:39 +0100 Subject: [PATCH 7/7] use assert --- src/stacked_borrows.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index a90be317705..41b5cc2a239 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -322,9 +322,7 @@ impl<'tcx> Stack { } return; } - if self.frozen_since.is_some() { - bug!("Trying to create non-frozen reference to frozen location"); - } + assert!(self.frozen_since.is_none(), "Trying to create non-frozen reference to frozen location"); // Push new item to the stack. let itm = match bor {