mathieu/os-rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rollup merge of #63075 - RalfJung:deref-checks, r=oli-obk

Browse source 
Miri: Check that a ptr is aligned and inbounds already when evaluating `*`

This syncs Miri with what the Nomicon and the Reference say, and resolves https://github.com/rust-lang/miri/issues/447.

Also this would not have worked without https://github.com/rust-lang/rust/pull/62982 due to new cycles. ;)

r? @oli-obk
This commit is contained in:
Mazdak Farrokhzad 2019-08-14 22:56:20 +02:00 • committed by GitHub
commit c358476c1b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 66 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
src/librustc/mir/interpret/pointer.rs
View file

@ -189,8 +189,11 @@ impl<'tcx, Tag> Pointer<Tag> {
Pointer { alloc_id: self.alloc_id, offset: self.offset, tag: () }
}
/// Test if the pointer is "inbounds" of an allocation of the given size.
/// A pointer is "inbounds" even if its offset is equal to the size; this is
/// a "one-past-the-end" pointer.
#[inline(always)]
pub fn check_in_alloc(
pub fn check_inbounds_alloc(
self,
allocation_size: Size,
msg: CheckInAllocMsg,

4
src/librustc_mir/interpret/memory.rs
View file

@ -368,7 +368,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
// It is sufficient to check this for the end pointer. The addition
// checks for overflow.
let end_ptr = ptr.offset(size, self)?;
end_ptr.check_in_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?;
end_ptr.check_inbounds_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?;
// Test align. Check this last; if both bounds and alignment are violated
// we want the error to be about the bounds.
if let Some(align) = align {
@ -400,7 +400,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
) -> bool {
let (size, _align) = self.get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)
.expect("alloc info with MaybeDead cannot fail");
ptr.check_in_alloc(size, CheckInAllocMsg::NullPointerTest).is_err()
ptr.check_inbounds_alloc(size, CheckInAllocMsg::NullPointerTest).is_err()
}
}

4
src/librustc_mir/interpret/operand.rs
View file

