Move validation before interning

Author: oli-obk

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -16,12 +16,12 @@ use super::{CanAccessMutGlobal, CompileTimeEvalContext, CompileTimeInterpreter};
 use crate::const_eval::CheckAlignment;
 use crate::errors;
 use crate::errors::ConstEvalError;
-use crate::interpret::eval_nullary_intrinsic;
 use crate::interpret::{
     create_static_alloc, intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate,
     InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
     StackPopCleanup,
 };
+use crate::interpret::{eval_nullary_intrinsic, patch_mutability_of_allocs};
 
 // Returns a pointer to where the result lives
 #[instrument(level = "trace", skip(ecx, body))]
@@ -81,11 +81,13 @@ fn eval_body_using_ecx<'mir, 'tcx, R: InterpretationResult<'tcx>>(
     // The main interpreter loop.
     while ecx.step()? {}
 
-    // Intern the result
-    intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
+    patch_mutability_of_allocs(ecx, intern_kind, &ret)?;
 
     // Since evaluation had no errors, validate the resulting constant.
-    const_validate_mplace(&ecx, &ret, cid)?;
+    const_validate_mplace(ecx, &ret, cid)?;
+
+    // Intern the result
+    intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
 
     Ok(R::make_result(ret, ecx))
 }

compiler/rustc_const_eval/src/const_eval/valtrees.rs

@@ -10,11 +10,11 @@ use super::machine::CompileTimeEvalContext;
 use super::{ValTreeCreationError, ValTreeCreationResult, VALTREE_MAX_NODES};
 use crate::const_eval::CanAccessMutGlobal;
 use crate::errors::MaxNumNodesInConstErr;
-use crate::interpret::MPlaceTy;
 use crate::interpret::{
     intern_const_alloc_recursive, ImmTy, Immediate, InternKind, MemPlaceMeta, MemoryKind, PlaceTy,
     Projectable, Scalar,
 };
+use crate::interpret::{patch_mutability_of_allocs, MPlaceTy};
 
 #[instrument(skip(ecx), level = "debug")]
 fn branches<'tcx>(
@@ -323,6 +323,7 @@ pub fn valtree_to_const_value<'tcx>(
 
             valtree_into_mplace(&mut ecx, &place, valtree);
             dump_place(&ecx, &place);
+            patch_mutability_of_allocs(&mut ecx, InternKind::Constant, &place).unwrap();
             intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &place).unwrap();
 
             op_to_const(&ecx, &place.into(), /* for diagnostics */ false)
@@ -359,6 +360,7 @@ fn valtree_to_ref<'tcx>(
 
     valtree_into_mplace(ecx, &pointee_place, valtree);
     dump_place(ecx, &pointee_place);
+    patch_mutability_of_allocs(ecx, InternKind::Constant, &pointee_place).unwrap();
     intern_const_alloc_recursive(ecx, InternKind::Constant, &pointee_place).unwrap();
 
     pointee_place.to_ref(&ecx.tcx)

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -61,26 +61,13 @@ impl HasStaticRootDefId for const_eval::CompileTimeInterpreter<'_, '_> {
 fn intern_shallow<'rt, 'mir, 'tcx, T, M: CompileTimeMachine<'mir, 'tcx, T>>(
     ecx: &'rt mut InterpCx<'mir, 'tcx, M>,
     alloc_id: AllocId,
-    mutability: Mutability,
 ) -> Result<impl Iterator<Item = CtfeProvenance> + 'tcx, ()> {
     trace!("intern_shallow {:?}", alloc_id);
     // remove allocation
     // FIXME(#120456) - is `swap_remove` correct?
