Skip to content

Commit 22106c3

Browse files
RalfJungRalfJung
committed
const validation: ensure we don't have &mut to read-only memory or UnsafeCell in read-only memory
1 parent 3fd5624 commit 22106c3

13 files changed

+405
-66
lines changed

‎compiler/rustc_const_eval/messages.ftl

+2-2
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ const_eval_validation_invalid_fn_ptr = {$front_matter}: encountered {$value}, bu
438438
const_eval_validation_invalid_ref_meta = {$front_matter}: encountered invalid reference metadata: total size is bigger than largest supported object
439439
const_eval_validation_invalid_ref_slice_meta = {$front_matter}: encountered invalid reference metadata: slice is bigger than largest supported object
440440
const_eval_validation_invalid_vtable_ptr = {$front_matter}: encountered {$value}, but expected a vtable pointer
441-
const_eval_validation_mutable_ref_in_const = {$front_matter}: encountered mutable reference in a `const`
441+
const_eval_validation_mutable_ref_to_immutable = {$front_matter}: encountered mutable reference or box pointing to read-only memory
442442
const_eval_validation_never_val = {$front_matter}: encountered a value of the never type `!`
443443
const_eval_validation_null_box = {$front_matter}: encountered a null box
444444
const_eval_validation_null_fn_ptr = {$front_matter}: encountered a null function pointer
@@ -456,7 +456,7 @@ const_eval_validation_unaligned_ref = {$front_matter}: encountered an unaligned
456456
const_eval_validation_uninhabited_enum_variant = {$front_matter}: encountered an uninhabited enum variant
457457
const_eval_validation_uninhabited_val = {$front_matter}: encountered a value of uninhabited type `{$ty}`
458458
const_eval_validation_uninit = {$front_matter}: encountered uninitialized memory, but {$expected}
459-
const_eval_validation_unsafe_cell = {$front_matter}: encountered `UnsafeCell` in a `const`
459+
const_eval_validation_unsafe_cell = {$front_matter}: encountered `UnsafeCell` in read-only memory
460460
461461
const_eval_write_to_read_only =
462462
writing to {$allocation} which is read-only

‎compiler/rustc_const_eval/src/const_eval/eval_queries.rs

+13-2
Original file line numberDiff line numberDiff line change
@@ -336,10 +336,21 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
336336
let mode = match tcx.static_mutability(cid.instance.def_id()) {
337337
Some(_) if cid.promoted.is_some() => {
338338
// Promoteds in statics are allowed to point to statics.
339-
CtfeValidationMode::Const { inner, allow_static_ptrs: true }
339+
CtfeValidationMode::Const {
340+
allow_immutable_unsafe_cell: false,
341+
allow_static_ptrs: true,
342+
}
340343
}
341344
Some(_) => CtfeValidationMode::Regular, // a `static`
342-
None => CtfeValidationMode::Const { inner, allow_static_ptrs: false },
345+
None => {
346+
// In normal `const` (not promoted), the outermost allocation is always only copied,
347+
// so having `UnsafeCell` in there is okay despite them being in immutable memory.
348+
let allow_immutable_unsafe_cell = cid.promoted.is_none() && !inner;
349+
CtfeValidationMode::Const {
350+
allow_immutable_unsafe_cell,
351+
allow_static_ptrs: false,
352+
}
353+
}
343354
};
344355
ecx.const_validate_operand(&mplace.into(), path, &mut ref_tracking, mode)?;
345356
inner = true;

