diff --git a/src/librustc/mir/interpret/pointer.rs b/src/librustc/mir/interpret/pointer.rs index 0a99851337994..b55e6bc54bc5f 100644 --- a/src/librustc/mir/interpret/pointer.rs +++ b/src/librustc/mir/interpret/pointer.rs @@ -189,8 +189,11 @@ impl<'tcx, Tag> Pointer { 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, diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index aef09df4537be..6771d7661e3a4 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -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() } } diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index f778eb1734c4b..139a92c7b1190 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -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(), diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index f66c4adf76397..16686c3800f87 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -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 diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index d152e2b50fa1b..e55827837fa1e 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -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, _) => { diff --git a/src/test/ui/consts/const-eval/ub-nonnull.rs b/src/test/ui/consts/const-eval/ub-nonnull.rs index bcbb4358aec03..431ff356ade19 100644 --- a/src/test/ui/consts/const-eval/ub-nonnull.rs +++ b/src/test/ui/consts/const-eval/ub-nonnull.rs @@ -11,10 +11,11 @@ const NON_NULL_PTR: NonNull = unsafe { mem::transmute(&1) }; const NULL_PTR: NonNull = 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 = { 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) } }; diff --git a/src/test/ui/consts/const-eval/ub-nonnull.stderr b/src/test/ui/consts/const-eval/ub-nonnull.stderr index 2f9423fed3530..7b3c97e5fbf96 100644 --- a/src/test/ui/consts/const-eval/ub-nonnull.stderr +++ b/src/test/ui/consts/const-eval/ub-nonnull.stderr @@ -6,21 +6,26 @@ LL | const NULL_PTR: NonNull = 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 = { 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