-    let Some((_kind, mut alloc)) = ecx.memory.alloc_map.swap_remove(&alloc_id) else {
+    let Some((_kind, alloc)) = ecx.memory.alloc_map.swap_remove(&alloc_id) else {
         return Err(());
     };
-    // Set allocation mutability as appropriate. This is used by LLVM to put things into
-    // read-only memory, and also by Miri when evaluating other globals that
-    // access this one.
-    match mutability {
-        Mutability::Not => {
-            alloc.mutability = Mutability::Not;
-        }
-        Mutability::Mut => {
-            // This must be already mutable, we won't "un-freeze" allocations ever.
-            assert_eq!(alloc.mutability, Mutability::Mut);
-        }
-    }
     // link the alloc id to the actual allocation
     let alloc = ecx.tcx.mk_const_alloc(alloc);
     if let Some(static_id) = ecx.machine.static_def_id() {
@@ -122,14 +109,9 @@ pub enum InternKind {
     Promoted,
 }
 
-/// Intern `ret` and everything it references.
-///
-/// This *cannot raise an interpreter error*. Doing so is left to validation, which
-/// tracks where in the value we are and thus can show much better error messages.
-///
-/// For `InternKind::Static` the root allocation will not be interned, but must be handled by the caller.
-#[instrument(level = "debug", skip(ecx))]
-pub fn intern_const_alloc_recursive<
+/// Now that evaluation is finished, and we are not going to modify allocations anymore,
+/// recursively mark all allocations as immutable if the item kind calls for it (const/promoted/immut static).
+pub fn patch_mutability_of_allocs<
     'mir,
     'tcx: 'mir,
     M: CompileTimeMachine<'mir, 'tcx, const_eval::MemoryKind>,
@@ -140,12 +122,12 @@ pub fn intern_const_alloc_recursive<
 ) -> Result<(), ErrorGuaranteed> {
     // We are interning recursively, and for mutability we are distinguishing the "root" allocation
     // that we are starting in, and all other allocations that we are encountering recursively.
-    let (base_mutability, inner_mutability, is_static) = match intern_kind {
+    let (base_mutability, inner_mutability) = match intern_kind {
         InternKind::Constant | InternKind::Promoted => {
             // Completely immutable. Interning anything mutably here can only lead to unsoundness,
             // since all consts are conceptually independent values but share the same underlying
             // memory.
-            (Mutability::Not, Mutability::Not, false)
+            (Mutability::Not, Mutability::Not)
         }
         InternKind::Static(Mutability::Not) => {
             (
@@ -158,30 +140,79 @@ pub fn intern_const_alloc_recursive<
                 // Inner allocations are never mutable. They can only arise via the "tail
                 // expression" / "outer scope" rule, and we treat them consistently with `const`.
                 Mutability::Not,
-                true,
             )
         }
         InternKind::Static(Mutability::Mut) => {
             // Just make everything mutable. We accept code like
             // `static mut X = &mut [42]`, so even inner allocations need to be mutable.
-            (Mutability::Mut, Mutability::Mut, true)
+            (Mutability::Mut, Mutability::Mut)
         }
     };
+    let base_alloc_id = ret.ptr().provenance.unwrap().alloc_id();
+    let mut todo: Vec<_> = {
+        let base_alloc = &mut ecx.memory.alloc_map.get_mut(&base_alloc_id).unwrap().1;
+        base_alloc.mutability = base_mutability;
+        base_alloc.provenance().ptrs().iter().copied().collect()
+    };
+    let mut seen = FxHashSet::default();
+    seen.insert(base_alloc_id);
+    while let Some((_, prov)) = todo.pop() {
+        if !seen.insert(prov.alloc_id()) {
+            // Already processed
+            continue;
+        }
+        let Some((_, alloc)) = &mut ecx.memory.alloc_map.get_mut(&prov.alloc_id()) else {
+            continue;
+        };
+        // We always intern with `inner_mutability`, and furthermore we ensured above that if
+        // that is "immutable", then there are *no* mutable pointers anywhere in the newly
+        // interned memory -- justifying that we can indeed intern immutably. However this also
+        // means we can *not* easily intern immutably here if `prov.immutable()` is true and
+        // `inner_mutability` is `Mut`: there might be other pointers to that allocation, and
+        // we'd have to somehow check that they are *all* immutable before deciding that this
+        // allocation can be made immutable. In the future we could consider analyzing all
+        // pointers before deciding which allocations can be made immutable; but for now we are
+        // okay with losing some potential for immutability here. This can anyway only affect
+        // `static mut`.
+        alloc.mutability = inner_mutability;
+        todo.extend(alloc.provenance().ptrs().iter().copied());
+    }
+    Ok(())
+}
+
+/// Intern `ret` and everything it references.
+///
+/// This *cannot raise an interpreter error*. Doing so is left to validation, which
+/// tracks where in the value we are and thus can show much better error messages.
+///
+/// For `InternKind::Static` the root allocation will not be interned, but must be handled by the caller.
+#[instrument(level = "debug", skip(ecx))]
+pub fn intern_const_alloc_recursive<
+    'mir,
+    'tcx: 'mir,
+    M: CompileTimeMachine<'mir, 'tcx, const_eval::MemoryKind>,
+>(
+    ecx: &mut InterpCx<'mir, 'tcx, M>,
+    intern_kind: InternKind,
+    ret: &MPlaceTy<'tcx>,
+) -> Result<(), ErrorGuaranteed> {
+    let (inner_mutability, is_static) = match intern_kind {
+        InternKind::Constant | InternKind::Promoted => (Mutability::Not, false),
+        InternKind::Static(mutability) => (mutability, true),
+    };
 
     // Intern the base allocation, and initialize todo list for recursive interning.
     let base_alloc_id = ret.ptr().provenance.unwrap().alloc_id();
-    trace!(?base_alloc_id, ?base_mutability);
+    trace!(?base_alloc_id);
     // First we intern the base allocation, as it requires a different mutability.
     // This gives us the initial set of nested allocations, which will then all be processed
     // recursively in the loop below.
     let mut todo: Vec<_> = if is_static {
         // Do not steal the root allocation, we need it later to create the return value of `eval_static_initializer`.
-        // But still change its mutability to match the requested one.
-        let alloc = ecx.memory.alloc_map.get_mut(&base_alloc_id).unwrap();
-        alloc.1.mutability = base_mutability;
+        let alloc = ecx.memory.alloc_map.get(&base_alloc_id).unwrap();
         alloc.1.provenance().ptrs().iter().map(|&(_, prov)| prov).collect()
     } else {
-        intern_shallow(ecx, base_alloc_id, base_mutability).unwrap().collect()
+        intern_shallow(ecx, base_alloc_id).unwrap().collect()
     };
     // We need to distinguish "has just been interned" from "was already in `tcx`",
     // so we track this in a separate set.
@@ -247,17 +278,7 @@ pub fn intern_const_alloc_recursive<
             continue;
         }
         just_interned.insert(alloc_id);
-        // We always intern with `inner_mutability`, and furthermore we ensured above that if
-        // that is "immutable", then there are *no* mutable pointers anywhere in the newly
-        // interned memory -- justifying that we can indeed intern immutably. However this also
-        // means we can *not* easily intern immutably here if `prov.immutable()` is true and
-        // `inner_mutability` is `Mut`: there might be other pointers to that allocation, and
-        // we'd have to somehow check that they are *all* immutable before deciding that this
-        // allocation can be made immutable. In the future we could consider analyzing all
-        // pointers before deciding which allocations can be made immutable; but for now we are
-        // okay with losing some potential for immutability here. This can anyway only affect
-        // `static mut`.
-        todo.extend(intern_shallow(ecx, alloc_id, inner_mutability).map_err(|()| {
+        todo.extend(intern_shallow(ecx, alloc_id).map_err(|()| {
             ecx.tcx.dcx().emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
         })?);
     }
@@ -287,7 +308,8 @@ pub fn intern_const_alloc_for_constprop<
         return Ok(());
     }
     // Move allocation to `tcx`.
-    for _ in intern_shallow(ecx, alloc_id, Mutability::Not).map_err(|()| err_ub!(DeadLocal))? {
+    ecx.memory.alloc_map.get_mut(&alloc_id).unwrap().1.mutability = Mutability::Not;
+    for _ in intern_shallow(ecx, alloc_id).map_err(|()| err_ub!(DeadLocal))? {
         // We are not doing recursive interning, so we don't currently support provenance.
         // (If this assertion ever triggers, we should just implement a
         // proper recursive interning loop -- or just call `intern_const_alloc_recursive`.
@@ -314,7 +336,8 @@ impl<'mir, 'tcx: 'mir, M: super::intern::CompileTimeMachine<'mir, 'tcx, !>>
         let dest = self.allocate(layout, MemoryKind::Stack)?;
         f(self, &dest.clone().into())?;
         let alloc_id = dest.ptr().provenance.unwrap().alloc_id(); // this was just allocated, it must have provenance
-        for prov in intern_shallow(self, alloc_id, Mutability::Not).unwrap() {
+        self.memory.alloc_map.get_mut(&alloc_id).unwrap().1.mutability = Mutability::Not;
+        for prov in intern_shallow(self, alloc_id).unwrap() {
             // We are not doing recursive interning, so we don't currently support provenance.
             // (If this assertion ever triggers, we should just implement a
             // proper recursive interning loop -- or just call `intern_const_alloc_recursive`.

compiler/rustc_const_eval/src/interpret/mod.rs

@@ -22,7 +22,8 @@ pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in
 
 pub use self::eval_context::{format_interp_error, Frame, FrameInfo, InterpCx, StackPopCleanup};
 pub use self::intern::{
-    intern_const_alloc_for_constprop, intern_const_alloc_recursive, HasStaticRootDefId, InternKind,
+    intern_const_alloc_for_constprop, intern_const_alloc_recursive, patch_mutability_of_allocs,
+    HasStaticRootDefId, InternKind,
 };
 pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackPopJump};
 pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -26,10 +26,12 @@ use rustc_target::abi::{
 
 use std::hash::Hash;
 
+use crate::interpret::AllocKind;
+
 use super::{
-    format_interp_error, machine::AllocMap, AllocId, CheckInAllocMsg, GlobalAlloc, ImmTy,
-    Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable,
-    Scalar, ValueVisitor,
+    format_interp_error, AllocId, CheckInAllocMsg, GlobalAlloc, ImmTy, Immediate, InterpCx,
+    InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable, Scalar,
+    ValueVisitor,
 };
 
 // for the validation errors
@@ -450,10 +452,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
             if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr()) {
                 let mut skip_recursive_check = false;
                 // Let's see what kind of memory this points to.
-                // `unwrap` since dangling pointers have already been handled.
-                let alloc_kind = self.ecx.tcx.try_get_global_alloc(alloc_id).unwrap();
-                let alloc_actual_mutbl = match alloc_kind {
-                    GlobalAlloc::Static(did) => {
+                let alloc_actual_mutbl = match self.ecx.tcx.try_get_global_alloc(alloc_id) {
+                    Some(GlobalAlloc::Static(did)) => {
                         // Special handling for pointers to statics (irrespective of their type).
                         assert!(!self.ecx.tcx.is_thread_local_static(did));
                         assert!(self.ecx.tcx.is_static(did));
@@ -504,11 +504,19 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                             (Mutability::Not, false) => Mutability::Not,
                         }
                     }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
+                    Some(GlobalAlloc::Memory(alloc)) => alloc.inner().mutability,
+                    Some(GlobalAlloc::Function(..) | GlobalAlloc::VTable(..)) => {
                         // These are immutable, we better don't allow mutable pointers here.
                         Mutability::Not
                     }
+                    None => match self.ecx.get_alloc_info(alloc_id).2 {
+                        AllocKind::LiveData => self.ecx.get_alloc_mutability(alloc_id).unwrap(),
+                        AllocKind::Function | AllocKind::VTable => Mutability::Not,
+                        AllocKind::Dead => throw_validation_failure!(
+                            self.path,
+                            DanglingPtrUseAfterFree { ptr_kind }
+                        ),
+                    },
                 };
                 // Mutability check.
                 // If this allocation has size zero, there is no actual mutability here.
@@ -707,13 +715,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
     fn in_mutable_memory(&self, op: &OpTy<'tcx, M::Provenance>) -> bool {
         if let Some(mplace) = op.as_mplace_or_imm().left() {
             if let Some(alloc_id) = mplace.ptr().provenance.and_then(|p| p.get_alloc_id()) {
-                let mutability = match self.ecx.tcx.global_alloc(alloc_id) {
-                    GlobalAlloc::Static(_) => {
-                        self.ecx.memory.alloc_map.get(alloc_id).unwrap().1.mutability
-                    }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    _ => span_bug!(self.ecx.tcx.span, "not a memory allocation"),
-                };
+                let mutability = self.ecx.get_alloc_mutability(alloc_id).unwrap();
                 return mutability == Mutability::Mut;
             }
         }

compiler/rustc_const_eval/src/util/caller_location.rs

@@ -64,8 +64,7 @@ pub(crate) fn const_caller_location_provider(
     );
 
     let loc_place = alloc_caller_location(&mut ecx, file, line, col);
-    if intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &loc_place).is_err() {
-        bug!("intern_const_alloc_recursive should not error in this case")
-    }
+    patch_mutability_of_allocs(&mut ecx, InternKind::Constant, &loc_place).unwrap();
+    intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &loc_place).unwrap();
     mir::ConstValue::Scalar(Scalar::from_maybe_pointer(loc_place.ptr(), &tcx))
 }

tests/ui/consts/const-eval/heap/dealloc_intrinsic_dangling.rs

@@ -5,7 +5,7 @@
 use std::intrinsics;
 
 const _X: &'static u8 = unsafe {
-    //~^ error: dangling pointer in final value of constant
+    //~^ error: it is undefined behavior to use this value
     let ptr = intrinsics::const_allocate(4, 4);
     intrinsics::const_deallocate(ptr, 4, 4);
     &*ptr

tests/ui/consts/const-eval/heap/dealloc_intrinsic_dangling.stderr

@@ -1,14 +1,19 @@
-error: encountered dangling pointer in final value of constant
+error[E0080]: it is undefined behavior to use this value
   --> $DIR/dealloc_intrinsic_dangling.rs:7:1
    |
 LL | const _X: &'static u8 = unsafe {
-   | ^^^^^^^^^^^^^^^^^^^^^
+   | ^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered a dangling reference (use-after-free)
+   |
+   = 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.
+   = note: the raw bytes of the constant (size: 8, align: 8) {
+               ╾ALLOC0<imm>╼                         │ ╾──────╼
+           }
 
 error[E0080]: evaluation of constant value failed
   --> $DIR/dealloc_intrinsic_dangling.rs:18:5
    |
 LL |     *reference
-   |     ^^^^^^^^^^ memory access failed: ALLOC0 has been freed, so this pointer is dangling
+   |     ^^^^^^^^^^ memory access failed: ALLOC1 has been freed, so this pointer is dangling
 
 error: aborting due to 2 previous errors
 

tests/ui/consts/const-mut-refs/mut_ref_in_final_dynamic_check.rs

@@ -33,8 +33,8 @@ const fn helper_dangling() -> Option<&'static mut i32> { unsafe {
     // Undefined behaviour (dangling pointer), who doesn't love tests like this.
     Some(&mut *(&mut 42 as *mut i32))
 } }
-const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
-static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
+const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
+static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
 
 // These are fine! Just statics pointing to mutable statics, nothing fundamentally wrong with this.
 static MUT_STATIC: Option<&mut i32> = helper();