@ -246,7 +246,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
return Ok(None);
}
let ptr = match self.check_mplace_access(mplace, None)? {
let ptr = match self.check_mplace_access(mplace, None)
.expect("places should be checked on creation")
{
Some(ptr) => ptr,
None => return Ok(Some(ImmTy { // zero-sized type
imm: Scalar::zst().into(),

34
src/librustc_mir/interpret/place.rs
View file

@ -277,6 +277,10 @@ where
{
/// Take a value, which represents a (thin or fat) reference, and make it a place.
/// Alignment is just based on the type. This is the inverse of `MemPlace::to_ref()`.
///
/// Only call this if you are sure the place is "valid" (aligned and inbounds), or do not
/// want to ever use the place for memory access!
/// Generally prefer `deref_operand`.
pub fn ref_to_mplace(
&self,
val: ImmTy<'tcx, M::PointerTag>,
@ -304,7 +308,8 @@ where
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
let val = self.read_immediate(src)?;
trace!("deref to {} on {:?}", val.layout.ty, *val);
self.ref_to_mplace(val)
let place = self.ref_to_mplace(val)?;
self.mplace_access_checked(place)
}
/// Check if the given place is good for memory access with the given
@ -327,6 +332,23 @@ where
self.memory.check_ptr_access(place.ptr, size, place.align)
}
/// Return the "access-checked" version of this `MPlace`, where for non-ZST
/// this is definitely a `Pointer`.
pub fn mplace_access_checked(
&self,
mut place: MPlaceTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
let (size, align) = self.size_and_align_of_mplace(place)?
.unwrap_or((place.layout.size, place.layout.align.abi));
assert!(place.mplace.align <= align, "dynamic alignment less strict than static one?");
place.mplace.align = align; // maximally strict checking
// When dereferencing a pointer, it must be non-NULL, aligned, and live.
if let Some(ptr) = self.check_mplace_access(place, Some(size))? {
place.mplace.ptr = ptr.into();
}
Ok(place)
}
/// Force `place.ptr` to a `Pointer`.
/// Can be helpful to avoid lots of `force_ptr` calls later, if this place is used a lot.
pub fn force_mplace_ptr(
@ -750,7 +772,9 @@ where
// to handle padding properly, which is only correct if we never look at this data with the
// wrong type.
let ptr = match self.check_mplace_access(dest, None)? {
let ptr = match self.check_mplace_access(dest, None)
.expect("places should be checked on creation")
{
Some(ptr) => ptr,
None => return Ok(()), // zero-sized access
};
@ -853,8 +877,10 @@ where
});
assert_eq!(src.meta, dest.meta, "Can only copy between equally-sized instances");
let src = self.check_mplace_access(src, Some(size))?;
let dest = self.check_mplace_access(dest, Some(size))?;
let src = self.check_mplace_access(src, Some(size))
.expect("places should be checked on creation");
let dest = self.check_mplace_access(dest, Some(size))
.expect("places should be checked on creation");
let (src_ptr, dest_ptr) = match (src, dest) {
(Some(src_ptr), Some(dest_ptr)) => (src_ptr, dest_ptr),
(None, None) => return Ok(()), // zero-sized copy

8
src/librustc_mir/interpret/step.rs
View file

@ -240,8 +240,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ref(_, _, ref place) => {
let src = self.eval_place(place)?;
let val = self.force_allocation(src)?;
self.write_immediate(val.to_ref(), dest)?;
let place = self.force_allocation(src)?;
if place.layout.size.bytes() > 0 {
// definitely not a ZST
assert!(place.ptr.is_ptr(), "non-ZST places should be normalized to `Pointer`");
}
self.write_immediate(place.to_ref(), dest)?;
}
NullaryOp(mir::NullOp::Box, _) => {

7
src/test/ui/consts/const-eval/ub-nonnull.rs
View file

@ -11,10 +11,11 @@ const NON_NULL_PTR: NonNull<u8> = unsafe { mem::transmute(&1) };
const NULL_PTR: NonNull<u8> = unsafe { mem::transmute(0usize) };
//~^ ERROR it is undefined behavior to use this value
#[deny(const_err)] // this triggers a `const_err` so validation does not even happen
const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
//~^ ERROR it is undefined behavior to use this value
let ptr: &(u8, u8, u8) = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
let out_of_bounds_ptr = &ptr.2; // use address-of-field for pointer arithmetic
let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
// Use address-of-element for pointer arithmetic. This could wrap around to NULL!
let out_of_bounds_ptr = &ptr[255]; //~ ERROR any use of this value will cause an error
mem::transmute(out_of_bounds_ptr)
} };

29
src/test/ui/consts/const-eval/ub-nonnull.stderr
View file

@ -6,21 +6,26 @@ LL | const NULL_PTR: NonNull<u8> = unsafe { mem::transmute(0usize) };
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:14:1
error: any use of this value will cause an error
--> $DIR/ub-nonnull.rs:18:29
|
LL | / const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
LL | |
LL | | let ptr: &(u8, u8, u8) = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
LL | | let out_of_bounds_ptr = &ptr.2; // use address-of-field for pointer arithmetic
LL | | let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
LL | | // Use address-of-element for pointer arithmetic. This could wrap around to NULL!
LL | | let out_of_bounds_ptr = &ptr[255];
| | ^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of allocation 6 which has size 1
LL | | mem::transmute(out_of_bounds_ptr)
LL | | } };
| |____^ type validation failed: encountered a potentially NULL pointer, but expected something that cannot possibly fail to be greater or equal to 1
| |____-
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
note: lint level defined here
--> $DIR/ub-nonnull.rs:14:8
|
LL | #[deny(const_err)] // this triggers a `const_err` so validation does not even happen
| ^^^^^^^^^
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:21:1
--> $DIR/ub-nonnull.rs:22:1
|
LL | const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1
@ -28,7 +33,7 @@ LL | const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) };
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:23:1
--> $DIR/ub-nonnull.rs:24:1
|
LL | const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1
@ -36,7 +41,7 @@ LL | const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) };
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:30:1
--> $DIR/ub-nonnull.rs:31:1
|
LL | const UNINIT: NonZeroU8 = unsafe { Transmute { uninit: () }.out };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected something greater or equal to 1
@ -44,7 +49,7 @@ LL | const UNINIT: NonZeroU8 = unsafe { Transmute { uninit: () }.out };
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:38:1
--> $DIR/ub-nonnull.rs:39:1
|
LL | const BAD_RANGE1: RestrictedRange1 = unsafe { RestrictedRange1(42) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected something in the range 10..=30
@ -52,7 +57,7 @@ LL | const BAD_RANGE1: RestrictedRange1 = unsafe { RestrictedRange1(42) };
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
error[E0080]: it is undefined behavior to use this value
--> $DIR/ub-nonnull.rs:44:1
--> $DIR/ub-nonnull.rs:45:1
|
LL | const BAD_RANGE2: RestrictedRange2 = unsafe { RestrictedRange2(20) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 20, but expected something less or equal to 10, or greater or equal to 30