better document const-pattern dynamic soundness checks, and fix a soundness hole

3 files changed, 24 insertions, 9 deletions

diff --git a/src/librustc_mir/const_eval/eval_queries.rs b/src/librustc_mir/const_eval/eval_queries.rs
index 95c5d0f0b10..d97546e4391 100644
--- a/src/librustc_mir/const_eval/eval_queries.rs
+++ b/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,
                 )?;
             }
         }
diff --git a/src/librustc_mir/const_eval/machine.rs b/src/librustc_mir/const_eval/machine.rs
index 84031ec0f17..ce9d25599ff 100644
--- a/src/librustc_mir/const_eval/machine.rs
+++ b/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,8 @@ 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.
                 Err(ConstEvalErrKind::ConstAccessesStatic.into())
             } else {
                 // Immutable global, this read is fine.
diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs
index 838a5b32fc8..df3c3532203 100644
--- a/src/librustc_mir/interpret/validity.rs
+++ b/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!