Skip to content

Commit

Permalink
Check all dangling pointers in validation instead of in interning
Browse files Browse the repository at this point in the history
  • Loading branch information
oli-obk committed Mar 13, 2024
1 parent 38a1733 commit 722aabc
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 34 deletions.
1 change: 0 additions & 1 deletion compiler/rustc_const_eval/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const_eval_dangling_int_pointer =
const_eval_dangling_null_pointer =
{$bad_pointer_message}: null pointer is a dangling pointer (it has no provenance)
const_eval_dangling_ptr_in_final = encountered dangling pointer in final value of {const_eval_intern_kind}
const_eval_dead_local =
accessing a dead local variable
const_eval_dealloc_immutable =
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ fn const_validate_mplace<'mir, 'tcx>(
CtfeValidationMode::Const { allow_immutable_unsafe_cell: !inner }
}
};
ecx.const_validate_operand(&mplace.into(), path, &mut ref_tracking, mode)
ecx.const_validate_operand(mplace, path, &mut ref_tracking, mode)
// Instead of just reporting the `InterpError` via the usual machinery, we give a more targetted
// error about the validation failure.
.map_err(|error| report_validation_error(&ecx, error, alloc_id))?;
Expand Down
8 changes: 0 additions & 8 deletions compiler/rustc_const_eval/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,6 @@ use rustc_target::abi::{Size, WrappingRange};

use crate::interpret::InternKind;

#[derive(Diagnostic)]
#[diag(const_eval_dangling_ptr_in_final)]
pub(crate) struct DanglingPtrInFinal {
#[primary_span]
pub span: Span,
pub kind: InternKind,
}

#[derive(Diagnostic)]
#[diag(const_eval_mutable_ptr_in_final)]
pub(crate) struct MutablePtrInFinal {
Expand Down
6 changes: 2 additions & 4 deletions compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use rustc_span::sym;

use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, PlaceTy};
use crate::const_eval;
use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal};
use crate::errors::MutablePtrInFinal;

pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
'mir,
Expand Down Expand Up @@ -278,9 +278,7 @@ pub fn intern_const_alloc_recursive<
continue;
}
just_interned.insert(alloc_id);
todo.extend(intern_shallow(ecx, alloc_id).map_err(|()| {
ecx.tcx.dcx().emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
})?);
todo.extend(intern_shallow(ecx, alloc_id).unwrap());
}
if found_bad_mutable_pointer {
return Err(ecx
Expand Down
57 changes: 40 additions & 17 deletions compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,17 +606,14 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
if place.layout.is_unsized() {
self.check_wide_ptr_meta(place.meta(), place.layout)?;
}
if let Some(prov) = place.ptr().provenance {
if let Some(alloc_id) = prov.get_alloc_id() {
if let AllocKind::Dead = self.ecx.get_alloc_info(alloc_id).2 {
throw_validation_failure!(
self.path,
DanglingPtrUseAfterFree {
ptr_kind: PointerKind::Ref(Mutability::Not)
}
)
}
}
if self.ref_tracking.is_some()
&& let Some(alloc_id) = place.ptr().provenance.and_then(|p| p.get_alloc_id())
&& let AllocKind::Dead = self.ecx.get_alloc_info(alloc_id).2
{
throw_validation_failure!(
self.path,
DanglingPtrUseAfterFree { ptr_kind: PointerKind::Ref(Mutability::Not) }
)
}
Ok(true)
}
Expand Down Expand Up @@ -988,15 +985,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
path: Vec<PathElem>,
ref_tracking: Option<&mut RefTracking<MPlaceTy<'tcx, M::Provenance>, Vec<PathElem>>>,
ctfe_mode: Option<CtfeValidationMode>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Vec<PathElem>> {
trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);

// Construct a visitor
let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self };

// Run it.
match self.run_for_validation(|| visitor.visit_value(op)) {
Ok(()) => Ok(()),
Ok(()) => Ok(visitor.path),
// Pass through validation failures and "invalid program" issues.
Err(err)
if matches!(
Expand All @@ -1016,7 +1013,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
}
}
}

impl<'mir, 'tcx> InterpCx<'mir, 'tcx, crate::const_eval::CompileTimeInterpreter<'mir, 'tcx>> {
/// This function checks the data at `op` to be const-valid.
/// `op` is assumed to cover valid memory if it is an indirect operand.
/// It will error if the bits at the destination do not match the ones described by the layout.
Expand All @@ -1030,14 +1029,38 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
#[inline(always)]
pub(crate) fn const_validate_operand(
&self,
op: &OpTy<'tcx, M::Provenance>,
mplace: MPlaceTy<'tcx>,
path: Vec<PathElem>,
ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::Provenance>, Vec<PathElem>>,
ref_tracking: &mut RefTracking<MPlaceTy<'tcx>, Vec<PathElem>>,
ctfe_mode: CtfeValidationMode,
) -> InterpResult<'tcx> {
self.validate_operand_internal(op, path, Some(ref_tracking), Some(ctfe_mode))
let prov = mplace.ptr().provenance;
let path = self.validate_operand_internal(
&mplace.into(),
path,
Some(ref_tracking),
Some(ctfe_mode),
)?;

// There was no error, so let's check the rest of the relocations in the pointed-to allocation for
// dangling pointers.
if let Some(prov) = prov
&& let Some((_, alloc)) = self.memory.alloc_map().get(&prov.alloc_id())
{
for (_, prov) in alloc.provenance().ptrs().iter() {
if let AllocKind::Dead = self.get_alloc_info(prov.alloc_id()).2 {
throw_validation_failure!(
path,
DanglingPtrUseAfterFree { ptr_kind: PointerKind::Ref(Mutability::Not) }
)
}
}
}
Ok(())
}
}

impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// This function checks the data at `op` to be runtime-valid.
/// `op` is assumed to cover valid memory if it is an indirect operand.
/// It will error if the bits at the destination do not match the ones described by the layout.
Expand All @@ -1047,6 +1070,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// still correct to not use `ctfe_mode`: that mode is for validation of the final constant
// value, it rules out things like `UnsafeCell` in awkward places. It also can make checking
// recurse through references which, for now, we don't want here, either.
self.validate_operand_internal(op, vec![], None, None)
self.validate_operand_internal(op, vec![], None, None).map(drop)
}
}
2 changes: 1 addition & 1 deletion tests/ui/consts/dangling_raw_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ union Union {
ptr: *const u32
}

const BAR: Union = { //~ ERROR encountered dangling pointer in final value
const BAR: Union = { //~ ERROR it is undefined behavior
let x = 42;
Union { ptr: &x }
};
Expand Down
9 changes: 7 additions & 2 deletions tests/ui/consts/dangling_raw_ptr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ LL | const FOO: *const u32 = {
╾ALLOC0<imm>╼ │ ╾──────╼
}

error: encountered dangling pointer in final value of constant
error[E0080]: it is undefined behavior to use this value
--> $DIR/dangling_raw_ptr.rs:10:1
|
LL | const BAR: Union = {
| ^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^ 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) {
╾ALLOC1<imm>╼ │ ╾──────╼
}

error: aborting due to 2 previous errors

Expand Down

0 comments on commit 722aabc

Please sign in to comment.