Rollup merge of #71655 - RalfJung:const-pattern-soundness, r=oli-obk

Miri: better document and fix dynamic const pattern soundness checks

rust-lang/const-eval#42 got me thinking about soundness for consts being used in patterns, and I found a hole in our existing dynamic checks: a const referring to a mutable static *in a different crate* was not caught. This PR fixes that. It also adds some comments that explain which invariants are crucial for soundness of const-patterns.

Curiously, trying to weaponize this soundness hole failed: pattern matching compilation ICEd when encountering the cross-crate static, saying "expected allocation ID alloc0 to point to memory". I don't know why that would happen, statics *should* be entirely normal memory for pattern matching to access.

r? @oli-obk
Cc @rust-lang/wg-const-eval

Author: Dylan-DPC

src/librustc_mir/const_eval/eval_queries.rs

@@ -193,7 +193,7 @@ fn validate_and_turn_into_const<'tcx>(
                     mplace.into(),
                     path,
                     &mut ref_tracking,
-                    /*may_ref_to_static*/ is_static,
+                    /*may_ref_to_static*/ ecx.memory.extra.can_access_statics,
                 )?;
             }
         }

src/librustc_mir/const_eval/machine.rs

@@ -99,7 +99,12 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {
 
 #[derive(Copy, Clone, Debug)]
 pub struct MemoryExtra {
-    /// Whether this machine may read from statics
+    /// We need to make sure consts never point to anything mutable, even recursively. That is
+    /// relied on for pattern matching on consts with references.
+    /// To achieve this, two pieces have to work together:
+    /// * Interning makes everything outside of statics immutable.
+    /// * Pointers to allocations inside of statics can never leak outside, to a non-static global.
+    /// This boolean here controls the second part.
     pub(super) can_access_statics: bool,
 }
 
@@ -337,6 +342,10 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
             } else if static_def_id.is_some() {
                 // Machine configuration does not allow us to read statics
                 // (e.g., `const` initializer).
+                // See const_eval::machine::MemoryExtra::can_access_statics for why
+                // this check is so important: if we could read statics, we could read pointers
+                // to mutable allocations *inside* statics. These allocations are not themselves
+                // statics, so pointers to them can get around the check in `validity.rs`.
                 Err(ConstEvalErrKind::ConstAccessesStatic.into())
             } else {
                 // Immutable global, this read is fine.

src/librustc_mir/interpret/memory.rs

@@ -453,7 +453,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 // thing here: one maps to `GlobalAlloc::Static`, this is the "lazy" ID,
                 // and the other one is maps to `GlobalAlloc::Memory`, this is returned by
                 // `const_eval_raw` and it is the "resolved" ID.
-                // The resolved ID is never used by the interpreted progrma, it is hidden.
+                // The resolved ID is never used by the interpreted program, it is hidden.
+                // This is relied upon for soundness of const-patterns; a pointer to the resolved
+                // ID would "sidestep" the checks that make sure consts do not point to statics!
                 // The `GlobalAlloc::Memory` branch here is still reachable though; when a static
                 // contains a reference to memory that was created during its evaluation (i.e., not
                 // to another static), those inner references only exist in "resolved" form.

src/librustc_mir/interpret/validity.rs

@@ -404,19 +404,27 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 // Skip validation entirely for some external statics
                 let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id);
                 if let Some(GlobalAlloc::Static(did)) = alloc_kind {
-                    // `extern static` cannot be validated as they have no body.
-                    // FIXME: Statics from other crates are also skipped.
-                    // They might be checked at a different type, but for now we
-                    // want to avoid recursing too deeply.  This is not sound!
-                    if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
-                        return Ok(());
-                    }
+                    // See const_eval::machine::MemoryExtra::can_access_statics for why
+                    // this check is so important.
+                    // This check is reachable when the const just referenced the static,
+                    // but never read it (so we never entered `before_access_global`).
+                    // We also need to do it here instead of going on to avoid running
+                    // into the `before_access_global` check during validation.
                     if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
                         throw_validation_failure!(
                             format_args!("a {} pointing to a static variable", kind),
                             self.path
                         );
                     }
