Skip to content

Add runtime validity checks inside MaybeUninit::assume_init #98073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Prev Previous commit
Next Next commit
Properly emit a slice of Invariant, add tests
  • Loading branch information
5225225 committed Jun 18, 2022
commit 3d94e2514c31ddd79f04369a7763c8362007d135
16 changes: 16 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
Original file line number Diff line number Diff line change
@@ -100,6 +100,22 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
let b_llval = bx.const_usize((end - start) as u64);
OperandValue::Pair(a_llval, b_llval)
}
ConstValue::CustomSlice { data, length } => {
let Abi::ScalarPair(a_scalar, _) = layout.abi else {
bug!("from_const: invalid ScalarPair layout: {:#?}", layout);
};
let a = Scalar::from_pointer(
Pointer::new(bx.tcx().create_memory_alloc(data), Size::ZERO),
&bx.tcx(),
);
let a_llval = bx.scalar_to_backend(
a,
a_scalar,
bx.scalar_pair_element_backend_type(layout, 0, true),
);
let b_llval = bx.const_usize(length as u64);
OperandValue::Pair(a_llval, b_llval)
}
ConstValue::ByRef { alloc, offset } => {
return bx.load_operand(bx.from_const_alloc(layout, alloc, offset));
}
11 changes: 8 additions & 3 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
use std::convert::TryFrom;

use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
use rustc_middle::mir::{
self,
interpret::{ConstValue, GlobalId, InterpResult, Scalar},
@@ -106,8 +107,8 @@ pub(crate) fn eval_nullary_intrinsic<'tcx>(
},
sym::validity_invariants_of => {
ensure_monomorphic_enough(tcx, tp_ty)?;
let alloc = validity_invariants_of::alloc_validity_invariants_of(tcx, tp_ty);
ConstValue::Slice { data: alloc, start: 0, end: alloc.inner().len() }
let (data, length) = validity_invariants_of::alloc_validity_invariants_of(tcx, tp_ty);
ConstValue::CustomSlice { data, length }
}
other => bug!("`{}` is not a zero arg intrinsic", other),
})
@@ -176,7 +177,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
sym::needs_drop => self.tcx.types.bool,
sym::type_id => self.tcx.types.u64,
sym::type_name => self.tcx.mk_static_str(),
sym::validity_invariants_of => self.tcx.mk_static_bytes(),
sym::validity_invariants_of => {
let item = self.tcx.require_lang_item(LangItem::ValidityInvariant, None);
let ty = self.tcx.type_of(item);
self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, self.tcx.mk_slice(ty))
}
_ => bug!(),
};
let val =
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::lang_items::LangItem;
use rustc_middle::mir::interpret::{AllocRange, Allocation, ConstAllocation, Scalar as MirScalar};
use rustc_middle::mir::Mutability;
use rustc_middle::ty::layout::LayoutCx;
use rustc_middle::ty::{ParamEnv, ParamEnvAnd};
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_target::abi::{
Abi, FieldsShape, HasDataLayout, Integer, Primitive, Scalar, Size, TyAndLayout, WrappingRange,
Abi, FieldsShape, HasDataLayout, Integer, Primitive, Scalar, Size, TyAndLayout, WrappingRange, Variants,
};

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -25,7 +26,12 @@ struct InvariantKey {
}

