From f76f128dc9dea86f52a40d465430a7feddca271a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Aug 2024 15:34:59 +0200 Subject: const-eval interning: accpt interior mutable pointers in final value (but keep rejecting mutable references) --- .../rustc_const_eval/src/check_consts/check.rs | 5 +++-- .../src/const_eval/eval_queries.rs | 26 +++++++++++----------- .../rustc_const_eval/src/const_eval/machine.rs | 23 ++++++++++++++----- compiler/rustc_const_eval/src/errors.rs | 7 ++---- compiler/rustc_const_eval/src/interpret/intern.rs | 18 +++++++++------ 5 files changed, 47 insertions(+), 32 deletions(-) (limited to 'compiler/rustc_const_eval/src') diff --git a/compiler/rustc_const_eval/src/check_consts/check.rs b/compiler/rustc_const_eval/src/check_consts/check.rs index 8be327a8b56..828d8354c63 100644 --- a/compiler/rustc_const_eval/src/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/check_consts/check.rs @@ -538,8 +538,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> { // final value. // Note: This is only sound if every local that has a `StorageDead` has a // `StorageDead` in every control flow path leading to a `return` terminator. - // The good news is that interning will detect if any unexpected mutable - // pointer slips through. + // If anything slips through, there's no safety net -- safe code can create + // references to variants of `!Freeze` enums as long as that variant is `Freeze`, + // so interning can't protect us here. if self.local_is_transient(place.local) { self.check_op(ops::TransientCellBorrow); } else { diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index 7ccebd83f24..594b98f34ef 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -10,7 +10,6 @@ use rustc_middle::traits::Reveal; use rustc_middle::ty::layout::LayoutOf; use rustc_middle::ty::print::with_no_trimmed_paths; use rustc_middle::ty::{self, Ty, TyCtxt}; -use rustc_session::lint; use rustc_span::def_id::LocalDefId; use rustc_span::{Span, DUMMY_SP}; use rustc_target::abi::{self, Abi}; @@ -18,13 +17,12 @@ use tracing::{debug, instrument, trace}; use super::{CanAccessMutGlobal, CompileTimeInterpCx, CompileTimeMachine}; use crate::const_eval::CheckAlignment; -use crate::errors::{self, ConstEvalError, DanglingPtrInFinal}; use crate::interpret::{ create_static_alloc, eval_nullary_intrinsic, intern_const_alloc_recursive, throw_exhaust, CtfeValidationMode, GlobalId, Immediate, InternKind, InternResult, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, StackPopCleanup, }; -use crate::CTRL_C_RECEIVED; +use crate::{errors, CTRL_C_RECEIVED}; // Returns a pointer to where the result lives #[instrument(level = "trace", skip(ecx, body))] @@ -105,18 +103,15 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>( return Err(ecx .tcx .dcx() - .emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind }) + .emit_err(errors::DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind }) .into()); } Err(InternResult::FoundBadMutablePointer) => { - // only report mutable pointers if there were no dangling pointers - let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind }; - ecx.tcx.emit_node_span_lint( - lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE, - ecx.machine.best_lint_scope(*ecx.tcx), - err_diag.span, - err_diag, - ) + return Err(ecx + .tcx + .dcx() + .emit_err(errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind }) + .into()); } } @@ -448,7 +443,12 @@ fn report_eval_error<'tcx>( error, DUMMY_SP, || super::get_span_and_frames(ecx.tcx, ecx.stack()), - |span, frames| ConstEvalError { span, error_kind: kind, instance, frame_notes: frames }, + |span, frames| errors::ConstEvalError { + span, + error_kind: kind, + instance, + frame_notes: frames, + }, ) } diff --git a/compiler/rustc_const_eval/src/const_eval/machine.rs b/compiler/rustc_const_eval/src/const_eval/machine.rs index 7405ca09342..6a691abae02 100644 --- a/compiler/rustc_const_eval/src/const_eval/machine.rs +++ b/compiler/rustc_const_eval/src/const_eval/machine.rs @@ -718,16 +718,29 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> { _kind: mir::RetagKind, val: &ImmTy<'tcx, CtfeProvenance>, ) -> InterpResult<'tcx, ImmTy<'tcx, CtfeProvenance>> { - // If it's a frozen shared reference that's not already immutable, make it immutable. + // If it's a frozen shared reference that's not already immutable, potentially make it immutable. // (Do nothing on `None` provenance, that cannot store immutability anyway.) if let ty::Ref(_, ty, mutbl) = val.layout.ty.kind() && *mutbl == Mutability::Not - && val.to_scalar_and_meta().0.to_pointer(ecx)?.provenance.is_some_and(|p| !p.immutable()) - // That next check is expensive, that's why we have all the guards above. - && ty.is_freeze(*ecx.tcx, ecx.param_env) + && val + .to_scalar_and_meta() + .0 + .to_pointer(ecx)? + .provenance + .is_some_and(|p| !p.immutable()) { + // That next check is expensive, that's why we have all the guards above. + let is_immutable = ty.is_freeze(*ecx.tcx, ecx.param_env); let place = ecx.ref_to_mplace(val)?; - let new_place = place.map_provenance(CtfeProvenance::as_immutable); + let new_place = if is_immutable { + place.map_provenance(CtfeProvenance::as_immutable) + } else { + // Even if it is not immutable, remember that it is a shared reference. + // This allows it to become part of the final value of the constant. + // (See for why we allow this + // even when there is interior mutability.) + place.map_provenance(CtfeProvenance::as_shared_ref) + }; Ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout)) } else { Ok(val.clone()) diff --git a/compiler/rustc_const_eval/src/errors.rs b/compiler/rustc_const_eval/src/errors.rs index 0b366b43f95..57e52254757 100644 --- a/compiler/rustc_const_eval/src/errors.rs +++ b/compiler/rustc_const_eval/src/errors.rs @@ -35,13 +35,10 @@ pub(crate) struct NestedStaticInThreadLocal { pub span: Span, } -#[derive(LintDiagnostic)] +#[derive(Diagnostic)] #[diag(const_eval_mutable_ptr_in_final)] pub(crate) struct MutablePtrInFinal { - // rust-lang/rust#122153: This was marked as `#[primary_span]` under - // `derive(Diagnostic)`. Since we expect we may hard-error in future, we are - // keeping the field (and skipping it under `derive(LintDiagnostic)`). - #[skip_arg] + #[primary_span] pub span: Span, pub kind: InternKind, } diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index 8b0a2afa4d6..e81431b45fb 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -223,16 +223,20 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval continue; } - // Crucially, we check this *before* checking whether the `alloc_id` - // has already been interned. The point of this check is to ensure that when - // there are multiple pointers to the same allocation, they are *all* immutable. - // Therefore it would be bad if we only checked the first pointer to any given - // allocation. + // Ensure that this is is derived from a shared reference. Crucially, we check this *before* + // checking whether the `alloc_id` has already been interned. The point of this check is to + // ensure that when there are multiple pointers to the same allocation, they are *all* + // derived from a shared reference. Therefore it would be bad if we only checked the first + // pointer to any given allocation. // (It is likely not possible to actually have multiple pointers to the same allocation, // so alternatively we could also check that and ICE if there are multiple such pointers.) + // See for why we are checking for + // "shared reference" and not "immutable", i.e., for why we are allowed interior-mutable + // shared references: they can actually be created in safe code while pointing to apparently + // "immutable" values, via promotion of `&None::>`. if intern_kind != InternKind::Promoted && inner_mutability == Mutability::Not - && !prov.immutable() + && !prov.shared_ref() { if ecx.tcx.try_get_global_alloc(alloc_id).is_some() && !just_interned.contains(&alloc_id) @@ -245,7 +249,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval // this to the todo list, since after all it is already interned. continue; } - // Found a mutable pointer inside a const where inner allocations should be + // Found a mutable reference inside a const where inner allocations should be // immutable. We exclude promoteds from this, since things like `&mut []` and // `&None::>` lead to promotion that can produce mutable pointers. We rely // on the promotion analysis not screwing up to ensure that it is sound to intern -- cgit 1.4.1-3-g733a5 From 123757ae07c05d8adafbe00d10b7119608ee13d9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 18 Aug 2024 13:01:36 +0200 Subject: turn errors that should be impossible due to our static checks into ICEs --- compiler/rustc_const_eval/src/interpret/intern.rs | 52 +++++++++++++--------- .../rustc_const_eval/src/interpret/validity.rs | 9 +++- tests/crashes/const_mut_ref_check_bypass.rs | 11 +++++ .../const-eval/heap/alloc_intrinsic_untyped.rs | 3 ++ .../const-eval/heap/alloc_intrinsic_untyped.stderr | 2 +- 5 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 tests/crashes/const_mut_ref_check_bypass.rs (limited to 'compiler/rustc_const_eval/src') diff --git a/compiler/rustc_const_eval/src/interpret/intern.rs b/compiler/rustc_const_eval/src/interpret/intern.rs index e81431b45fb..5df989b4c1d 100644 --- a/compiler/rustc_const_eval/src/interpret/intern.rs +++ b/compiler/rustc_const_eval/src/interpret/intern.rs @@ -20,6 +20,7 @@ use rustc_hir as hir; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs; use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult}; use rustc_middle::query::TyCtxtAt; +use rustc_middle::span_bug; use rustc_middle::ty::layout::TyAndLayout; use rustc_span::def_id::LocalDefId; use rustc_span::sym; @@ -223,41 +224,52 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval continue; } - // Ensure that this is is derived from a shared reference. Crucially, we check this *before* + // Ensure that this is derived from a shared reference. Crucially, we check this *before* // checking whether the `alloc_id` has already been interned. The point of this check is to // ensure that when there are multiple pointers to the same allocation, they are *all* // derived from a shared reference. Therefore it would be bad if we only checked the first // pointer to any given allocation. // (It is likely not possible to actually have multiple pointers to the same allocation, // so alternatively we could also check that and ICE if there are multiple such pointers.) - // See for why we are checking for - // "shared reference" and not "immutable", i.e., for why we are allowed interior-mutable - // shared references: they can actually be created in safe code while pointing to apparently - // "immutable" values, via promotion of `&None::>`. + // See for why we are checking for "shared + // reference" and not "immutable", i.e., for why we are allowing interior-mutable shared + // references: they can actually be created in safe code while pointing to apparently + // "immutable" values, via promotion or tail expression lifetime extension of + // `&None::>`. + // We also exclude promoteds from this as `&mut []` can be promoted, which is a mutable + // reference pointing to an immutable (zero-sized) allocation. We rely on the promotion + // analysis not screwing up to ensure that it is sound to intern promoteds as immutable. if intern_kind != InternKind::Promoted && inner_mutability == Mutability::Not && !prov.shared_ref() { - if ecx.tcx.try_get_global_alloc(alloc_id).is_some() - && !just_interned.contains(&alloc_id) - { + let is_already_global = ecx.tcx.try_get_global_alloc(alloc_id).is_some(); + if is_already_global && !just_interned.contains(&alloc_id) { // This is a pointer to some memory from another constant. We encounter mutable // pointers to such memory since we do not always track immutability through // these "global" pointers. Allowing them is harmless; the point of these checks // during interning is to justify why we intern the *new* allocations immutably, - // so we can completely ignore existing allocations. We also don't need to add - // this to the todo list, since after all it is already interned. + // so we can completely ignore existing allocations. + // We can also skip the rest of this loop iteration, since after all it is already + // interned. continue; } - // Found a mutable reference inside a const where inner allocations should be - // immutable. We exclude promoteds from this, since things like `&mut []` and - // `&None::>` lead to promotion that can produce mutable pointers. We rely - // on the promotion analysis not screwing up to ensure that it is sound to intern - // promoteds as immutable. - trace!("found bad mutable pointer"); - // Prefer dangling pointer errors over mutable pointer errors - if result.is_ok() { - result = Err(InternResult::FoundBadMutablePointer); + // If this is a dangling pointer, that's actually fine -- the problematic case is + // when there is memory there that someone might expect to be mutable, but we make it immutable. + let dangling = !is_already_global && !ecx.memory.alloc_map.contains_key(&alloc_id); + if !dangling { + // Found a mutable reference inside a const where inner allocations should be + // immutable. + if !ecx.tcx.sess.opts.unstable_opts.unleash_the_miri_inside_of_you { + span_bug!( + ecx.tcx.span, + "the static const safety checks accepted mutable references they should not have accepted" + ); + } + // Prefer dangling pointer errors over mutable pointer errors + if result.is_ok() { + result = Err(InternResult::FoundBadMutablePointer); + } } } if ecx.tcx.try_get_global_alloc(alloc_id).is_some() { @@ -265,7 +277,6 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval debug_assert!(!ecx.memory.alloc_map.contains_key(&alloc_id)); 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 @@ -276,6 +287,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval // 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`. + just_interned.insert(alloc_id); match intern_shallow(ecx, alloc_id, inner_mutability) { Ok(nested) => todo.extend(nested), Err(()) => { diff --git a/compiler/rustc_const_eval/src/interpret/validity.rs b/compiler/rustc_const_eval/src/interpret/validity.rs index fb24f983ca9..469af35ec1b 100644 --- a/compiler/rustc_const_eval/src/interpret/validity.rs +++ b/compiler/rustc_const_eval/src/interpret/validity.rs @@ -14,7 +14,6 @@ use hir::def::DefKind; use rustc_ast::Mutability; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; -use rustc_middle::bug; use rustc_middle::mir::interpret::ValidationErrorKind::{self, *}; use rustc_middle::mir::interpret::{ alloc_range, ExpectedKind, InterpError, InvalidMetaKind, Misalignment, PointerKind, Provenance, @@ -22,6 +21,7 @@ use rustc_middle::mir::interpret::{ }; use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout}; use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_middle::{bug, span_bug}; use rustc_span::symbol::{sym, Symbol}; use rustc_target::abi::{ Abi, FieldIdx, FieldsShape, Scalar as ScalarAbi, Size, VariantIdx, Variants, WrappingRange, @@ -617,6 +617,13 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> { if ptr_expected_mutbl == Mutability::Mut && alloc_actual_mutbl == Mutability::Not { + if !self.ecx.tcx.sess.opts.unstable_opts.unleash_the_miri_inside_of_you + { + span_bug!( + self.ecx.tcx.span, + "the static const safety checks accepted mutable references they should not have accepted" + ); + } throw_validation_failure!(self.path, MutableRefToImmutable); } // In a const, everything must be completely immutable. diff --git a/tests/crashes/const_mut_ref_check_bypass.rs b/tests/crashes/const_mut_ref_check_bypass.rs new file mode 100644 index 00000000000..6234536c72b --- /dev/null +++ b/tests/crashes/const_mut_ref_check_bypass.rs @@ -0,0 +1,11 @@ +// Version of `tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs` without the flag that +// suppresses the ICE. +//@ known-bug: #129233 +#![feature(core_intrinsics)] +#![feature(const_heap)] +#![feature(const_mut_refs)] +use std::intrinsics; + +const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 }; + +fn main() {} diff --git a/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs b/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs index 105e8e38d84..7be14e3cff7 100644 --- a/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs +++ b/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs @@ -1,3 +1,6 @@ +// We unleash Miri here since this test demonstrates code that bypasses the checks against interning +// mutable pointers, which currently ICEs. Unleashing Miri silence the ICE. +//@ compile-flags: -Zunleash-the-miri-inside-of-you #![feature(core_intrinsics)] #![feature(const_heap)] #![feature(const_mut_refs)] diff --git a/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr b/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr index bd82e6781be..119198bd347 100644 --- a/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr +++ b/tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr @@ -1,5 +1,5 @@ error: encountered mutable pointer in final value of constant - --> $DIR/alloc_intrinsic_untyped.rs:6:1 + --> $DIR/alloc_intrinsic_untyped.rs:9:1 | LL | const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 }; | ^^^^^^^^^^^^^^^^^^^ -- cgit 1.4.1-3-g733a5