Skip to content

Commit d47196b

Browse files
RalfJungRalfJung
committed
miri value visitor: detect primitives by type, not layout
1 parent 6548be2 commit d47196b

File tree

4 files changed

+163
-131
lines changed

4 files changed

+163
-131
lines changed

‎src/librustc_mir/interpret/validity.rs

Lines changed: 158 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -306,23 +306,119 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
306306
Ok(())
307307
}
308308

309-
fn visit_primitive(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
309+
/// Check a reference or `Box`.
310+
fn check_safe_pointer(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
310311
let value = self.ecx.read_immediate(value)?;
312+
// Handle wide pointers.
313+
// Check metadata early, for better diagnostics
314+
let place = try_validation!(self.ecx.ref_to_mplace(value), "undefined pointer", self.path);
315+
if place.layout.is_unsized() {
316+
self.check_wide_ptr_meta(place.meta, place.layout)?;
317+
}
318+
// Make sure this is dereferenceable and all.
319+
let (size, align) = self
320+
.ecx
321+
.size_and_align_of(place.meta, place.layout)?
322+
// for the purpose of validity, consider foreign types to have
323+
// alignment and size determined by the layout (size will be 0,
324+
// alignment should take attributes into account).
325+
.unwrap_or_else(|| (place.layout.size, place.layout.align.abi));
326+
let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align(
327+
place.ptr,
328+
size,
329+
Some(align),
330+
CheckInAllocMsg::InboundsTest,
331+
) {
332+
Ok(ptr) => ptr,
333+
Err(err) => {
334+
info!(
335+
"{:?} did not pass access check for size {:?}, align {:?}",
336+
place.ptr, size, align
337+
);
338+
match err.kind {
339+
err_unsup!(InvalidNullPointerUsage) => {
340+
throw_validation_failure!("a NULL reference", self.path)
341+
}
342+
err_unsup!(AlignmentCheckFailed { required, has }) => {
343+
throw_validation_failure!(
344+
format_args!(
345+
"an unaligned reference \
346+
(required {} byte alignment but found {})",
347+
required.bytes(),
348+
has.bytes()
349+
),
350+
self.path
351+
)
352+
}
353+
err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(
354+
"a dangling reference (created from integer)",
355+
self.path
356+
),
357+
_ => throw_validation_failure!(
358+
"a dangling reference (not entirely in bounds)",
359+
self.path
360+
),
361+
}
362+
}
363+
};
364+
// Recursive checking
365+
if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts {
366+
if let Some(ptr) = ptr {
367+
// not a ZST
368+
// Skip validation entirely for some external statics
369+
let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id);
370+
if let Some(GlobalAlloc::Static(did)) = alloc_kind {
371+
// `extern static` cannot be validated as they have no body.
372+
// FIXME: Statics from other crates are also skipped.
373+
// They might be checked at a different type, but for now we
374+
// want to avoid recursing too deeply. This is not sound!
375+
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
376+
return Ok(());
377+
}
378+
}
379+
}
380+
// Proceed recursively even for ZST, no reason to skip them!
381+
// `!` is a ZST and we want to validate it.
382+
// Normalize before handing `place` to tracking because that will
383+
// check for duplicates.
384+
let place = if size.bytes() > 0 {
385+
self.ecx.force_mplace_ptr(place).expect("we already bounds-checked")
386+
} else {
387+
place
388+
};
389+
let path = &self.path;
390+
ref_tracking.track(place, || {
391+
// We need to clone the path anyway, make sure it gets created
392+
// with enough space for the additional `Deref`.
393+
let mut new_path = Vec::with_capacity(path.len() + 1);
394+
new_path.clone_from(path);
395+
new_path.push(PathElem::Deref);
396+
new_path
397+
});
398+
}
399+
Ok(())
400+
}
401+
402+
/// Check if this is a value of primitive type, and if yes check the validity of the value
403+
/// at that type. Return `true` if the type is indeed primitive.
404+
fn visit_primitive(&mut self, value: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, bool> {
311405
// Go over all the primitive types
312406
let ty = value.layout.ty;
313407
match ty.kind {
314408
ty::Bool => {
315-
let value = value.to_scalar_or_undef();
409+
let value = self.ecx.read_scalar(value)?;
316410
try_validation!(value.to_bool(), value, self.path, "a boolean");
411+
Ok(true)
317412
}
318413
ty::Char => {
319-
let value = value.to_scalar_or_undef();
414+
let value = self.ecx.read_scalar(value)?;
320415
try_validation!(value.to_char(), value, self.path, "a valid unicode codepoint");
416+
Ok(true)
321417
}
322418
ty::Float(_) | ty::Int(_) | ty::Uint(_) => {
419+
let value = self.ecx.read_scalar(value)?;
323420
// NOTE: Keep this in sync with the array optimization for int/float
324421
// types below!
325-
let value = value.to_scalar_or_undef();
326422
if self.ref_tracking_for_consts.is_some() {
327423
// Integers/floats in CTFE: Must be scalar bits, pointers are dangerous
328424
let is_bits = value.not_undef().map_or(false, |v| v.is_bits());
@@ -337,120 +433,68 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
337433
// At run-time, for now, we accept *anything* for these types, including
338434
// undef. We should fix that, but let's start low.
339435
}
436+
Ok(true)
340437
}
341438
ty::RawPtr(..) => {
342439
// We are conservative with undef for integers, but try to
343440
// actually enforce our current rules for raw pointers.
344-
let place =
345-
try_validation!(self.ecx.ref_to_mplace(value), "undefined pointer", self.path);
441+
let place = try_validation!(
442+
self.ecx.ref_to_mplace(self.ecx.read_immediate(value)?),
443+
"undefined pointer",
444+
self.path
445+
);
346446
if place.layout.is_unsized() {
347447
self.check_wide_ptr_meta(place.meta, place.layout)?;
348448
}
449+
Ok(true)
349450
}
350-
_ if ty.is_box() || ty.is_region_ptr() => {
351-
// Handle wide pointers.
352-
// Check metadata early, for better diagnostics
353-
let place =
354-
try_validation!(self.ecx.ref_to_mplace(value), "undefined pointer", self.path);
355-
if place.layout.is_unsized() {
356-
self.check_wide_ptr_meta(place.meta, place.layout)?;
357-
}
358-
// Make sure this is dereferenceable and all.
359-
let (size, align) = self
360-
.ecx
361-
.size_and_align_of(place.meta, place.layout)?
362-
// for the purpose of validity, consider foreign types to have
363-
// alignment and size determined by the layout (size will be 0,
364-
// alignment should take attributes into account).
365-
.unwrap_or_else(|| (place.layout.size, place.layout.align.abi));
366-
let ptr: Option<_> = match self.ecx.memory.check_ptr_access_align(
367-
place.ptr,
368-
size,
369-
Some(align),
370-
CheckInAllocMsg::InboundsTest,
371-
) {
372-
Ok(ptr) => ptr,
373-
Err(err) => {
374-
info!(
375-
"{:?} did not pass access check for size {:?}, align {:?}",
376-
place.ptr, size, align
377-
);
378-
match err.kind {
379-
err_unsup!(InvalidNullPointerUsage) => {
380-
throw_validation_failure!("a NULL reference", self.path)
381-
}
382-
err_unsup!(AlignmentCheckFailed { required, has }) => {
383-
throw_validation_failure!(
384-
format_args!(
385-
"an unaligned reference \
386-
(required {} byte alignment but found {})",
387-
required.bytes(),
388-
has.bytes()
389-
),
390-
self.path
391-
)
392-
}
393-
err_unsup!(ReadBytesAsPointer) => throw_validation_failure!(
394-
"a dangling reference (created from integer)",
395-
self.path
396-
),
397-
_ => throw_validation_failure!(
398-
"a dangling reference (not entirely in bounds)",
399-
self.path
400-
),
401-
}
402-
}
403-
};
404-
// Recursive checking
405-
if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts {
406-
if let Some(ptr) = ptr {
407-
// not a ZST
408-
// Skip validation entirely for some external statics
409-
let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id);
410-
if let Some(GlobalAlloc::Static(did)) = alloc_kind {
411-
// `extern static` cannot be validated as they have no body.
412-
// FIXME: Statics from other crates are also skipped.
413-
// They might be checked at a different type, but for now we
414-
// want to avoid recursing too deeply. This is not sound!
415-
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
416-
return Ok(());
417-
}
418-
}
419-
}
420-
// Proceed recursively even for ZST, no reason to skip them!
421-
// `!` is a ZST and we want to validate it.
422-
// Normalize before handing `place` to tracking because that will
423-
// check for duplicates.
424-
let place = if size.bytes() > 0 {
425-
self.ecx.force_mplace_ptr(place).expect("we already bounds-checked")
426-
} else {
427-
place
428-
};
429-
let path = &self.path;
430-
ref_tracking.track(place, || {
431-
// We need to clone the path anyway, make sure it gets created
432-
// with enough space for the additional `Deref`.
433-
let mut new_path = Vec::with_capacity(path.len() + 1);
434-
new_path.clone_from(path);
435-
new_path.push(PathElem::Deref);
436-
new_path
437-
});
438-
}
451+
ty::Ref(..) => {
452+
self.check_safe_pointer(value)?;
453+
Ok(true)
454+
}
455+
ty::Adt(def, ..) if def.is_box() => {
456+
// FIXME make sure we have a test for `Box`!
457+
self.check_safe_pointer(value)?;
458+
Ok(true)
439459
}
440460
ty::FnPtr(_sig) => {
441-
let value = value.to_scalar_or_undef();
461+
let value = self.ecx.read_scalar(value)?;
442462
let _fn = try_validation!(
443463
value.not_undef().and_then(|ptr| self.ecx.memory.get_fn(ptr)),
444464
value,
445465
self.path,
446466
"a function pointer"
447467
);
448468
// FIXME: Check if the signature matches
469+
Ok(true)
449470
}
450-
// This should be all the (inhabited) primitive types
451-
_ => bug!("Unexpected primitive type {}", value.layout.ty),
471+
ty::Never => throw_validation_failure!("a value of the never type `!`", self.path),
472+
ty::Foreign(..) | ty::FnDef(..) => {
473+
// Nothing to check.
474+
Ok(true)
475+
}
476+
// This should be all the (inhabited) primitive types. The rest is compound, we
477+
// check them by visiting their fields/variants.
478+
// (`Str` UTF-8 check happens in `visit_aggregate`, too.)
479+
ty::Adt(..)
480+
| ty::Tuple(..)
481+
| ty::Array(..)
482+
| ty::Slice(..)
483+
| ty::Str
484+
| ty::Dynamic(..)
485+
| ty::Closure(..)
486+
| ty::Generator(..)
487+
| ty::GeneratorWitness(..) => Ok(false),
488+
// Some types only occur during inference, we should not see them here.
489+
ty::Error
490+
| ty::Infer(..)
491+
| ty::Placeholder(..)
492+
| ty::Bound(..)
493+
| ty::Param(..)
494+
| ty::Opaque(..)
495+
| ty::UnnormalizedProjection(..)
496+
| ty::Projection(..) => bug!("Encountered invalid type {:?}", ty),
452497
}
453-
Ok(())
454498
}
455499