// FIXME: Don't add duplicate invariants (maybe use a HashMap?)
fn add_invariants<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, invs: &mut FxHashMap<InvariantKey, WrappingRange>, offset: Size) {
fn add_invariants<'tcx>(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
invs: &mut FxHashMap<InvariantKey, WrappingRange>,
offset: Size,
) {
let x = tcx.layout_of(ParamEnvAnd { param_env: ParamEnv::reveal_all(), value: ty });

if let Ok(layout) = x {
@@ -46,8 +52,14 @@ fn add_invariants<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, invs: &mut FxHashMap<In
let _: Result<_, _> = invs.try_insert(InvariantKey { offset, size }, valid_range);
}

//dbg!(&ty, &layout);
if !matches!(layout.layout.variants(), Variants::Single { .. }) {
// We *don't* want to look for fields inside enums.
return;
}

let param_env = ParamEnv::reveal_all();
let unwrap = LayoutCx { tcx, param_env };
let layout_cx = LayoutCx { tcx, param_env };

match layout.layout.fields() {
FieldsShape::Primitive => {}
@@ -57,13 +69,13 @@ fn add_invariants<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, invs: &mut FxHashMap<In
// That would lead to false negatives, though.
for idx in 0..*count {
let off = offset + *stride * idx;
let f = layout.field(&unwrap, idx as usize);
let f = layout.field(&layout_cx, idx as usize);
add_invariants(tcx, f.ty, invs, off);
}
}
FieldsShape::Arbitrary { offsets, .. } => {
for (idx, &field_offset) in offsets.iter().enumerate() {
let f = layout.field(&unwrap, idx);
let f = layout.field(&layout_cx, idx);
if f.ty == ty {
// Some types contain themselves as fields, such as
// &mut [T]
@@ -90,7 +102,7 @@ fn get_layout_of_invariant<'tcx>(tcx: TyCtxt<'tcx>) -> TyAndLayout<'tcx, Ty<'tcx
pub(crate) fn alloc_validity_invariants_of<'tcx>(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
) -> ConstAllocation<'tcx> {
) -> (ConstAllocation<'tcx>, usize) {
let mut invs = FxHashMap::default();

let layout = tcx.data_layout();
@@ -126,22 +138,17 @@ pub(crate) fn alloc_validity_invariants_of<'tcx>(

let offset_range = AllocRange { start: offset + start_off, size: Size::from_bytes(16) };
alloc
.write_scalar(
&tcx,
offset_range,
MirScalar::from_u128(invariant.1.start).into(),
)
.write_scalar(&tcx, offset_range, MirScalar::from_u128(invariant.1.start).into())
.unwrap();

let offset_range = AllocRange { start: offset + end_off, size: Size::from_bytes(16) };
alloc
.write_scalar(
&tcx,
offset_range,
MirScalar::from_u128(invariant.1.end).into(),
)
.write_scalar(&tcx, offset_range, MirScalar::from_u128(invariant.1.end).into())
.unwrap();
}

tcx.intern_const_alloc(alloc)
// The allocation is not mutable, we just needed write_scalar.
alloc.mutability = Mutability::Not;

(tcx.intern_const_alloc(alloc), invs.len())
}
13 changes: 13 additions & 0 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
@@ -692,6 +692,19 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self,
))
}
ConstValue::CustomSlice { data, length } => {
// We rely on mutability being set correctly in `data` to prevent writes
// where none should happen.
let ptr = Pointer::new(
self.tcx.create_memory_alloc(data),
Size::ZERO, // offset: 0
);
Operand::Immediate(Immediate::new_slice(
Scalar::from_pointer(self.global_base_pointer(ptr)?, &*self.tcx),
u64::try_from(length).unwrap(),
self,
))
}
};
Ok(OpTy { op, layout })
}
10 changes: 9 additions & 1 deletion compiler/rustc_middle/src/mir/interpret/value.rs
Original file line number Diff line number Diff line change
@@ -37,6 +37,9 @@ pub enum ConstValue<'tcx> {
/// Used only for `&[u8]` and `&str`
Slice { data: ConstAllocation<'tcx>, start: usize, end: usize },

/// Like `Slice`, but for types that aren't 1 byte long.
CustomSlice { data: ConstAllocation<'tcx>, length: usize },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear to me why Slice cannot be adapted for this, but my opinion here is probably not worth much since I don’t fiddle much around mir/interpret.


/// A value not represented/representable by `Scalar` or `Slice`
ByRef {
/// The backing memory of the value, may contain more memory than needed for just the value
@@ -61,6 +64,9 @@ impl<'a, 'tcx> Lift<'tcx> for ConstValue<'a> {
ConstValue::ByRef { alloc, offset } => {
ConstValue::ByRef { alloc: tcx.lift(alloc)?, offset }
}
ConstValue::CustomSlice { data, length } => {
ConstValue::CustomSlice { data: tcx.lift(data)?, length }
}
})
}
}
@@ -69,7 +75,9 @@ impl<'tcx> ConstValue<'tcx> {
#[inline]
pub fn try_to_scalar(&self) -> Option<Scalar<AllocId>> {
match *self {
ConstValue::ByRef { .. } | ConstValue::Slice { .. } => None,
ConstValue::ByRef { .. }
| ConstValue::Slice { .. }
| ConstValue::CustomSlice { .. } => None,
ConstValue::Scalar(val) => Some(val),
}
}
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
@@ -451,6 +451,7 @@ impl<'tcx> Visitor<'tcx> for ExtraComments<'tcx> {
let fmt_val = |val: &ConstValue<'tcx>| match val {
ConstValue::Scalar(s) => format!("Scalar({:?})", s),
ConstValue::Slice { .. } => format!("Slice(..)"),
ConstValue::CustomSlice { .. } => format!("CustomSlice(..)"),
ConstValue::ByRef { .. } => format!("ByRef(..)"),
};

