Rollup merge of rust-lang#67337 - oli-obk:no_mut_static_ref_from_const, r=RalfJung

Ensure that evaluating or validating a constant never reads from a static

r? @RalfJung

as per rust-lang#66302 (comment)

This does not yet address the fact that evaluation of a constant can read from a static (under unleash-miri)

Author: Centril

src/librustc_mir/const_eval.rs

@@ -45,9 +45,15 @@ fn mk_eval_cx<'mir, 'tcx>(
     tcx: TyCtxt<'tcx>,
     span: Span,
     param_env: ty::ParamEnv<'tcx>,
+    can_access_statics: bool,
 ) -> CompileTimeEvalContext<'mir, 'tcx> {
     debug!("mk_eval_cx: {:?}", param_env);
-    InterpCx::new(tcx.at(span), param_env, CompileTimeInterpreter::new(), Default::default())
+    InterpCx::new(
+        tcx.at(span),
+        param_env,
+        CompileTimeInterpreter::new(),
+        MemoryExtra { can_access_statics },
+    )
 }
 
 fn op_to_const<'tcx>(
@@ -176,6 +182,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
 #[derive(Clone, Debug)]
 pub enum ConstEvalError {
     NeedsRfc(String),
+    ConstAccessesStatic,
 }
 
 impl<'tcx> Into<InterpErrorInfo<'tcx>> for ConstEvalError {
@@ -195,6 +202,7 @@ impl fmt::Display for ConstEvalError {
                     msg
                 )
             }
+            ConstAccessesStatic => write!(f, "constant accesses static"),
         }
     }
 }
@@ -204,6 +212,7 @@ impl Error for ConstEvalError {
         use self::ConstEvalError::*;
         match *self {
             NeedsRfc(_) => "this feature needs an rfc before being allowed inside constants",
+            ConstAccessesStatic => "constant accesses static",
         }
     }
 
@@ -224,6 +233,12 @@ pub struct CompileTimeInterpreter<'mir, 'tcx> {
     pub(super) loop_detector: snapshot::InfiniteLoopDetector<'mir, 'tcx>,
 }
 
+#[derive(Copy, Clone, Debug)]
+pub struct MemoryExtra {
+    /// Whether this machine may read from statics
+    can_access_statics: bool,
+}
+
 impl<'mir, 'tcx> CompileTimeInterpreter<'mir, 'tcx> {
     fn new() -> Self {
         CompileTimeInterpreter {
@@ -311,7 +326,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
     type ExtraFnVal = !;
 
     type FrameExtra = ();
-    type MemoryExtra = ();
+    type MemoryExtra = MemoryExtra;
     type AllocExtra = ();
 
     type MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>;
@@ -473,7 +488,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
 
     #[inline(always)]
     fn init_allocation_extra<'b>(
-        _memory_extra: &(),
+        _memory_extra: &MemoryExtra,
         _id: AllocId,
         alloc: Cow<'b, Allocation>,
         _kind: Option<MemoryKind<!>>,
@@ -484,7 +499,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
 
     #[inline(always)]
     fn tag_static_base_pointer(
-        _memory_extra: &(),
+        _memory_extra: &MemoryExtra,
         _id: AllocId,
     ) -> Self::PointerTag {
         ()
@@ -527,6 +542,17 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
     fn stack_push(_ecx: &mut InterpCx<'mir, 'tcx, Self>) -> InterpResult<'tcx> {
         Ok(())
     }
+
+    fn before_access_static(
+        memory_extra: &MemoryExtra,
+        _allocation: &Allocation,
+    ) -> InterpResult<'tcx> {
+        if memory_extra.can_access_statics {
+            Ok(())
+        } else {
+            Err(ConstEvalError::ConstAccessesStatic.into())
+        }
+    }
 }
 
 /// Extracts a field of a (variant of a) const.
@@ -540,7 +566,7 @@ pub fn const_field<'tcx>(
     value: &'tcx ty::Const<'tcx>,
 ) -> &'tcx ty::Const<'tcx> {
     trace!("const_field: {:?}, {:?}", field, value);
-    let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
+    let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, false);
     // get the operand again
     let op = ecx.eval_const_to_op(value, None).unwrap();
     // downcast
@@ -560,7 +586,7 @@ pub fn const_caller_location<'tcx>(
     (file, line, col): (Symbol, u32, u32),
 ) -> &'tcx ty::Const<'tcx> {
     trace!("const_caller_location: {}:{}:{}", file, line, col);
-    let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all());
+    let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all(), false);
 
     let loc_ty = tcx.caller_location_ty();
     let loc_place = ecx.alloc_caller_location(file, line, col);
@@ -581,7 +607,7 @@ pub fn const_variant_index<'tcx>(
     val: &'tcx ty::Const<'tcx>,
 ) -> VariantIdx {
     trace!("const_variant_index: {:?}", val);
-    let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
+    let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, false);
     let op = ecx.eval_const_to_op(val, None).unwrap();
     ecx.read_discriminant(op).unwrap().1
 }