‎compiler/rustc_const_eval/src/errors.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -625,13 +625,13 @@ impl<'tcx> ReportErrorExt for ValidationErrorInfo<'tcx> {
625625

626626
PointerAsInt { .. } => const_eval_validation_pointer_as_int,
627627
PartialPointer => const_eval_validation_partial_pointer,
628-
MutableRefInConst => const_eval_validation_mutable_ref_in_const,
628+
MutableRefToImmutable => const_eval_validation_mutable_ref_to_immutable,
629629
NullFnPtr => const_eval_validation_null_fn_ptr,
630630
NeverVal => const_eval_validation_never_val,
631631
NullablePtrOutOfRange { .. } => const_eval_validation_nullable_ptr_out_of_range,
632632
PtrOutOfRange { .. } => const_eval_validation_ptr_out_of_range,
633633
OutOfRange { .. } => const_eval_validation_out_of_range,
634-
UnsafeCell => const_eval_validation_unsafe_cell,
634+
UnsafeCellInImmutable => const_eval_validation_unsafe_cell,
635635
UninhabitedVal { .. } => const_eval_validation_uninhabited_val,
636636
InvalidEnumTag { .. } => const_eval_validation_invalid_enum_tag,
637637
UninhabitedEnumVariant => const_eval_validation_uninhabited_enum_variant,
@@ -778,10 +778,10 @@ impl<'tcx> ReportErrorExt for ValidationErrorInfo<'tcx> {
778778
NullPtr { .. }
779779
| PtrToStatic { .. }
780780
| PtrToMut { .. }
781-
| MutableRefInConst
781+
| MutableRefToImmutable
782782
| NullFnPtr
783783
| NeverVal
784-
| UnsafeCell
784+
| UnsafeCellInImmutable
785785
| InvalidMetaSliceTooLarge { .. }
786786
| InvalidMetaTooLarge { .. }
787787
| DanglingPtrUseAfterFree { .. }

‎compiler/rustc_const_eval/src/interpret/validity.rs

+94-30
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@ use std::num::NonZeroUsize;
99

1010
use either::{Left, Right};
1111

12+
use hir::def::DefKind;
1213
use rustc_ast::Mutability;
1314
use rustc_data_structures::fx::FxHashSet;
1415
use rustc_hir as hir;
1516
use rustc_middle::mir::interpret::{
16-
ExpectedKind, InterpError, InvalidMetaKind, PointerKind, ValidationErrorInfo,
17+
ExpectedKind, InterpError, InvalidMetaKind, PointerKind, Provenance, ValidationErrorInfo,
1718
ValidationErrorKind, ValidationErrorKind::*,
1819
};
1920
use rustc_middle::ty;
@@ -123,15 +124,34 @@ pub enum PathElem {
123124
}
124125

125126
/// Extra things to check for during validation of CTFE results.
127+
#[derive(Copy, Clone)]
126128
pub enum CtfeValidationMode {
127129
/// Regular validation, nothing special happening.
128130
Regular,
129131
/// Validation of a `const`.
130-
/// `inner` says if this is an inner, indirect allocation (as opposed to the top-level const
131-
/// allocation). Being an inner allocation makes a difference because the top-level allocation
132-
/// of a `const` is copied for each use, but the inner allocations are implicitly shared.
132+
/// `allow_immutable_unsafe_cell` says whether we allow `UnsafeCell` in immutable memory (which is the
133+
/// case for the top-level allocation of a `const`, where this is fine because the allocation will be
134+
/// copied at each use site).
133135
/// `allow_static_ptrs` says if pointers to statics are permitted (which is the case for promoteds in statics).
134-
Const { inner: bool, allow_static_ptrs: bool },
136+
Const { allow_immutable_unsafe_cell: bool, allow_static_ptrs: bool },
137+
}
138+
139+
impl CtfeValidationMode {
140+
fn allow_immutable_unsafe_cell(self) -> bool {
141+
match self {
142+
CtfeValidationMode::Regular => false,
143+
CtfeValidationMode::Const { allow_immutable_unsafe_cell, .. } => {
144+
allow_immutable_unsafe_cell
145+
}
146+
}
147+
}
148+
149+
fn allow_static_ptrs(self) -> bool {
150+
match self {
151+
CtfeValidationMode::Regular => true, // statics can point to statics
152+
CtfeValidationMode::Const { allow_static_ptrs, .. } => allow_static_ptrs,
153+
}
154+
}
135155
}
136156

137157
/// State for tracking recursive validation of references
@@ -412,6 +432,21 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
412432
}
413433
// Recursive checking
414434
if let Some(ref_tracking) = self.ref_tracking.as_deref_mut() {
435+
// Determine whether this pointer expects to be pointing to something mutable.
436+
let ptr_expected_mutbl = match ptr_kind {
437+
PointerKind::Box => Mutability::Mut,
438+
PointerKind::Ref => {
439+
let tam = value.layout.ty.builtin_deref(false).unwrap();
440+
// ZST never require mutability. We do not take into account interior mutability here
441+
// since we cannot know if there really is an `UnsafeCell` -- so we check that
442+
// in the recursive descent behind this reference.
443+
if size == Size::ZERO || tam.mutbl == Mutability::Not {
444+
Mutability::Not
445+
} else {
446+
Mutability::Mut
447+
}
448+
}
449+
};
415450
// Proceed recursively even for ZST, no reason to skip them!
416451
// `!` is a ZST and we want to validate it.
417452
if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr()) {
@@ -422,16 +457,29 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
422457
// Special handling for pointers to statics (irrespective of their type).
423458
assert!(!self.ecx.tcx.is_thread_local_static(did));
424459
assert!(self.ecx.tcx.is_static(did));
425-
if matches!(
426-
self.ctfe_mode,
427-
Some(CtfeValidationMode::Const { allow_static_ptrs: false, .. })
428-
) {
460+
if self.ctfe_mode.is_some_and(|c| !c.allow_static_ptrs()) {
429461
// See const_eval::machine::MemoryExtra::can_access_statics for why
430462
// this check is so important.
431463
// This check is reachable when the const just referenced the static,
432464
// but never read it (so we never entered `before_access_global`).
433465
throw_validation_failure!(self.path, PtrToStatic { ptr_kind });
434466
}
467+
// Mutability check.
468+
if ptr_expected_mutbl == Mutability::Mut {
469+
if matches!(
470+
self.ecx.tcx.def_kind(did),
471+
DefKind::Static(Mutability::Not)
472+
) && self
473+
.ecx
474+
.tcx
475+
.type_of(did)
476+
.no_bound_vars()
477+
.expect("statics should not have generic parameters")
478+
.is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all())
479+
{
480+
throw_validation_failure!(self.path, MutableRefToImmutable);
481+
}
482+
}
435483
// We skip recursively checking other statics. These statics must be sound by
436484
// themselves, and the only way to get broken statics here is by using
437485
// unsafe code.
@@ -453,9 +501,20 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
453501
// and we would catch that here.
454502
throw_validation_failure!(self.path, PtrToMut { ptr_kind });
455503
}
504+
if ptr_expected_mutbl == Mutability::Mut
505+
&& alloc.inner().mutability == Mutability::Not
506+
{
507+
throw_validation_failure!(self.path, MutableRefToImmutable);
508+
}
456509
}
457-
// Nothing to check for these.
458-
None | Some(GlobalAlloc::Function(..) | GlobalAlloc::VTable(..)) => {}
510+
Some(GlobalAlloc::Function(..) | GlobalAlloc::VTable(..)) => {
511+
// These are immutable, we better don't allow mutable pointers here.
512+
if ptr_expected_mutbl == Mutability::Mut {
513+
throw_validation_failure!(self.path, MutableRefToImmutable);
514+
}
515+
}
516+
// Dangling, already handled.
517+
None => bug!(),
459518
}
460519
}
461520
let path = &self.path;
@@ -525,17 +584,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
525584
}
526585
Ok(true)
527586
}
528-
ty::Ref(_, ty, mutbl) => {
529-
if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. }))
530-
&& *mutbl == Mutability::Mut
531-
{
532-
// A mutable reference inside a const? That does not seem right (except if it is
533-
// a ZST).
534-
let layout = self.ecx.layout_of(*ty)?;
535-
if !layout.is_zst() {
536-
throw_validation_failure!(self.path, MutableRefInConst);
537-
}
538-
}
587+
ty::Ref(..) => {
539588
self.check_safe_pointer(value, PointerKind::Ref)?;
540589
Ok(true)
541590
}
@@ -636,6 +685,19 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
636685
)
637686
}
638687
}
688+
689+
fn in_mutable_memory(&self, op: &OpTy<'tcx, M::Provenance>) -> bool {
690+
if let Some(mplace) = op.as_mplace_or_imm().left() {
691+
if let Some(alloc_id) = mplace.ptr().provenance.and_then(|p| p.get_alloc_id()) {
692+
if self.ecx.tcx.global_alloc(alloc_id).unwrap_memory().inner().mutability
693+
== Mutability::Mut
694+
{
695+
return true;
696+
}
697+
}
698+
}
699+
false
700+
}
639701
}
640702

