Skip to content

Commit 2e17247

Browse files
RalfJungRalfJung
committed
interpret: make read-pointer-as-bytes *always* work in Miri
and show some extra information when it happens in CTFE
1 parent e63a625 commit 2e17247

File tree

15 files changed

+153
-145
lines changed

15 files changed

+153
-145
lines changed

Diff for: ‎compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,10 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(
186186
let size = Size::from_bytes(
187187
4 * ret_lane_count, /* size_of([u32; ret_lane_count]) */
188188
);
189-
alloc.inner().get_bytes(fx, alloc_range(offset, size)).unwrap()
189+
alloc
190+
.inner()
191+
.get_bytes_strip_provenance(fx, alloc_range(offset, size))
192+
.unwrap()
190193
}
191194
_ => unreachable!("{:?}", idx_const),
192195
};

Diff for: ‎compiler/rustc_const_eval/src/const_eval/error.rs

+13
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use rustc_span::{Span, Symbol};
1010
use super::InterpCx;
1111
use crate::interpret::{
1212
struct_error, ErrorHandled, FrameInfo, InterpError, InterpErrorInfo, Machine, MachineStopType,
13+
UnsupportedOpInfo,
1314
};
1415

1516
/// The CTFE machine has some custom error kinds.
@@ -153,6 +154,18 @@ impl<'tcx> ConstEvalErr<'tcx> {
153154
if let Some(span_msg) = span_msg {
154155
err.span_label(self.span, span_msg);
155156
}
157+
// Add some more context for select error types.
158+
match self.error {
159+
InterpError::Unsupported(
160+
UnsupportedOpInfo::ReadPointerAsBytes
161+
| UnsupportedOpInfo::PartialPointerOverwrite(_)
162+
| UnsupportedOpInfo::PartialPointerCopy(_),
163+
) => {
164+
err.help("this code performed an operation that depends on the underlying bytes representing a pointer");
165+
err.help("the absolute address of a pointer is not known at compile-time, so such operations are not supported");
166+
}
167+
_ => {}
168+
}
156169
// Add spans for the stacktrace. Don't print a single-line backtrace though.
157170
if self.stacktrace.len() > 1 {
158171
// Helper closure to print duplicated lines.

Diff for: ‎compiler/rustc_const_eval/src/const_eval/eval_queries.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ use super::{CompileTimeEvalContext, CompileTimeInterpreter, ConstEvalErr};
22
use crate::interpret::eval_nullary_intrinsic;
33
use crate::interpret::{
44
intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId,
5-
Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
6-
StackPopCleanup,
5+
Immediate, InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy,
6+
RefTracking, StackPopCleanup,
77
};
88

99
use rustc_hir::def::DefKind;
@@ -385,7 +385,9 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
385385
ecx.tcx,
386386
"it is undefined behavior to use this value",
387387
|diag| {
388-
diag.note(NOTE_ON_UNDEFINED_BEHAVIOR_ERROR);
388+
if matches!(err.error, InterpError::UndefinedBehavior(_)) {
389+
diag.note(NOTE_ON_UNDEFINED_BEHAVIOR_ERROR);
390+
}
389391
diag.note(&format!(
390392
"the raw bytes of the constant ({}",
391393
display_allocation(

Diff for: ‎compiler/rustc_const_eval/src/interpret/intrinsics.rs

+17-4
Original file line numberDiff line numberDiff line change
@@ -687,10 +687,23 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
687687
let layout = self.layout_of(lhs.layout.ty.builtin_deref(true).unwrap().ty)?;
688688
assert!(!layout.is_unsized());
689689

690-
let lhs = self.read_pointer(lhs)?;
691-
let rhs = self.read_pointer(rhs)?;
692-
let lhs_bytes = self.read_bytes_ptr(lhs, layout.size)?;
693-
let rhs_bytes = self.read_bytes_ptr(rhs, layout.size)?;
690+
let get_bytes = |this: &InterpCx<'mir, 'tcx, M>,
691+
op: &OpTy<'tcx, <M as Machine<'mir, 'tcx>>::Provenance>,
692+
size|
693+
-> InterpResult<'tcx, &[u8]> {
694+
let ptr = this.read_pointer(op)?;
695+
let Some(alloc_ref) = self.get_ptr_alloc(ptr, size, Align::ONE)? else {
696+
// zero-sized access
697+
return Ok(&[]);
698+
};
699+
if alloc_ref.has_provenance() {
700+
throw_ub_format!("`raw_eq` on bytes with provenance");
701+
}
702+
alloc_ref.get_bytes_strip_provenance()
703+
};
704+
705+
let lhs_bytes = get_bytes(self, lhs, layout.size)?;
706+
let rhs_bytes = get_bytes(self, rhs, layout.size)?;
694707
Ok(Scalar::from_bool(lhs_bytes == rhs_bytes))
695708
}
696709
}

Diff for: ‎compiler/rustc_const_eval/src/interpret/memory.rs

+14-10
Original file line numberDiff line numberDiff line change
@@ -953,10 +953,10 @@ impl<'tcx, 'a, Prov: Provenance, Extra> AllocRef<'a, 'tcx, Prov, Extra> {
953953
}
954954

955955
/// `range` is relative to this allocation reference, not the base of the allocation.
956-
pub fn check_bytes(&self, range: AllocRange) -> InterpResult<'tcx> {
956+
pub fn get_bytes_strip_provenance<'b>(&'b self) -> InterpResult<'tcx, &'a [u8]> {
957957
Ok(self
958958
.alloc
959-
.check_bytes(&self.tcx, self.range.subrange(range))
959+
.get_bytes_strip_provenance(&self.tcx, self.range)
960960
.map_err(|e| e.to_interp_error(self.alloc_id))?)
961961
}
962962

@@ -967,10 +967,11 @@ impl<'tcx, 'a, Prov: Provenance, Extra> AllocRef<'a, 'tcx, Prov, Extra> {
967967
}
968968

969969
impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
970-
/// Reads the given number of bytes from memory. Returns them as a slice.
970+
/// Reads the given number of bytes from memory, and strips their provenance if possible.
971+
/// Returns them as a slice.
971972
///
972973
/// Performs appropriate bounds checks.
973-
pub fn read_bytes_ptr(
974+
pub fn read_bytes_ptr_strip_provenance(
974975
&self,
975976
ptr: Pointer<Option<M::Provenance>>,
976977
size: Size,
@@ -983,7 +984,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
983984
// (We are staying inside the bounds here so all is good.)
984985
Ok(alloc_ref
985986
.alloc
986-
.get_bytes(&alloc_ref.tcx, alloc_ref.range)
987+
.get_bytes_strip_provenance(&alloc_ref.tcx, alloc_ref.range)
987988
.map_err(|e| e.to_interp_error(alloc_ref.alloc_id))?)
988989
}
989990

@@ -1078,12 +1079,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
10781079
return Ok(());
10791080
};
10801081

1081-
// This checks provenance edges on the src, which needs to happen before
1082+
// Checks provenance edges on the src, which needs to happen before
10821083
// `prepare_provenance_copy`.
1083-
let src_bytes = src_alloc
1084-
.get_bytes_with_uninit_and_ptr(&tcx, src_range)
1085-
.map_err(|e| e.to_interp_error(src_alloc_id))?
1086-
.as_ptr(); // raw ptr, so we can also get a ptr to the destination allocation
1084+
if src_alloc.range_has_provenance(&tcx, alloc_range(src_range.start, Size::ZERO)) {
1085+
throw_unsup!(PartialPointerCopy(Pointer::new(src_alloc_id, src_range.start)));
1086+
}
1087+
if src_alloc.range_has_provenance(&tcx, alloc_range(src_range.end(), Size::ZERO)) {
1088+
throw_unsup!(PartialPointerCopy(Pointer::new(src_alloc_id, src_range.end())));
1089+
}
1090+
let src_bytes = src_alloc.get_bytes_unchecked(src_range).as_ptr(); // raw ptr, so we can also get a ptr to the destination allocation
10871091
// first copy the provenance to a temporary buffer, because
10881092
// `get_bytes_mut` will clear the provenance, which is correct,
10891093
// since we don't want to keep any provenance at the target.

Diff for: ‎compiler/rustc_const_eval/src/interpret/operand.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
415415
/// Turn the wide MPlace into a string (must already be dereferenced!)
416416
pub fn read_str(&self, mplace: &MPlaceTy<'tcx, M::Provenance>) -> InterpResult<'tcx, &str> {
417417
let len = mplace.len(self)?;
418-
let bytes = self.read_bytes_ptr(mplace.ptr, Size::from_bytes(len))?;
418+
let bytes = self.read_bytes_ptr_strip_provenance(mplace.ptr, Size::from_bytes(len))?;
419419
let str = std::str::from_utf8(bytes).map_err(|err| err_ub!(InvalidStr(err)))?;
420420
Ok(str)
421421
}

Diff for: ‎compiler/rustc_const_eval/src/interpret/place.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
//! into a place.
33
//! All high-level functions to write to memory work on places as destinations.
44
5-
use std::hash::Hash;
6-
75
use rustc_ast::Mutability;
86
use rustc_middle::mir;
97
use rustc_middle::ty;
@@ -290,7 +288,7 @@ impl<'tcx, Prov: Provenance> PlaceTy<'tcx, Prov> {
290288
// FIXME: Working around https://github.com/rust-lang/rust/issues/54385
291289
impl<'mir, 'tcx: 'mir, Prov, M> InterpCx<'mir, 'tcx, M>
292290
where
293-
Prov: Provenance + Eq + Hash + 'static,
291+
Prov: Provenance + 'static,
294292
M: Machine<'mir, 'tcx, Provenance = Prov>,
295293
{
296294
/// Take a value, which represents a (thin or wide) reference, and make it a place.

Diff for: ‎compiler/rustc_const_eval/src/interpret/projection.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
//! but we still need to do bounds checking and adjust the layout. To not duplicate that with MPlaceTy, we actually
88
//! implement the logic on OpTy, and MPlaceTy calls that.
99
10-
use std::hash::Hash;
11-
1210
use rustc_middle::mir;
1311
use rustc_middle::ty;
1412
use rustc_middle::ty::layout::LayoutOf;
@@ -22,7 +20,7 @@ use super::{
2220
// FIXME: Working around https://github.com/rust-lang/rust/issues/54385
2321
impl<'mir, 'tcx: 'mir, Prov, M> InterpCx<'mir, 'tcx, M>
2422
where
25-
Prov: Provenance + Eq + Hash + 'static,
23+
Prov: Provenance + 'static,
2624
M: Machine<'mir, 'tcx, Provenance = Prov>,
2725
{
2826
//# Field access

Diff for: ‎compiler/rustc_const_eval/src/interpret/validity.rs

+12-23
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ use rustc_target::abi::{Abi, Scalar as ScalarAbi, Size, VariantIdx, Variants, Wr
2020
use std::hash::Hash;
2121

2222
use super::{
23-
alloc_range, CheckInAllocMsg, GlobalAlloc, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy,
24-
Machine, MemPlaceMeta, OpTy, Scalar, ValueVisitor,
23+
CheckInAllocMsg, GlobalAlloc, ImmTy, Immediate, InterpCx, InterpResult, MPlaceTy, Machine,
24+
MemPlaceMeta, OpTy, Scalar, ValueVisitor,
2525
};
2626

2727
macro_rules! throw_validation_failure {
@@ -312,7 +312,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
312312
Ok(try_validation!(
313313
self.ecx.read_immediate(op),
314314
self.path,
315-
err_unsup!(ReadPointerAsBytes) => { "(potentially part of) a pointer" } expected { "{expected}" },
316315
err_ub!(InvalidUninitBytes(None)) => { "uninitialized memory" } expected { "{expected}" }
317316
))
318317
}
@@ -345,11 +344,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
345344
// FIXME: check if the type/trait match what ty::Dynamic says?
346345
}
347346
ty::Slice(..) | ty::Str => {
348-
let _len = try_validation!(
349-
meta.unwrap_meta().to_machine_usize(self.ecx),
350-
self.path,
351-
err_unsup!(ReadPointerAsBytes) => { "non-integer slice length in wide pointer" },
352-
);
347+
let _len = meta.unwrap_meta().to_machine_usize(self.ecx)?;
353348
// We do not check that `len * elem_size <= isize::MAX`:
354349
// that is only required for references, and there it falls out of the
355350
// "dereferenceable" check performed by Stacked Borrows.
@@ -669,8 +664,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
669664
{ "{:x}", val } expected { "a valid enum tag" },
670665
err_ub!(InvalidUninitBytes(None)) =>
671666
{ "uninitialized bytes" } expected { "a valid enum tag" },
672-
err_unsup!(ReadPointerAsBytes) =>
673-
{ "a pointer" } expected { "a valid enum tag" },
674667
)
675668
.1)
676669
})
@@ -810,10 +803,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
810803
let mplace = op.assert_mem_place(); // strings are unsized and hence never immediate
811804
let len = mplace.len(self.ecx)?;
812805
try_validation!(
813-
self.ecx.read_bytes_ptr(mplace.ptr, Size::from_bytes(len)),
806+
self.ecx.read_bytes_ptr_strip_provenance(mplace.ptr, Size::from_bytes(len)),
814807
self.path,
815808
err_ub!(InvalidUninitBytes(..)) => { "uninitialized data in `str`" },
816-
err_unsup!(ReadPointerAsBytes) => { "a pointer in `str`" },
817809
);
818810
}
819811
ty::Array(tys, ..) | ty::Slice(tys)
@@ -861,9 +853,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
861853
// We also accept uninit, for consistency with the slow path.
862854
let alloc = self.ecx.get_ptr_alloc(mplace.ptr, size, mplace.align)?.expect("we already excluded size 0");
863855

864-
match alloc.check_bytes(alloc_range(Size::ZERO, size)) {
856+
match alloc.get_bytes_strip_provenance() {
865857
// In the happy case, we needn't check anything else.
866-
Ok(()) => {}
858+
Ok(_) => {}
867859
// Some error happened, try to provide a more detailed description.
868860
Err(err) => {
869861
// For some errors we might be able to provide extra information.
@@ -881,9 +873,6 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
881873

882874
throw_validation_failure!(self.path, { "uninitialized bytes" })
883875
}
884-
err_unsup!(ReadPointerAsBytes) => {
885-
throw_validation_failure!(self.path, { "a pointer" } expected { "plain (non-pointer) bytes" })
886-
}
887876

888877
// Propagate upwards (that will also check for unexpected errors).
889878
_ => return Err(err),
@@ -924,14 +913,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
924913
Ok(()) => Ok(()),
925914
// Pass through validation failures.
926915
Err(err) if matches!(err.kind(), err_ub!(ValidationFailure { .. })) => Err(err),
927-
// Also pass through InvalidProgram, those just indicate that we could not
928-
// validate and each caller will know best what to do with them.
929-
Err(err) if matches!(err.kind(), InterpError::InvalidProgram(_)) => Err(err),
930-
// Avoid other errors as those do not show *where* in the value the issue lies.
931-
Err(err) => {
916+
// Complain about any other kind of UB error -- those are bad because we'd like to
917+
// report them in a way that shows *where* in the value the issue lies.
918+
Err(err) if matches!(err.kind(), InterpError::UndefinedBehavior(_)) => {
932919
err.print_backtrace();
933-
bug!("Unexpected error during validation: {}", err);
920+
bug!("Unexpected Undefined Behavior error during validation: {}", err);
934921
}
922+
// Pass through everything else.
923+
Err(err) => Err(err),
935924
}
936925
}
937926

0 commit comments

Comments
 (0)