Auto merge of #1513 - RalfJung:int-align, r=RalfJung

bors · bors · commit 62502a0da56c · 2020-08-17T15:09:49.000Z
add option to use force_int for alignment check Fixes #1074. Depends on rust-lang/rust#75592.
diff --git a/README.md b/README.md
@@ -165,9 +165,8 @@ up the sysroot.  If you are using `miri` (the Miri driver) directly, see the
 
 Miri adds its own set of `-Z` flags:
 
-* `-Zmiri-disable-alignment-check` disables checking pointer alignment. This is
-  useful to avoid [false positives][alignment-false-positives]. However, setting
-  this flag means Miri could miss bugs in your program.
+* `-Zmiri-disable-alignment-check` disables checking pointer alignment, so you
+  can focus on other failures.
 * `-Zmiri-disable-stacked-borrows` disables checking the experimental
   [Stacked Borrows] aliasing rules.  This can make Miri run faster, but it also
   means no aliasing violations will be detected.
@@ -189,6 +188,18 @@ Miri adds its own set of `-Z` flags:
   entropy.  The default seed is 0.  **NOTE**: This entropy is not good enough
   for cryptographic use!  Do not generate secret keys in Miri or perform other
   kinds of cryptographic operations that rely on proper random numbers.
+* `-Zmiri-symbolic-alignment-check` makes the alignment check more strict.  By
+  default, alignment is checked by casting the pointer to an integer, and making
+  sure that is a multiple of the alignment.  This can lead to cases where a
+  program passes the alignment check by pure chance, because things "happened to
+  be" sufficiently aligned -- there is no UB in this execution but there would
+  be UB in others.  To avoid such cases, the symbolic alignment check only takes
+  into account the requested alignment of the relevant allocation, and the
+  offset into that allocation.  This avoids missing such bugs, but it also
+  incurs some false positives when the code does manual integer arithmetic to
+  ensure alignment.  (The standard library `align_to` method works fine in both
+  modes; under symbolic alignment it only fills the middle slice when the
+  allocation guarantees sufficient alignment.)
 * `-Zmiri-track-alloc-id=<id>` shows a backtrace when the given allocation is
   being allocated or freed.  This helps in debugging memory leaks and
   use after free bugs.
@@ -200,8 +211,6 @@ Miri adds its own set of `-Z` flags:
   assigned to a stack frame.  This helps in debugging UB related to Stacked
   Borrows "protectors".
 
-[alignment-false-positives]: https://github.com/rust-lang/miri/issues/1074
-
 Some native rustc `-Z` flags are also very relevant for Miri:
 
 * `-Zmir-opt-level` controls how many MIR optimizations are performed.  Miri