+                    // `extern static` cannot be validated as they have no body.
+                    // FIXME: Statics from other crates are also skipped.
+                    // They might be checked at a different type, but for now we
+                    // want to avoid recursing too deeply.  We might miss const-invalid data,
+                    // but things are still sound otherwise (in particular re: consts
+                    // referring to statics).
+                    if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
+                        return Ok(());
+                    }
                 }
             }
             // Proceed recursively even for ZST, no reason to skip them!

src/test/ui/consts/miri_unleashed/auxiliary/static_cross_crate.rs

@@ -0,0 +1,3 @@
+pub static mut ZERO: [u8; 1] = [0];
+pub static ZERO_REF: &[u8; 1] = unsafe { &ZERO };
+pub static mut OPT_ZERO: Option<u8> = Some(0);

src/test/ui/consts/miri_unleashed/const_refers_to_static.rs

@@ -7,7 +7,8 @@
 use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 
-// These tests only cause an error when *using* the const.
+// These fail during CTFE (as they read a static), so they only cause an error
+// when *using* the const.
 
 const MUTATE_INTERIOR_MUT: usize = {
     static FOO: AtomicUsize = AtomicUsize::new(0);

src/test/ui/consts/miri_unleashed/const_refers_to_static.stderr

@@ -1,47 +1,47 @@
 warning: skipping const checks
-  --> $DIR/const_refers_to_static.rs:14:5
+  --> $DIR/const_refers_to_static.rs:15:5
    |
 LL |     FOO.fetch_add(1, Ordering::Relaxed)
    |     ^^^
 
 warning: skipping const checks
-  --> $DIR/const_refers_to_static.rs:14:5
+  --> $DIR/const_refers_to_static.rs:15:5
    |
 LL |     FOO.fetch_add(1, Ordering::Relaxed)
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 warning: skipping const checks
-  --> $DIR/const_refers_to_static.rs:21:17
+  --> $DIR/const_refers_to_static.rs:22:17
    |
 LL |     unsafe { *(&FOO as *const _ as *const usize) }
    |                 ^^^
 
 warning: skipping const checks
-  --> $DIR/const_refers_to_static.rs:26:32
+  --> $DIR/const_refers_to_static.rs:27:32
    |
 LL | const READ_MUT: u32 = unsafe { MUTABLE };
    |                                ^^^^^^^
 
 warning: skipping const checks
-  --> $DIR/const_refers_to_static.rs:26:32
+  --> $DIR/const_refers_to_static.rs:27:32
    |
 LL | const READ_MUT: u32 = unsafe { MUTABLE };
    |                                ^^^^^^^
 
 error[E0080]: erroneous constant used
-  --> $DIR/const_refers_to_static.rs:31:5
+  --> $DIR/const_refers_to_static.rs:32:5
    |
 LL |     MUTATE_INTERIOR_MUT;
    |     ^^^^^^^^^^^^^^^^^^^ referenced constant has errors
 
 error[E0080]: erroneous constant used
-  --> $DIR/const_refers_to_static.rs:33:5
+  --> $DIR/const_refers_to_static.rs:34:5
    |
 LL |     READ_INTERIOR_MUT;
    |     ^^^^^^^^^^^^^^^^^ referenced constant has errors
 
 error[E0080]: erroneous constant used
-  --> $DIR/const_refers_to_static.rs:35:5
+  --> $DIR/const_refers_to_static.rs:36:5
    |
 LL |     READ_MUT;
    |     ^^^^^^^^ referenced constant has errors

src/test/ui/consts/miri_unleashed/const_refers_to_static2.rs

@@ -6,16 +6,21 @@
 use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 
-// These tests cause immediate error when *defining* the const.
+// These only fail during validation (they do not use but just create a reference to a static),
+// so they cause an immediate error when *defining* the const.
 
 const REF_INTERIOR_MUT: &usize = { //~ ERROR undefined behavior to use this value
+//~| NOTE encountered a reference pointing to a static variable
+//~| NOTE
     static FOO: AtomicUsize = AtomicUsize::new(0);
     unsafe { &*(&FOO as *const _ as *const usize) }
     //~^ WARN skipping const checks
 };
 
 // ok some day perhaps
 const READ_IMMUT: &usize = { //~ ERROR it is undefined behavior to use this value
+//~| NOTE encountered a reference pointing to a static variable
+//~| NOTE
     static FOO: usize = 0;
     &FOO
     //~^ WARN skipping const checks

src/test/ui/consts/miri_unleashed/const_refers_to_static2.stderr

@@ -1,19 +1,21 @@
 warning: skipping const checks
-  --> $DIR/const_refers_to_static2.rs:13:18
+  --> $DIR/const_refers_to_static2.rs:16:18
    |
 LL |     unsafe { &*(&FOO as *const _ as *const usize) }
    |                  ^^^
 
 warning: skipping const checks
-  --> $DIR/const_refers_to_static2.rs:20:6
+  --> $DIR/const_refers_to_static2.rs:25:6
    |
 LL |     &FOO
    |      ^^^
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/const_refers_to_static2.rs:11:1
+  --> $DIR/const_refers_to_static2.rs:12:1
    |
 LL | / const REF_INTERIOR_MUT: &usize = {
+LL | |
+LL | |
 LL | |     static FOO: AtomicUsize = AtomicUsize::new(0);
 LL | |     unsafe { &*(&FOO as *const _ as *const usize) }
 LL | |
@@ -23,9 +25,11 @@ LL | | };
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/const_refers_to_static2.rs:18:1
+  --> $DIR/const_refers_to_static2.rs:21:1
    |
 LL | / const READ_IMMUT: &usize = {
+LL | |
+LL | |
 LL | |     static FOO: usize = 0;
 LL | |     &FOO
 LL | |

src/test/ui/consts/miri_unleashed/const_refers_to_static_cross_crate.rs

@@ -0,0 +1,81 @@
+// compile-flags: -Zunleash-the-miri-inside-of-you -Zdeduplicate-diagnostics
+// aux-build:static_cross_crate.rs
+#![allow(const_err)]
+
+#![feature(exclusive_range_pattern, half_open_range_patterns, const_if_match, const_panic)]
+
+extern crate static_cross_crate;
+
+// Sneaky: reference to a mutable static.
+// Allowing this would be a disaster for pattern matching, we could violate exhaustiveness checking!
+const SLICE_MUT: &[u8; 1] = { //~ ERROR undefined behavior to use this value
+//~| NOTE encountered a reference pointing to a static variable
+//~| NOTE
+    unsafe { &static_cross_crate::ZERO }
+    //~^ WARN skipping const checks
+};
+
+const U8_MUT: &u8 = { //~ ERROR undefined behavior to use this value
+//~| NOTE encountered a reference pointing to a static variable
+//~| NOTE
+    unsafe { &static_cross_crate::ZERO[0] }
+    //~^ WARN skipping const checks
+};
+
+// Also test indirection that reads from other static. This causes a const_err.
+#[warn(const_err)] //~ NOTE
+const U8_MUT2: &u8 = { //~ NOTE
+    unsafe { &(*static_cross_crate::ZERO_REF)[0] }
+    //~^ WARN skipping const checks
+    //~| WARN [const_err]
+    //~| NOTE constant accesses static
+};
+#[warn(const_err)] //~ NOTE
+const U8_MUT3: &u8 = { //~ NOTE
+    unsafe { match static_cross_crate::OPT_ZERO { Some(ref u) => u, None => panic!() } }
+    //~^ WARN skipping const checks
+    //~| WARN [const_err]
+    //~| NOTE constant accesses static
+};
+
+pub fn test(x: &[u8; 1]) -> bool {
+    match x {
+        SLICE_MUT => true,
+        //~^ ERROR could not evaluate constant pattern
+        &[1..] => false,
+    }
+}
+
+pub fn test2(x: &u8) -> bool {
+    match x {
+        U8_MUT => true,
+        //~^ ERROR could not evaluate constant pattern
+        &(1..) => false,
+    }
+}
+
+// We need to use these *in a pattern* to trigger the failure... likely because
+// the errors above otherwise stop compilation too early?
+pub fn test3(x: &u8) -> bool {
+    match x {
+        U8_MUT2 => true,
+        //~^ ERROR could not evaluate constant pattern
+        &(1..) => false,
+    }
+}
+pub fn test4(x: &u8) -> bool {
+    match x {
+        U8_MUT3 => true,
+        //~^ ERROR could not evaluate constant pattern
+        &(1..) => false,
+    }
+}
+
+fn main() {
+    unsafe {
+        static_cross_crate::ZERO[0] = 1;
+    }
+    // Now the pattern is not exhaustive any more!
+    test(&[0]);
+    test2(&0);
+}