641703
impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
@@ -699,10 +761,12 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
699761
op: &OpTy<'tcx, M::Provenance>,
700762
_fields: NonZeroUsize,
701763
) -> InterpResult<'tcx> {
702-
// Special check preventing `UnsafeCell` inside unions in the inner part of constants.
703-
if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true, .. })) {
764+
// Special check for CTFE validation, preventing `UnsafeCell` inside unions in immutable memory.
765+
if self.ctfe_mode.is_some_and(|c| !c.allow_immutable_unsafe_cell()) {
704766
if !op.layout.ty.is_freeze(*self.ecx.tcx, self.ecx.param_env) {
705-
throw_validation_failure!(self.path, UnsafeCell);
767+
if !self.in_mutable_memory(op) {
768+
throw_validation_failure!(self.path, UnsafeCellInImmutable);
769+
}
706770
}
707771
}
708772
Ok(())
@@ -724,11 +788,11 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
724788
}
725789

726790
// Special check preventing `UnsafeCell` in the inner part of constants
727-
if let Some(def) = op.layout.ty.ty_adt_def() {
728-
if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true, .. }))
729-
&& def.is_unsafe_cell()
730-
{
731-
throw_validation_failure!(self.path, UnsafeCell);
791+
if self.ctfe_mode.is_some_and(|c| !c.allow_immutable_unsafe_cell()) {
792+
if let Some(def) = op.layout.ty.ty_adt_def() && def.is_unsafe_cell() {
793+
if !self.in_mutable_memory(op) {
794+
throw_validation_failure!(self.path, UnsafeCellInImmutable);
795+
}
732796
}
733797
}
734798

‎compiler/rustc_middle/src/mir/interpret/error.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -414,13 +414,13 @@ pub enum ValidationErrorKind<'tcx> {
414414
PtrToUninhabited { ptr_kind: PointerKind, ty: Ty<'tcx> },
415415
PtrToStatic { ptr_kind: PointerKind },
416416
PtrToMut { ptr_kind: PointerKind },
417-
MutableRefInConst,
417+
MutableRefToImmutable,
418+
UnsafeCellInImmutable,
418419
NullFnPtr,
419420
NeverVal,
420421
NullablePtrOutOfRange { range: WrappingRange, max_value: u128 },
421422
PtrOutOfRange { range: WrappingRange, max_value: u128 },
422423
OutOfRange { value: String, range: WrappingRange, max_value: u128 },
423-
UnsafeCell,
424424
UninhabitedVal { ty: Ty<'tcx> },
425425
InvalidEnumTag { value: String },
426426
UninhabitedEnumVariant,

‎tests/ui/consts/invalid-union.32bit.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ error[E0080]: it is undefined behavior to use this value
22
--> $DIR/invalid-union.rs:41:1
33
|
44
LL | fn main() {
5-
| ^^^^^^^^^ constructing invalid value at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in a `const`
5+
| ^^^^^^^^^ constructing invalid value at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in read-only memory
66
|
77
= 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.
88
= note: the raw bytes of the constant (size: 4, align: 4) {

‎tests/ui/consts/invalid-union.64bit.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ error[E0080]: it is undefined behavior to use this value
22
--> $DIR/invalid-union.rs:41:1
33
|
44
LL | fn main() {
5-
| ^^^^^^^^^ constructing invalid value at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in a `const`
5+
| ^^^^^^^^^ constructing invalid value at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in read-only memory
66
|
77
= 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.
88
= note: the raw bytes of the constant (size: 8, align: 8) {

0 commit comments

Comments
 (0)