456500
fn visit_scalar(
@@ -558,40 +602,23 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
558602
}
559603

560604
#[inline(always)]
561-
fn visit_union(&mut self, _v: Self::V, fields: usize) -> InterpResult<'tcx> {
562-
// Empty unions are not accepted by rustc. That's great, it means we can
563-
// use that as a signal for detecting primitives. Make sure
564-
// we did not miss any primitive.
565-
assert!(fields > 0);
605+
fn visit_union(&mut self, op: OpTy<'tcx, M::PointerTag>, fields: usize) -> InterpResult<'tcx> {
606+
// Empty unions are not accepted by rustc. But uninhabited enums
607+
// claim to be unions, so allow them, too.
608+
assert!(op.layout.abi.is_uninhabited() || fields > 0);
566609
Ok(())
567610
}
568611

569612
#[inline]
570613
fn visit_value(&mut self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
571614
trace!("visit_value: {:?}, {:?}", *op, op.layout);
572615

573-
if op.layout.abi.is_uninhabited() {
574-
// Uninhabited types do not have sensible layout, stop right here.
575-
throw_validation_failure!(
576-
format_args!("a value of uninhabited type {:?}", op.layout.ty),
577-
self.path
578-
)
579-
}
580-
581-
// Check primitive types. We do this after checking for uninhabited types,
582-
// to exclude uninhabited enums (that also appear as fieldless unions here).
583-
// Primitives can have varying layout, so we check them separately and before aggregate
584-
// handling.
585-
// It is CRITICAL that we get this check right, or we might be validating the wrong thing!
586-
let primitive = match op.layout.fields {
587-
// Primitives appear as Union with 0 fields - except for Boxes and fat pointers.
588-
layout::FieldPlacement::Union(0) => true,
589-
_ => op.layout.ty.builtin_deref(true).is_some(),
590-
};
591-
if primitive {
592-
// No need to recurse further or check scalar layout, this is a leaf type.
593-
return self.visit_primitive(op);
616+
// Check primitive types -- the leafs of our recursive descend.
617+
if self.visit_primitive(op)? {
618+
return Ok(());
594619
}
620+
// Sanity check: `builtin_deref` does not know any pointers that are not primitive.
621+
assert!(op.layout.ty.builtin_deref(true).is_none());
595622

596623
// Recursively walk the type. Translate some possible errors to something nicer.
597624
match self.walk_value(op) {
@@ -618,7 +645,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
618645
// scalars, we do the same check on every "level" (e.g., first we check
619646
// MyNewtype and then the scalar in there).
620647
match op.layout.abi {
621-
layout::Abi::Uninhabited => unreachable!(), // checked above
648+
layout::Abi::Uninhabited => {
649+
throw_validation_failure!(
650+
format_args!("a value of uninhabited type {:?}", op.layout.ty),
651+
self.path
652+
);
653+
}
622654
layout::Abi::Scalar(ref scalar_layout) => {
623655
self.visit_scalar(op, scalar_layout)?;
624656
}

‎src/test/ui/consts/const-eval/ub-uninhabit.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ error[E0080]: it is undefined behavior to use this value
1818
--> $DIR/ub-uninhabit.rs:21:1
1919
|
2020
LL | const BAD_BAD_ARRAY: [Bar; 1] = unsafe { (TransmuteUnion::<(), [Bar; 1]> { a: () }).b };
21-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Bar; 1]
21+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Bar at [0]
2222
|
2323
= 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.
2424

‎src/test/ui/consts/const-eval/validate_uninhabited_zsts.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ error[E0080]: it is undefined behavior to use this value
2020
--> $DIR/validate_uninhabited_zsts.rs:17:1
2121
|
2222
LL | const BAR: [Empty; 3] = [unsafe { std::mem::transmute(()) }; 3];
23-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type [Empty; 3]
23+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Empty at [0]
2424
|
2525
= 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.
2626

0 commit comments

Comments
 (0)