diff --git a/rust-version b/rust-version
@@ -1 +1 @@
-814bc4fe9364865bfaa94d7825b8eabc11245c7c
+8cdc94e84040ce797fd33d0a7cfda4ec4f2f2421
diff --git a/src/bin/miri.rs b/src/bin/miri.rs
@@ -172,48 +172,41 @@ fn main() {
     init_early_loggers();
 
     // Parse our arguments and split them across `rustc` and `miri`.
-    let mut validate = true;
-    let mut stacked_borrows = true;
-    let mut check_alignment = true;
-    let mut communicate = false;
-    let mut ignore_leaks = false;
-    let mut seed: Option<u64> = None;
-    let mut tracked_pointer_tag: Option<miri::PtrId> = None;
-    let mut tracked_call_id: Option<miri::CallId> = None;
-    let mut tracked_alloc_id: Option<miri::AllocId> = None;
+    let mut miri_config = miri::MiriConfig::default();
     let mut rustc_args = vec![];
-    let mut crate_args = vec![];
     let mut after_dashdash = false;
-    let mut excluded_env_vars = vec![];
     for arg in env::args() {
         if rustc_args.is_empty() {
             // Very first arg: binary name.
             rustc_args.push(arg);
         } else if after_dashdash {
             // Everything that comes after `--` is forwarded to the interpreted crate.
-            crate_args.push(arg);
+            miri_config.args.push(arg);
         } else {
             match arg.as_str() {
                 "-Zmiri-disable-validation" => {
-                    validate = false;
+                    miri_config.validate = false;
                 }
                 "-Zmiri-disable-stacked-borrows" => {
-                    stacked_borrows = false;
+                    miri_config.stacked_borrows = false;
                 }
                 "-Zmiri-disable-alignment-check" => {
-                    check_alignment = false;
+                    miri_config.check_alignment = miri::AlignmentCheck::None;
+                }
+                "-Zmiri-symbolic-alignment-check" => {
+                    miri_config.check_alignment = miri::AlignmentCheck::Symbolic;
                 }
                 "-Zmiri-disable-isolation" => {
-                    communicate = true;
+                    miri_config.communicate = true;
                 }
                 "-Zmiri-ignore-leaks" => {
-                    ignore_leaks = true;
+                    miri_config.ignore_leaks = true;
                 }
                 "--" => {
                     after_dashdash = true;
                 }
                 arg if arg.starts_with("-Zmiri-seed=") => {
-                    if seed.is_some() {
+                    if miri_config.seed.is_some() {
                         panic!("Cannot specify -Zmiri-seed multiple times!");
                     }
                     let seed_raw = hex::decode(arg.strip_prefix("-Zmiri-seed=").unwrap())
@@ -234,10 +227,10 @@ fn main() {
 
                     let mut bytes = [0; 8];
                     bytes[..seed_raw.len()].copy_from_slice(&seed_raw);
-                    seed = Some(u64::from_be_bytes(bytes));
+                    miri_config.seed = Some(u64::from_be_bytes(bytes));
                 }
                 arg if arg.starts_with("-Zmiri-env-exclude=") => {
-                    excluded_env_vars
+                    miri_config.excluded_env_vars
                         .push(arg.strip_prefix("-Zmiri-env-exclude=").unwrap().to_owned());
                 }
                 arg if arg.starts_with("-Zmiri-track-pointer-tag=") => {
@@ -249,7 +242,7 @@ fn main() {
                         ),
                     };
                     if let Some(id) = miri::PtrId::new(id) {
-                        tracked_pointer_tag = Some(id);
+                        miri_config.tracked_pointer_tag = Some(id);
                     } else {
                         panic!("-Zmiri-track-pointer-tag requires a nonzero argument");
                     }
@@ -263,7 +256,7 @@ fn main() {
                         ),
                     };
                     if let Some(id) = miri::CallId::new(id) {
-                        tracked_call_id = Some(id);
+                        miri_config.tracked_call_id = Some(id);
                     } else {
                         panic!("-Zmiri-track-call-id requires a nonzero argument");
                     }
@@ -276,7 +269,7 @@ fn main() {
                             err
                         ),
                     };
-                    tracked_alloc_id = Some(miri::AllocId(id));
+                    miri_config.tracked_alloc_id = Some(miri::AllocId(id));
                 }
                 _ => {
                     // Forward to rustc.
@@ -287,19 +280,6 @@ fn main() {
     }
 
     debug!("rustc arguments: {:?}", rustc_args);
-    debug!("crate arguments: {:?}", crate_args);
-    let miri_config = miri::MiriConfig {
-        validate,
-        stacked_borrows,
-        check_alignment,
-        communicate,
-        ignore_leaks,
-        excluded_env_vars,
-        seed,
-        args: crate_args,
-        tracked_pointer_tag,
-        tracked_call_id,
-        tracked_alloc_id,
-    };
+    debug!("crate arguments: {:?}", miri_config.args);
     run_compiler(rustc_args, &mut MiriCompilerCalls { miri_config })
 }
diff --git a/src/diagnostics.rs b/src/diagnostics.rs
@@ -100,11 +100,12 @@ pub fn report_error<'tcx, 'mir>(
                     panic!("Error should never be raised by Miri: {:?}", e.kind),
                 Unsupported(_) =>
                     vec![format!("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support")],
-                UndefinedBehavior(UndefinedBehaviorInfo::AlignmentCheckFailed { .. }) =>
+                UndefinedBehavior(UndefinedBehaviorInfo::AlignmentCheckFailed { .. })
+                    if ecx.memory.extra.check_alignment == AlignmentCheck::Symbolic
+                =>
                     vec![
                         format!("this usually indicates that your program performed an invalid operation and caused Undefined Behavior"),
-                        format!("but alignment errors can also be false positives, see https://github.com/rust-lang/miri/issues/1074"),
-                        format!("you can disable the alignment check with `-Zmiri-disable-alignment-check`, but that could hide true bugs")
+                        format!("but due to `-Zmiri-symbolic-alignment-check`, alignment errors can also be false positives"),
                     ],
                 UndefinedBehavior(_) =>
                     vec![
diff --git a/src/eval.rs b/src/eval.rs
@@ -13,15 +13,25 @@ use rustc_target::abi::LayoutOf;
 
 use crate::*;
 
+#[derive(Copy, Clone, Debug, PartialEq)]
+pub enum AlignmentCheck {
+    /// Do not check alignment.
+    None,
+    /// Check alignment "symbolically", i.e., using only the requested alignment for an allocation and not its real base address.
+    Symbolic,
+    /// Check alignment on the actual physical integer address.
+    Int,
+}
+
 /// Configuration needed to spawn a Miri instance.
 #[derive(Clone)]
 pub struct MiriConfig {
     /// Determine if validity checking is enabled.
     pub validate: bool,
     /// Determines if Stacked Borrows is enabled.
     pub stacked_borrows: bool,
-    /// Determines if alignment checking is enabled.
-    pub check_alignment: bool,
+    /// Controls alignment checking.
+    pub check_alignment: AlignmentCheck,
     /// Determines if communication with the host environment is enabled.
     pub communicate: bool,
     /// Determines if memory leaks should be ignored.
@@ -45,7 +55,7 @@ impl Default for MiriConfig {
         MiriConfig {
             validate: true,
             stacked_borrows: true,
-            check_alignment: true,
+            check_alignment: AlignmentCheck::Int,
             communicate: false,
             ignore_leaks: false,
             excluded_env_vars: vec![],
diff --git a/src/lib.rs b/src/lib.rs
@@ -55,7 +55,7 @@ pub use crate::diagnostics::{
     register_diagnostic, report_error, EvalContextExt as DiagnosticsEvalContextExt,
     TerminationInfo, NonHaltingDiagnostic,
 };
-pub use crate::eval::{create_ecx, eval_main, MiriConfig};
+pub use crate::eval::{create_ecx, eval_main, AlignmentCheck, MiriConfig};
 pub use crate::helpers::EvalContextExt as HelpersEvalContextExt;
 pub use crate::machine::{
     AllocExtra, Evaluator, FrameData, MemoryExtra, MiriEvalContext, MiriEvalContextExt,
diff --git a/src/machine.rs b/src/machine.rs
@@ -128,7 +128,7 @@ pub struct MemoryExtra {
     tracked_alloc_id: Option<AllocId>,
 
     /// Controls whether alignment of memory accesses is being checked.
-    check_alignment: bool,
+    pub(crate) check_alignment: AlignmentCheck,
 }
 
 impl MemoryExtra {
@@ -138,7 +138,7 @@ impl MemoryExtra {
         tracked_pointer_tag: Option<PtrId>,
         tracked_call_id: Option<CallId>,
         tracked_alloc_id: Option<AllocId>,
-        check_alignment: bool,
+        check_alignment: AlignmentCheck,
     ) -> Self {
         let stacked_borrows = if stacked_borrows {
             Some(Rc::new(RefCell::new(stacked_borrows::GlobalState::new(tracked_pointer_tag, tracked_call_id))))
@@ -336,7 +336,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
 
     #[inline(always)]
     fn enforce_alignment(memory_extra: &MemoryExtra) -> bool {
-        memory_extra.check_alignment
+        memory_extra.check_alignment != AlignmentCheck::None
+    }
+
+    #[inline(always)]
+    fn force_int_for_alignment_check(memory_extra: &Self::MemoryExtra) -> bool {
+        memory_extra.check_alignment == AlignmentCheck::Int
     }
 
     #[inline(always)]
diff --git a/src/shims/mod.rs b/src/shims/mod.rs
@@ -13,8 +13,6 @@ pub mod tls;
 
 // End module management, begin local code
 
-use std::convert::TryFrom;
-
 use log::trace;
 
 use rustc_middle::{mir, ty};
@@ -37,8 +35,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         // There are some more lang items we want to hook that CTFE does not hook (yet).
         if this.tcx.lang_items().align_offset_fn() == Some(instance.def.def_id()) {
             let &[ptr, align] = check_arg_count(args)?;
-            this.align_offset(ptr, align, ret, unwind)?;
-            return Ok(None);
+            if this.align_offset(ptr, align, ret, unwind)? {
+                return Ok(None);
+            }
         }
 
         // Try to see if we can do something about foreign items.
@@ -56,46 +55,48 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         Ok(Some(&*this.load_mir(instance.def, None)?))
     }
 
+    /// Returns `true` if the computation was performed, and `false` if we should just evaluate
+    /// the actual MIR of `align_offset`.
     fn align_offset(
         &mut self,
         ptr_op: OpTy<'tcx, Tag>,
         align_op: OpTy<'tcx, Tag>,
         ret: Option<(PlaceTy<'tcx, Tag>, mir::BasicBlock)>,
         unwind: Option<mir::BasicBlock>,
-    ) -> InterpResult<'tcx> {
+    ) -> InterpResult<'tcx, bool> {
         let this = self.eval_context_mut();
         let (dest, ret) = ret.unwrap();
 
+        if this.memory.extra.check_alignment != AlignmentCheck::Symbolic {
+            // Just use actual implementation.
+            return Ok(false);
+        }
+
         let req_align = this
             .force_bits(this.read_scalar(align_op)?.check_init()?, this.pointer_size())?;
 
         // Stop if the alignment is not a power of two.
         if !req_align.is_power_of_two() {
-            return this.start_panic("align_offset: align is not a power-of-two", unwind);
+            this.start_panic("align_offset: align is not a power-of-two", unwind)?;
+            return Ok(true); // nothing left to do
         }
 
         let ptr_scalar = this.read_scalar(ptr_op)?.check_init()?;
 
-        // Default: no result.
-        let mut result = this.machine_usize_max();
         if let Ok(ptr) = this.force_ptr(ptr_scalar) {
             // Only do anything if we can identify the allocation this goes to.
             let cur_align =
                 this.memory.get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)?.1.bytes();
             if u128::from(cur_align) >= req_align {
                 // If the allocation alignment is at least the required alignment we use the
-                // libcore implementation.
-                // FIXME: is this correct in case of truncation?
-                result = u64::try_from(
-                    (this.force_bits(ptr_scalar, this.pointer_size())? as *const i8)
-                        .align_offset(usize::try_from(req_align).unwrap())
-                ).unwrap();
+                // real implementation.
+                return Ok(false);
             }
         }
 
-        // Return result, and jump to caller.
-        this.write_scalar(Scalar::from_machine_usize(result, this), dest)?;
+        // Return error result (usize::MAX), and jump to caller.
+        this.write_scalar(Scalar::from_machine_usize(this.machine_usize_max(), this), dest)?;
         this.go_to_block(ret);
-        Ok(())
+        Ok(true)
     }
 }
diff --git a/tests/compile-fail/unaligned_pointers/atomic_unaligned.rs b/tests/compile-fail/unaligned_pointers/atomic_unaligned.rs
@@ -1,3 +1,4 @@
+// compile-flags: -Zmiri-symbolic-alignment-check
 #![feature(core_intrinsics)]
 
 fn main() {
diff --git a/tests/compile-fail/unaligned_pointers/dyn_alignment.rs b/tests/compile-fail/unaligned_pointers/dyn_alignment.rs
@@ -1,4 +1,4 @@
-// should find the bug even without these, but gets masked by optimizations
+// should find the bug even without validation and stacked borrows, but gets masked by optimizations
 // compile-flags: -Zmiri-disable-validation -Zmiri-disable-stacked-borrows -Zmir-opt-level=0
 
 #[repr(align(256))]
@@ -15,5 +15,5 @@ fn main() {
     // Overwrite the data part of `ptr` so it points to `buf`.
     unsafe { (&mut ptr as *mut _ as *mut *const u8).write(&buf as *const _ as *const u8); }
     // Re-borrow that. This should be UB.
-    let _ptr = &*ptr; //~ ERROR accessing memory with alignment 4, but alignment 256 is required
+    let _ptr = &*ptr; //~ ERROR alignment 256 is required
 }
diff --git a/tests/compile-fail/unaligned_pointers/intptrcast_alignment_check.rs b/tests/compile-fail/unaligned_pointers/intptrcast_alignment_check.rs
@@ -1,7 +1,9 @@
-// Even with intptrcast and without validation, we want to be *sure* to catch bugs
-// that arise from pointers being insufficiently aligned. The only way to achieve
-// that is not not let programs exploit integer information for alignment, so here
-// we test that this is indeed the case.
+// compile-flags: -Zmiri-symbolic-alignment-check
+// With the symbolic alignment check, even with intptrcast and without
+// validation, we want to be *sure* to catch bugs that arise from pointers being
+// insufficiently aligned. The only way to achieve that is not not let programs
+// exploit integer information for alignment, so here we test that this is
+// indeed the case.
 //
 // See https://github.com/rust-lang/miri/issues/1074.
 fn main() {
diff --git a/tests/compile-fail/unaligned_pointers/reference_to_packed.rs b/tests/compile-fail/unaligned_pointers/reference_to_packed.rs
@@ -15,5 +15,5 @@ fn main() {
         y: 99,
     };
     let p = unsafe { &foo.x };
-    let i = *p; //~ ERROR memory with alignment 1, but alignment 4 is required
+    let i = *p; //~ ERROR alignment 4 is required
 }
diff --git a/tests/compile-fail/unaligned_pointers/unaligned_ptr_addr_of.rs b/tests/compile-fail/unaligned_pointers/unaligned_ptr_addr_of.rs
@@ -4,9 +4,11 @@
 use std::ptr;
 
 fn main() {
-    let x = [2u16, 3, 4]; // Make it big enough so we don't get an out-of-bounds error.
-    let x = &x[0] as *const _ as *const u32;
-    // This must fail because alignment is violated: the allocation's base is not sufficiently aligned.
-    // The deref is UB even if we just put the result into a raw pointer.
-    let _x = unsafe { ptr::raw_const!(*x) }; //~ ERROR memory with alignment 2, but alignment 4 is required
+    for _ in 0..10 { // Try many times as this might work by chance.
+        let x = [2u16, 3, 4]; // Make it big enough so we don't get an out-of-bounds error.
+        let x = &x[0] as *const _ as *const u32;
+        // This must fail because alignment is violated: the allocation's base is not sufficiently aligned.
+        // The deref is UB even if we just put the result into a raw pointer.
+        let _x = unsafe { ptr::raw_const!(*x) }; //~ ERROR memory with alignment 2, but alignment 4 is required
+    }
 }
diff --git a/tests/compile-fail/unaligned_pointers/unaligned_ptr_zst.rs b/tests/compile-fail/unaligned_pointers/unaligned_ptr_zst.rs
diff --git a/tests/run-pass/align.rs b/tests/run-pass/align.rs
diff --git a/tests/run-pass/align_offset_symbolic.rs b/tests/run-pass/align_offset_symbolic.rs
diff --git a/tests/run-pass/align_offset_symbolic.stdout b/tests/run-pass/align_offset_symbolic.stdout
diff --git a/tests/run-pass/panic/catch_panic.rs b/tests/run-pass/panic/catch_panic.rs
diff --git a/tests/run-pass/panic/catch_panic.stderr b/tests/run-pass/panic/catch_panic.stderr

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-814bc4fe9364865bfaa94d7825b8eabc11245c7c`
	`1`	`+8cdc94e84040ce797fd33d0a7cfda4ec4f2f2421`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+// compile-flags: -Zmiri-symbolic-alignment-check`
`1`	`2`	`#![feature(core_intrinsics)]`
`2`	`3`
`3`	`4`	`fn main() {`