@@ -679,7 +680,9 @@ pub fn write_allocations<'tcx>(
ConstValue::Scalar(interpret::Scalar::Int { .. }) => {
Either::Left(Either::Right(std::iter::empty()))
}
ConstValue::ByRef { alloc, .. } | ConstValue::Slice { data: alloc, .. } => {
ConstValue::ByRef { alloc, .. }
| ConstValue::Slice { data: alloc, .. }
| ConstValue::CustomSlice { data: alloc, .. } => {
Either::Right(alloc_ids_from_alloc(alloc))
}
}
5 changes: 0 additions & 5 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
@@ -2339,11 +2339,6 @@ impl<'tcx> TyCtxt<'tcx> {
self.mk_imm_ref(self.lifetimes.re_static, self.types.str_)
}

#[inline]
pub fn mk_static_bytes(self) -> Ty<'tcx> {
self.mk_imm_ref(self.lifetimes.re_static, self.mk_slice(self.types.u8))
}

#[inline]
pub fn mk_adt(self, def: AdtDef<'tcx>, substs: SubstsRef<'tcx>) -> Ty<'tcx> {
// Take a copy of substs so that we own the vectors inside.
8 changes: 7 additions & 1 deletion compiler/rustc_typeck/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ use crate::require_same_types;

use rustc_errors::struct_span_err;
use rustc_hir as hir;
use rustc_hir::lang_items::LangItem;
use rustc_middle::traits::{ObligationCause, ObligationCauseCode};
use rustc_middle::ty::subst::Subst;
use rustc_middle::ty::{self, TyCtxt};
@@ -192,7 +193,12 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {
sym::needs_drop => (1, Vec::new(), tcx.types.bool),

sym::type_name => (1, Vec::new(), tcx.mk_static_str()),
sym::validity_invariants_of => (1, Vec::new(), tcx.mk_static_bytes()),
sym::validity_invariants_of => {
let item = tcx.require_lang_item(LangItem::ValidityInvariant, None);
let ty = tcx.type_of(item);
let slice = tcx.mk_imm_ref(tcx.lifetimes.re_static, tcx.mk_slice(ty));
(1, Vec::new(), slice)
}
sym::type_id => (1, Vec::new(), tcx.types.u64),
sym::offset | sym::arith_offset => (
1,
48 changes: 20 additions & 28 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
@@ -1976,6 +1976,13 @@ extern "rust-intrinsic" {
/// [`std::hint::black_box`]: crate::hint::black_box
#[rustc_const_unstable(feature = "const_black_box", issue = "none")]
pub fn black_box<T>(dummy: T) -> T;

/// Returns a list of invaraints that must be valid in order for T to be valid.
///
/// This is used internally to allow for runtime assertions inside `MaybeUninit::assume_init`
#[cfg(not(bootstrap))]
#[rustc_const_unstable(feature = "const_intrinsic_validity_invariants_of", issue = "none")]
pub fn validity_invariants_of<T>() -> &'static [Invariant];
}

// Some functions are defined here because they accidentally got made
@@ -2138,10 +2145,10 @@ pub const unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: us
}
}