@@ -610,7 +636,9 @@ fn validate_and_turn_into_const<'tcx>(
     key: ty::ParamEnvAnd<'tcx, GlobalId<'tcx>>,
 ) -> ::rustc::mir::interpret::ConstEvalResult<'tcx> {
     let cid = key.value;
-    let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env);
+    let def_id = cid.instance.def.def_id();
+    let is_static = tcx.is_static(def_id);
+    let ecx = mk_eval_cx(tcx, tcx.def_span(key.value.instance.def_id()), key.param_env, is_static);
     let val = (|| {
         let mplace = ecx.raw_const_to_mplace(constant)?;
         let mut ref_tracking = RefTracking::new(mplace);
@@ -624,8 +652,7 @@ fn validate_and_turn_into_const<'tcx>(
         // Now that we validated, turn this into a proper constant.
         // Statics/promoteds are always `ByRef`, for the rest `op_to_const` decides
         // whether they become immediates.
-        let def_id = cid.instance.def.def_id();
-        if tcx.is_static(def_id) || cid.promoted.is_some() {
+        if is_static || cid.promoted.is_some() {
             let ptr = mplace.ptr.to_ptr()?;
             Ok(tcx.mk_const(ty::Const {
                 val: ty::ConstKind::Value(ConstValue::ByRef {
@@ -732,12 +759,14 @@ pub fn const_eval_raw_provider<'tcx>(
         return Err(ErrorHandled::Reported);
     }
 
+    let is_static = tcx.is_static(def_id);
+
     let span = tcx.def_span(cid.instance.def_id());
     let mut ecx = InterpCx::new(
         tcx.at(span),
         key.param_env,
         CompileTimeInterpreter::new(),
-        Default::default()
+        MemoryExtra { can_access_statics: is_static },
     );
 
     let res = ecx.load_mir(cid.instance.def, cid.promoted);
@@ -751,7 +780,7 @@ pub fn const_eval_raw_provider<'tcx>(
     }).map_err(|error| {
         let err = error_to_const_error(&ecx, error);
         // errors in statics are always emitted as fatal errors
-        if tcx.is_static(def_id) {
+        if is_static {
             // Ensure that if the above error was either `TooGeneric` or `Reported`
             // an error must be reported.
             let v = err.report_as_error(ecx.tcx, "could not evaluate static initializer");

src/librustc_mir/interpret/intern.rs

@@ -20,7 +20,6 @@ pub trait CompileTimeMachine<'mir, 'tcx> = Machine<
     PointerTag = (),
     ExtraFnVal = !,
     FrameExtra = (),
-    MemoryExtra = (),
     AllocExtra = (),
     MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
 >;
@@ -320,12 +319,20 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
             // We can't call the `intern_shallow` method here, as its logic is tailored to safe
             // references and a `leftover_allocations` set (where we only have a todo-list here).
             // So we hand-roll the interning logic here again.
-            if base_intern_mode != InternMode::Static {
-                // If it's not a static, it *must* be immutable.
-                // We cannot have mutable memory inside a constant.
-                // FIXME: ideally we would assert that they already are immutable, to double-
-                // check our static checks.
-                alloc.mutability = Mutability::Not;
+            match base_intern_mode {
+                InternMode::Static => {}
+                InternMode::Const | InternMode::ConstBase => {
+                    // If it's not a static, it *must* be immutable.
+                    // We cannot have mutable memory inside a constant.
+                    // We use `delay_span_bug` here, because this can be reached in the presence
+                    // of fancy transmutes.
+                    if alloc.mutability == Mutability::Mut {
+                        // For better errors later, mark the allocation as immutable
+                        // (on top of the delayed ICE).
+                        alloc.mutability = Mutability::Not;
+                        ecx.tcx.sess.delay_span_bug(ecx.tcx.span, "mutable allocation in constant");
+                    }
+                }
             }
             let alloc = tcx.intern_const_alloc(alloc);
             tcx.alloc_map.lock().set_alloc_id_memory(alloc_id, alloc);
@@ -337,6 +344,8 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
         } else if ecx.memory.dead_alloc_map.contains_key(&alloc_id) {
             // dangling pointer
             throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into()))
+        } else if ecx.tcx.alloc_map.lock().get(alloc_id).is_none() {
+            span_bug!(ecx.tcx.span, "encountered unknown alloc id {:?}", alloc_id);
         }
     }
     Ok(())

src/librustc_mir/interpret/machine.rs

@@ -212,7 +212,10 @@ pub trait Machine<'mir, 'tcx>: Sized {
     }
 
     /// Called before a `StaticKind::Static` value is accessed.
-    fn before_access_static(_allocation: &Allocation) -> InterpResult<'tcx> {
+    fn before_access_static(
+        _memory_extra: &Self::MemoryExtra,
+        _allocation: &Allocation,
+    ) -> InterpResult<'tcx> {
         Ok(())
     }
 

src/librustc_mir/interpret/memory.rs

@@ -116,15 +116,16 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> HasDataLayout for Memory<'mir, 'tcx, M>
 // carefully copy only the reachable parts.
 impl<'mir, 'tcx, M> Clone for Memory<'mir, 'tcx, M>
 where
-    M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = (), MemoryExtra = ()>,
+    M: Machine<'mir, 'tcx, PointerTag = (), AllocExtra = ()>,
+    M::MemoryExtra: Copy,
     M::MemoryMap: AllocMap<AllocId, (MemoryKind<M::MemoryKinds>, Allocation)>,
 {
     fn clone(&self) -> Self {
         Memory {
             alloc_map: self.alloc_map.clone(),
             extra_fn_ptr_map: self.extra_fn_ptr_map.clone(),
             dead_alloc_map: self.dead_alloc_map.clone(),
-            extra: (),
+            extra: self.extra,
             tcx: self.tcx,
         }
     }
@@ -455,7 +456,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                     let id = raw_const.alloc_id;
                     let allocation = tcx.alloc_map.lock().unwrap_memory(id);
 
-                    M::before_access_static(allocation)?;
+                    M::before_access_static(memory_extra, allocation)?;
                     Cow::Borrowed(allocation)
                 }
             }

src/librustc_mir/transform/const_prop.rs

@@ -224,6 +224,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
     }
 
     fn before_access_static(
+        _memory_extra: &(),
         allocation: &Allocation<Self::PointerTag, Self::AllocExtra>,
     ) -> InterpResult<'tcx> {
         // if the static allocation is mutable or if it has relocations (it may be legal to mutate

src/test/ui/consts/const-points-to-static.rs

@@ -0,0 +1,12 @@
+// compile-flags: -Zunleash-the-miri-inside-of-you
+
+#![allow(dead_code)]
+
+const TEST: &u8 = &MY_STATIC;
+//~^ skipping const checks
+//~| it is undefined behavior to use this value
+
+static MY_STATIC: u8 = 4;
+
+fn main() {
+}

src/test/ui/consts/const-points-to-static.stderr

@@ -0,0 +1,17 @@
+warning: skipping const checks
+  --> $DIR/const-points-to-static.rs:5:20
+   |
+LL | const TEST: &u8 = &MY_STATIC;
+   |                    ^^^^^^^^^
+
+error[E0080]: it is undefined behavior to use this value
+  --> $DIR/const-points-to-static.rs:5:1
+   |
+LL | const TEST: &u8 = &MY_STATIC;
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constant accesses static
+   |
+   = 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: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0080`.

src/test/ui/consts/const-prop-read-static-in-const.rs

@@ -1,9 +1,8 @@
 // compile-flags: -Zunleash-the-miri-inside-of-you
-// run-pass
 
 #![allow(dead_code)]
 
-const TEST: u8 = MY_STATIC;
+const TEST: u8 = MY_STATIC; //~ ERROR any use of this value will cause an error
 //~^ skipping const checks
 
 static MY_STATIC: u8 = 4;

src/test/ui/consts/const-prop-read-static-in-const.stderr

@@ -1,6 +1,18 @@
 warning: skipping const checks
-  --> $DIR/const-prop-read-static-in-const.rs:6:18
+  --> $DIR/const-prop-read-static-in-const.rs:5:18
    |
 LL | const TEST: u8 = MY_STATIC;
    |                  ^^^^^^^^^
 
+error: any use of this value will cause an error
+  --> $DIR/const-prop-read-static-in-const.rs:5:18
+   |
+LL | const TEST: u8 = MY_STATIC;
+   | -----------------^^^^^^^^^-
+   |                  |
+   |                  constant accesses static
+   |
+   = note: `#[deny(const_err)]` on by default
+
+error: aborting due to previous error
+

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

@@ -0,0 +1,38 @@
+// compile-flags: -Zunleash-the-miri-inside-of-you
+#![warn(const_err)]
+
+#![feature(const_raw_ptr_deref)]
+
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering;
+
+const BOO: &usize = { //~ ERROR undefined behavior to use this value
+    static FOO: AtomicUsize = AtomicUsize::new(0);
+    unsafe { &*(&FOO as *const _ as *const usize) }
+    //~^ WARN skipping const checks
+};
+
+const FOO: usize = {
+    static FOO: AtomicUsize = AtomicUsize::new(0);
+    FOO.fetch_add(1, Ordering::Relaxed) //~ WARN any use of this value will cause an error
+    //~^ WARN skipping const checks
+    //~| WARN skipping const checks
+};
+
+const BAR: usize = {
+    static FOO: AtomicUsize = AtomicUsize::new(0);
+    unsafe { *(&FOO as *const _ as *const usize) } //~ WARN any use of this value will cause an err
+    //~^ WARN skipping const checks
+};
+
+static mut MUTABLE: u32 = 0;
+const BAD: u32 = unsafe { MUTABLE }; //~ WARN any use of this value will cause an error
+//~^ WARN skipping const checks
+
+// ok some day perhaps
+const BOO_OK: &usize = { //~ ERROR it is undefined behavior to use this value
+    static FOO: usize = 0;
+    &FOO
+    //~^ WARN skipping const checks
+};
+fn main() {}