#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq)]
#[repr(u8)]
#[allow(dead_code)] // https://github.com/rust-lang/rust/issues/85677
enum InvariantSize {
pub enum InvariantSize {
U8 = 0,
U16 = 1,
U32 = 2,
@@ -2156,35 +2163,20 @@ enum InvariantSize {
// Be sure to keep the fields in order (offset, size, start, end), validity_invariants_of.rs needs
// that.
#[cfg(not(bootstrap))]
#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Copy, PartialEq)]
#[lang = "ValidityInvariant"]
struct Invariant {
pub struct Invariant {
/// The offset in bytes from the start of the struct
offset: usize,
pub offset: usize,
/// The size/type of the invariant.
size: InvariantSize,
pub size: InvariantSize,
/// The start point of the valid range of this field. Is allowed to be > valid_range_end, see
/// <https://doc.rust-lang.org/nightly/nightly-rustc/rustc_target/abi/struct.WrappingRange.html>
/// which this follows the semantics of.
valid_range_start: u128,
pub valid_range_start: u128,

/// The end point of the range.
valid_range_end: u128,
}

#[cfg(not(bootstrap))]
/// Returns a list of all validity invariants of the type.
const fn validity_invariants_of<T>() -> &'static [Invariant] {
extern "rust-intrinsic" {
#[rustc_const_unstable(feature = "validity_invariants_of", issue = "none")]
pub fn validity_invariants_of<T>() -> &'static [u8];
}

let invariants: &'static [u8] = validity_invariants_of::<T>();
let sz = invariants.len() / core::mem::size_of::<Invariant>();

// SAFETY: we know this is valid because the intrinsic promises an aligned slice.
unsafe { core::slice::from_raw_parts(invariants.as_ptr().cast(), sz) }
pub valid_range_end: u128,
}

/// Copies `count * size_of::<T>()` bytes from `src` to `dst`. The source
@@ -2445,15 +2437,15 @@ where
}

#[cfg(bootstrap)]
pub(crate) const unsafe fn assert_validity_of<T>(_: *const T) -> bool {
pub const unsafe fn assert_validity_of<T>(_: *const T) -> bool {
true
}

#[cfg(not(bootstrap))]
/// Asserts that the value at `value` is a valid T.
///
/// Best effort, and is UB if the value is invalid.
pub(crate) unsafe fn assert_validity_of<T>(value: *const T) -> bool {
/// Best effort, can miss some UB, and is UB if the value is invalid.
pub unsafe fn assert_validity_of<T>(value: *const T) -> bool {
// We have to do this, since we call assert_validity_of inside MaybeUninit::assume_init
// and if we had used ptr::read_unaligned, that would be a recursive call.
#[repr(packed)]
@@ -2493,11 +2485,11 @@ pub(crate) unsafe fn assert_validity_of<T>(value: *const T) -> bool {

if start > end {
if !((start..=max).contains(&value) || (0..=end).contains(&value)) {
return false;
panic!("read value {value} which was not in range 0..={end} or {start}..={max} at offset {off} in type {}", core::any::type_name::<T>());
}
} else {
if !(start..=end).contains(&value) {
return false;
panic!("read value {value} which was not in range {start}..={end} at offset {off} in type {}", core::any::type_name::<T>());
}
}
}
2 changes: 0 additions & 2 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
@@ -156,8 +156,6 @@
#![feature(const_slice_from_ref)]
#![feature(const_slice_index)]
#![feature(const_is_char_boundary)]
#![cfg_attr(not(bootstrap), feature(validity_invariants_of))]
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change, probably should undo (the extra space does give some sense of separation between blocks that isn’t really there with just a comment alone)

// Language features:
#![feature(abi_unadjusted)]
#![feature(allow_internal_unsafe)]
Loading