From c50b9d197f54016714ad45808450e018c2c253b3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 14:26:36 +0200 Subject: [PATCH 1/8] Centralize bounds, alignment and NULL checking for memory accesses in one function: memory.check_ptr_access That function also takes care of converting a Scalar to a Pointer, should that be needed. Not all accesses need that though: if the access has size 0, None is returned. Everyone accessing memory based on a Scalar should use this method to get the Pointer they need. All operations on the Allocation work on Pointer inputs and expect all the checks to have happened (and will ICE if the bounds are violated). The operations on Memory work on Scalar inputs and do the checks themselves. The only other public method to check pointers is memory.ptr_may_be_null, which is needed in a few places. With this, we can make all the other methods (tests for a pointer being in-bounds and checking alignment) private helper methods, used to implement the two public methods. That maks the public API surface much easier to use and harder to mis-use. While I am at it, this also removes the assumption that the vtable part of a `dyn Trait`-fat-pointer is a `Pointer` (as opposed to a pointer cast to an integer, stored as raw bits). --- src/librustc/mir/interpret/allocation.rs | 100 +++++++++-------- src/librustc_mir/interpret/eval_context.rs | 2 +- src/librustc_mir/interpret/memory.rs | 124 ++++++++++++++++----- src/librustc_mir/interpret/operand.rs | 29 ++--- src/librustc_mir/interpret/place.rs | 24 ++-- src/librustc_mir/interpret/terminator.rs | 15 ++- src/librustc_mir/interpret/traits.rs | 20 +++- src/librustc_mir/interpret/validity.rs | 51 +++------ 8 files changed, 214 insertions(+), 151 deletions(-) diff --git a/src/librustc/mir/interpret/allocation.rs b/src/librustc/mir/interpret/allocation.rs index 8381125798a0d..964929547b77b 100644 --- a/src/librustc/mir/interpret/allocation.rs +++ b/src/librustc/mir/interpret/allocation.rs @@ -8,7 +8,7 @@ use crate::ty::layout::{Size, Align}; use syntax::ast::Mutability; use std::{iter, fmt::{self, Display}}; use crate::mir; -use std::ops::{Deref, DerefMut}; +use std::ops::{Range, Deref, DerefMut}; use rustc_data_structures::sorted_map::SortedMap; use rustc_macros::HashStable; use rustc_target::abi::HasDataLayout; @@ -146,37 +146,30 @@ impl Allocation { impl<'tcx> ::serialize::UseSpecializedDecodable for &'tcx Allocation {} -/// Alignment and bounds checks -impl<'tcx, Tag, Extra> Allocation { - /// Checks if the pointer is "in-bounds". Notice that a pointer pointing at the end - /// of an allocation (i.e., at the first *inaccessible* location) *is* considered - /// in-bounds! This follows C's/LLVM's rules. - /// If you want to check bounds before doing a memory access, better use `check_bounds`. - fn check_bounds_ptr( +/// Byte accessors +impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { + /// Just a small local helper function to avoid a bit of code repetition. + /// Returns the range of this allocation that was meant. + #[inline] + fn check_bounds( &self, - ptr: Pointer, - msg: CheckInAllocMsg, - ) -> InterpResult<'tcx> { - let allocation_size = self.bytes.len() as u64; - ptr.check_in_alloc(Size::from_bytes(allocation_size), msg) + offset: Size, + size: Size + ) -> Range { + let end = offset + size; // this does overflow checking + assert_eq!( + end.bytes() as usize as u64, end.bytes(), + "cannot handle this access on this host architecture" + ); + let end = end.bytes() as usize; + assert!( + end <= self.bytes.len(), + "Out-of-bounds access at offset {}, size {} in allocation of size {}", + offset.bytes(), size.bytes(), self.bytes.len() + ); + (offset.bytes() as usize)..end } - /// Checks if the memory range beginning at `ptr` and of size `Size` is "in-bounds". - #[inline(always)] - pub fn check_bounds( - &self, - cx: &impl HasDataLayout, - ptr: Pointer, - size: Size, - msg: CheckInAllocMsg, - ) -> InterpResult<'tcx> { - // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow) - self.check_bounds_ptr(ptr.offset(size, cx)?, msg) - } -} - -/// Byte accessors -impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// The last argument controls whether we error out when there are undefined /// or pointer bytes. You should never call this, call `get_bytes` or /// `get_bytes_with_undef_and_ptr` instead, @@ -184,16 +177,17 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// This function also guarantees that the resulting pointer will remain stable /// even when new allocations are pushed to the `HashMap`. `copy_repeatedly` relies /// on that. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. fn get_bytes_internal( &self, cx: &impl HasDataLayout, ptr: Pointer, size: Size, check_defined_and_ptr: bool, - msg: CheckInAllocMsg, ) -> InterpResult<'tcx, &[u8]> { - self.check_bounds(cx, ptr, size, msg)?; + let range = self.check_bounds(ptr.offset, size); if check_defined_and_ptr { self.check_defined(ptr, size)?; @@ -205,12 +199,13 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { AllocationExtra::memory_read(self, ptr, size)?; - assert_eq!(ptr.offset.bytes() as usize as u64, ptr.offset.bytes()); - assert_eq!(size.bytes() as usize as u64, size.bytes()); - let offset = ptr.offset.bytes() as usize; - Ok(&self.bytes[offset..offset + size.bytes() as usize]) + Ok(&self.bytes[range]) } + /// Check that these bytes are initialized and not pointer bytes, and then return them + /// as a slice. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes( &self, @@ -219,11 +214,13 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { size: Size, ) -> InterpResult<'tcx, &[u8]> { - self.get_bytes_internal(cx, ptr, size, true, CheckInAllocMsg::MemoryAccessTest) + self.get_bytes_internal(cx, ptr, size, true) } /// It is the caller's responsibility to handle undefined and pointer bytes. /// However, this still checks that there are no relocations on the *edges*. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes_with_undef_and_ptr( &self, @@ -232,11 +229,13 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { size: Size, ) -> InterpResult<'tcx, &[u8]> { - self.get_bytes_internal(cx, ptr, size, false, CheckInAllocMsg::MemoryAccessTest) + self.get_bytes_internal(cx, ptr, size, false) } /// Just calling this already marks everything as defined and removes relocations, /// so be sure to actually put data there! + /// + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn get_bytes_mut( &mut self, cx: &impl HasDataLayout, @@ -244,18 +243,14 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { size: Size, ) -> InterpResult<'tcx, &mut [u8]> { - assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`"); - self.check_bounds(cx, ptr, size, CheckInAllocMsg::MemoryAccessTest)?; + let range = self.check_bounds(ptr.offset, size); self.mark_definedness(ptr, size, true); self.clear_relocations(cx, ptr, size)?; AllocationExtra::memory_written(self, ptr, size)?; - assert_eq!(ptr.offset.bytes() as usize as u64, ptr.offset.bytes()); - assert_eq!(size.bytes() as usize as u64, size.bytes()); - let offset = ptr.offset.bytes() as usize; - Ok(&mut self.bytes[offset..offset + size.bytes() as usize]) + Ok(&mut self.bytes[range]) } } @@ -276,9 +271,10 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { let size_with_null = Size::from_bytes((size + 1) as u64); // Go through `get_bytes` for checks and AllocationExtra hooks. // We read the null, so we include it in the request, but we want it removed - // from the result! + // from the result, so we do subslicing. Ok(&self.get_bytes(cx, ptr, size_with_null)?[..size]) } + // This includes the case where `offset` is out-of-bounds to begin with. None => err!(UnterminatedCString(ptr.erase_tag())), } } @@ -306,7 +302,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Writes `src` to the memory starting at `ptr.offset`. /// - /// Will do bounds checks on the allocation. + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn write_bytes( &mut self, cx: &impl HasDataLayout, @@ -320,6 +316,8 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { } /// Sets `count` bytes starting at `ptr.offset` with `val`. Basically `memset`. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn write_repeat( &mut self, cx: &impl HasDataLayout, @@ -342,7 +340,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// * in oder to obtain a `Pointer` we need to check for ZSTness anyway due to integer pointers /// being valid for ZSTs /// - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn read_scalar( &self, cx: &impl HasDataLayout, @@ -378,7 +376,9 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { Ok(ScalarMaybeUndef::Scalar(Scalar::from_uint(bits, size))) } - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// Read a pointer-sized scalar. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn read_ptr_sized( &self, cx: &impl HasDataLayout, @@ -395,7 +395,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// * in oder to obtain a `Pointer` we need to check for ZSTness anyway due to integer pointers /// being valid for ZSTs /// - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn write_scalar( &mut self, cx: &impl HasDataLayout, @@ -435,7 +435,9 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { Ok(()) } - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// Write a pointer-sized scalar. + /// + /// It is the callers responsibility to check bounds and alignment beforehand. pub fn write_ptr_sized( &mut self, cx: &impl HasDataLayout, diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index 4afa4a0cbb3d7..ae994d14b1029 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -437,7 +437,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { Ok(Some((size.align_to(align), align))) } ty::Dynamic(..) => { - let vtable = metadata.expect("dyn trait fat ptr must have vtable").to_ptr()?; + let vtable = metadata.expect("dyn trait fat ptr must have vtable"); // the second entry in the vtable is the dynamic size of the object. Ok(Some(self.read_size_and_align_from_vtable(vtable)?)) } diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index de035ed779e2a..05d110a4372a1 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -250,7 +250,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { /// Checks that the pointer is aligned AND non-NULL. This supports ZSTs in two ways: /// You can pass a scalar, and a `Pointer` does not have to actually still be allocated. - pub fn check_align( + fn check_align( &self, ptr: Scalar, required_align: Align @@ -260,7 +260,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { Err(ptr) => { // check this is not NULL -- which we can ensure only if this is in-bounds // of some (potentially dead) allocation. - let align = self.check_bounds_ptr(ptr, InboundsCheck::MaybeDead, + let align = self.check_ptr_bounds(ptr, InboundsCheck::MaybeDead, CheckInAllocMsg::NullPointerTest)?; (ptr.offset.bytes(), align) } @@ -291,12 +291,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } } - /// Checks if the pointer is "in-bounds". Notice that a pointer pointing at the end - /// of an allocation (i.e., at the first *inaccessible* location) *is* considered - /// in-bounds! This follows C's/LLVM's rules. - /// If you want to check bounds before doing a memory access, better first obtain - /// an `Allocation` and call `check_bounds`. - pub fn check_bounds_ptr( + /// Checks if the pointer is "in-bounds" of *some* (live or dead) allocation. Notice that + /// a pointer pointing at the end of an allocation (i.e., at the first *inaccessible* location) + /// *is* considered in-bounds! This follows C's/LLVM's rules. + /// `liveness` can be used to rule out dead allocations. Testing in-bounds with a dead + /// allocation is useful e.g. to exclude the possibility of this pointer being NULL. + /// If you want to check bounds before doing a memory access, call `check_ptr_access`. + fn check_ptr_bounds( &self, ptr: Pointer, liveness: InboundsCheck, @@ -306,6 +307,77 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { ptr.check_in_alloc(allocation_size, msg)?; Ok(align) } + + /// Check if the given scalar is allowed to do a memory access of given `size` + /// and `align`. On success, returns `None` for zero-sized accesses (where + /// nothing else is left to do) and a `Pointer` to use for the actual access otherwise. + /// Crucially, if the input is a `Pointer`, we will test it for liveness + /// *even of* the size is 0. + /// + /// Everyone accessing memory based on a `Scalar` should use this method to get the + /// `Pointer` they need. And even if you already have a `Pointer`, call this method + /// to make sure it is sufficiently aligned and not dangling. Not doing that may + /// cause ICEs. + pub fn check_ptr_access( + &self, + sptr: Scalar, + size: Size, + align: Align, + ) -> InterpResult<'tcx, Option>> { + // Normalize to a `Pointer` if we definitely need one. + let normalized = if size.bytes() == 0 { + // Can be an integer, just take what we got. + sptr + } else { + // A "real" access, we must get a pointer. + Scalar::Ptr(self.force_ptr(sptr)?) + }; + Ok(match normalized.to_bits_or_ptr(self.pointer_size(), self) { + Ok(bits) => { + let bits = bits as u64; // it's ptr-sized + assert!(size.bytes() == 0); + // Must be non-NULL and aligned. + if bits == 0 { + return err!(InvalidNullPointerUsage); + } + if bits % align.bytes() != 0 { + let bits_pow1 = 1 << bits.trailing_zeros(); + return err!(AlignmentCheckFailed { + has: Align::from_bytes(bits_pow1).unwrap(), + required: align, + }); + } + None + } + Err(ptr) => { + // Test bounds. + self.check_ptr_bounds( + ptr.offset(size, self)?, + InboundsCheck::Live, + CheckInAllocMsg::MemoryAccessTest, + )?; + // Test align and non-NULL. + self.check_align(ptr.into(), align)?; + // FIXME: Alignment check is too strict, depending on the base address that + // got picked we might be aligned even if this check fails. + // We instead have to fall back to converting to an integer and checking + // the "real" alignment. + + // We can still be zero-sized in this branch, in which case we have to + // return `None`. + if size.bytes() == 0 { None } else { Some(ptr) } + } + }) + } + + /// Test if the pointer might be NULL. + pub fn ptr_may_be_null( + &self, + ptr: Pointer, + ) -> bool { + self.check_ptr_bounds(ptr, InboundsCheck::MaybeDead, CheckInAllocMsg::NullPointerTest) + .is_err() + } } /// Allocation accessors @@ -629,24 +701,22 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } } -/// Byte Accessors +/// Reading and writing. impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { + /// Performs appropriate bounds checks. pub fn read_bytes( &self, ptr: Scalar, size: Size, ) -> InterpResult<'tcx, &[u8]> { - if size.bytes() == 0 { - Ok(&[]) - } else { - let ptr = self.force_ptr(ptr)?; - self.get(ptr.alloc_id)?.get_bytes(self, ptr, size) - } + let ptr = match self.check_ptr_access(ptr, size, Align::from_bytes(1).unwrap())? { + Some(ptr) => ptr, + None => return Ok(&[]), // zero-sized access + }; + self.get(ptr.alloc_id)?.get_bytes(self, ptr, size) } -} -/// Reading and writing. -impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { + /// Performs appropriate bounds checks. pub fn copy( &mut self, src: Scalar, @@ -659,6 +729,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { self.copy_repeatedly(src, src_align, dest, dest_align, size, 1, nonoverlapping) } + /// Performs appropriate bounds checks. pub fn copy_repeatedly( &mut self, src: Scalar, @@ -669,15 +740,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { length: u64, nonoverlapping: bool, ) -> InterpResult<'tcx> { - self.check_align(src, src_align)?; - self.check_align(dest, dest_align)?; - if size.bytes() == 0 { - // Nothing to do for ZST, other than checking alignment and - // non-NULLness which already happened. - return Ok(()); - } - let src = self.force_ptr(src)?; - let dest = self.force_ptr(dest)?; + // We need to check *both* before early-aborting due to the size being 0. + let (src, dest) = match (self.check_ptr_access(src, size, src_align)?, + self.check_ptr_access(dest, size * length, dest_align)?) + { + (Some(src), Some(dest)) => (src, dest), + // One of the two sizes is 0. + _ => return Ok(()), + }; // first copy the relocations to a temporary buffer, because // `get_bytes_mut` will clear the relocations, which is correct, diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index ec391b7bc5f02..2ed6917e9da56 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -9,9 +9,9 @@ use rustc::ty::layout::{ }; use rustc::mir::interpret::{ - GlobalId, AllocId, CheckInAllocMsg, + GlobalId, AllocId, ConstValue, Pointer, Scalar, - InterpResult, InterpError, InboundsCheck, + InterpResult, InterpError, sign_extend, truncate, }; use super::{ @@ -226,19 +226,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { } let (ptr, ptr_align) = mplace.to_scalar_ptr_align(); - if mplace.layout.is_zst() { - // Not all ZSTs have a layout we would handle below, so just short-circuit them - // all here. - self.memory.check_align(ptr, ptr_align)?; - return Ok(Some(ImmTy { + let ptr = match self.memory.check_ptr_access(ptr, mplace.layout.size, ptr_align)? { + Some(ptr) => ptr, + None => return Ok(Some(ImmTy { imm: Immediate::Scalar(Scalar::zst().into()), layout: mplace.layout, - })); - } + })), // zero-sized access + }; - // check for integer pointers before alignment to report better errors - let ptr = self.force_ptr(ptr)?; - self.memory.check_align(ptr.into(), ptr_align)?; match mplace.layout.abi { layout::Abi::Scalar(..) => { let scalar = self.memory @@ -250,17 +245,18 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { })) } layout::Abi::ScalarPair(ref a, ref b) => { + // We checked `ptr_align` above, so all fields will have the alignment they need. + // We would anyway check against `ptr_align.restrict_for_offset(b_offset)`, + // which `ptr.offset(b_offset)` cannot possibly fail to satisfy. let (a, b) = (&a.value, &b.value); let (a_size, b_size) = (a.size(self), b.size(self)); let a_ptr = ptr; let b_offset = a_size.align_to(b.align(self).abi); - assert!(b_offset.bytes() > 0); // we later use the offset to test which field to use + assert!(b_offset.bytes() > 0); // we later use the offset to tell apart the fields let b_ptr = ptr.offset(b_offset, self)?; let a_val = self.memory .get(ptr.alloc_id)? .read_scalar(self, a_ptr, a_size)?; - let b_align = ptr_align.restrict_for_offset(b_offset); - self.memory.check_align(b_ptr.into(), b_align)?; let b_val = self.memory .get(ptr.alloc_id)? .read_scalar(self, b_ptr, b_size)?; @@ -639,8 +635,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { Err(ptr) => { // The niche must be just 0 (which an inbounds pointer value never is) let ptr_valid = niche_start == 0 && variants_start == variants_end && - self.memory.check_bounds_ptr(ptr, InboundsCheck::MaybeDead, - CheckInAllocMsg::NullPointerTest).is_ok(); + !self.memory.ptr_may_be_null(ptr); if !ptr_valid { return err!(InvalidDiscriminant(raw_discr.erase_tag().into())); } diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 1285549015cdd..1351b5bb8bd88 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -222,9 +222,9 @@ impl<'tcx, Tag> MPlaceTy<'tcx, Tag> { } #[inline] - pub(super) fn vtable(self) -> InterpResult<'tcx, Pointer> { + pub(super) fn vtable(self) -> Scalar { match self.layout.ty.sty { - ty::Dynamic(..) => self.mplace.meta.unwrap().to_ptr(), + ty::Dynamic(..) => self.mplace.meta.unwrap(), _ => bug!("vtable not supported on type {:?}", self.layout.ty), } } @@ -746,15 +746,13 @@ where // type things are read at. In case `src_val` is a `ScalarPair`, we don't do any magic here // to handle padding properly, which is only correct if we never look at this data with the // wrong type. + assert!(!dest.layout.is_unsized()); - // Nothing to do for ZSTs, other than checking alignment - if dest.layout.is_zst() { - return self.memory.check_align(ptr, ptr_align); - } + let ptr = match self.memory.check_ptr_access(ptr, dest.layout.size, ptr_align)? { + Some(ptr) => ptr, + None => return Ok(()), // zero-sized access + }; - // check for integer pointers before alignment to report better errors - let ptr = self.force_ptr(ptr)?; - self.memory.check_align(ptr.into(), ptr_align)?; let tcx = &*self.tcx; // FIXME: We should check that there are dest.layout.size many bytes available in // memory. The code below is not sufficient, with enough padding it might not @@ -771,6 +769,9 @@ where ) } Immediate::ScalarPair(a_val, b_val) => { + // We checked `ptr_align` above, so all fields will have the alignment they need. + // We would anyway check against `ptr_align.restrict_for_offset(b_offset)`, + // which `ptr.offset(b_offset)` cannot possibly fail to satisfy. let (a, b) = match dest.layout.abi { layout::Abi::ScalarPair(ref a, ref b) => (&a.value, &b.value), _ => bug!("write_immediate_to_mplace: invalid ScalarPair layout: {:#?}", @@ -778,11 +779,8 @@ where }; let (a_size, b_size) = (a.size(self), b.size(self)); let b_offset = a_size.align_to(b.align(self).abi); - let b_align = ptr_align.restrict_for_offset(b_offset); let b_ptr = ptr.offset(b_offset, self)?; - self.memory.check_align(b_ptr.into(), b_align)?; - // It is tempting to verify `b_offset` against `layout.fields.offset(1)`, // but that does not work: We could be a newtype around a pair, then the // fields do not match the `ScalarPair` components. @@ -1053,7 +1051,7 @@ where /// Also return some more information so drop doesn't have to run the same code twice. pub(super) fn unpack_dyn_trait(&self, mplace: MPlaceTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, (ty::Instance<'tcx>, MPlaceTy<'tcx, M::PointerTag>)> { - let vtable = mplace.vtable()?; // also sanity checks the type + let vtable = mplace.vtable(); // also sanity checks the type let (instance, ty) = self.read_drop_type_from_vtable(vtable)?; let layout = self.layout_of(ty)?; diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index 190f7818ddb03..af061f968104e 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -425,12 +425,15 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { } }; // Find and consult vtable - let vtable = receiver_place.vtable()?; - self.memory.check_align(vtable.into(), self.tcx.data_layout.pointer_align.abi)?; - let fn_ptr = self.memory.get(vtable.alloc_id)?.read_ptr_sized( - self, - vtable.offset(ptr_size * (idx as u64 + 3), self)?, - )?.to_ptr()?; + let vtable = receiver_place.vtable(); + let vtable_slot = vtable.ptr_offset(ptr_size * (idx as u64 + 3), self)?; + let vtable_slot = self.memory.check_ptr_access( + vtable_slot, + ptr_size, + self.tcx.data_layout.pointer_align.abi, + )?.expect("cannot be a ZST"); + let fn_ptr = self.memory.get(vtable_slot.alloc_id)? + .read_ptr_sized(self, vtable_slot)?.to_ptr()?; let instance = self.memory.get_fn(fn_ptr)?; // `*mut receiver_place.layout.ty` is almost the layout that we diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index 4ae0ee530553c..bbf063a18b695 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -101,10 +101,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { /// Returns the drop fn instance as well as the actual dynamic type pub fn read_drop_type_from_vtable( &self, - vtable: Pointer, + vtable: Scalar, ) -> InterpResult<'tcx, (ty::Instance<'tcx>, Ty<'tcx>)> { // we don't care about the pointee type, we just want a pointer - self.memory.check_align(vtable.into(), self.tcx.data_layout.pointer_align.abi)?; + let vtable = self.memory.check_ptr_access( + vtable, + self.tcx.data_layout.pointer_size, + self.tcx.data_layout.pointer_align.abi, + )?.expect("cannot be a ZST"); let drop_fn = self.memory .get(vtable.alloc_id)? .read_ptr_sized(self, vtable)? @@ -113,17 +117,23 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { trace!("Found drop fn: {:?}", drop_instance); let fn_sig = drop_instance.ty(*self.tcx).fn_sig(*self.tcx); let fn_sig = self.tcx.normalize_erasing_late_bound_regions(self.param_env, &fn_sig); - // the drop function takes *mut T where T is the type being dropped, so get that + // The drop function takes `*mut T` where `T` is the type being dropped, so get that. let ty = fn_sig.inputs()[0].builtin_deref(true).unwrap().ty; Ok((drop_instance, ty)) } pub fn read_size_and_align_from_vtable( &self, - vtable: Pointer, + vtable: Scalar, ) -> InterpResult<'tcx, (Size, Align)> { let pointer_size = self.pointer_size(); - self.memory.check_align(vtable.into(), self.tcx.data_layout.pointer_align.abi)?; + // We check for size = 3*ptr_size, that covers the drop fn (unused here), + // the size, and the align. + let vtable = self.memory.check_ptr_access( + vtable, + 3*pointer_size, + self.tcx.data_layout.pointer_align.abi, + )?.expect("cannot be a ZST"); let alloc = self.memory.get(vtable.alloc_id)?; let size = alloc.read_ptr_sized(self, vtable.offset(pointer_size, self)?)? .to_bits(pointer_size)? as u64; diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index e42c667fec9ef..57c3fe4cb3e27 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -3,11 +3,11 @@ use std::ops::RangeInclusive; use syntax_pos::symbol::{sym, Symbol}; use rustc::hir; -use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf, VariantIdx}; +use rustc::ty::layout::{self, TyLayout, LayoutOf, VariantIdx}; use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ - Scalar, GlobalAlloc, InterpResult, InterpError, CheckInAllocMsg, + GlobalAlloc, InterpResult, InterpError, }; use std::hash::Hash; @@ -365,8 +365,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> let tail = self.ecx.tcx.struct_tail(layout.ty); match tail.sty { ty::Dynamic(..) => { - let vtable = try_validation!(meta.unwrap().to_ptr(), - "non-pointer vtable in fat pointer", self.path); + let vtable = meta.unwrap(); + try_validation!(self.ecx.memory.check_ptr_access( + vtable, + 3*self.ecx.tcx.data_layout.pointer_size, // drop, size, align + self.ecx.tcx.data_layout.pointer_align.abi, + ), "dangling or unaligned vtable pointer", self.path); try_validation!(self.ecx.read_drop_type_from_vtable(vtable), "invalid drop fn in vtable", self.path); try_validation!(self.ecx.read_size_and_align_from_vtable(vtable), @@ -384,14 +388,14 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> bug!("Unexpected unsized type tail: {:?}", tail), } } - // Make sure this is non-NULL and aligned + // Make sure this is dereferencable and all. let (size, align) = self.ecx.size_and_align_of(meta, layout)? // for the purpose of validity, consider foreign types to have // alignment and size determined by the layout (size will be 0, // alignment should take attributes into account). .unwrap_or_else(|| (layout.size, layout.align.abi)); - match self.ecx.memory.check_align(ptr, align) { - Ok(_) => {}, + let ptr: Option<_> = match self.ecx.memory.check_ptr_access(ptr, size, align) { + Ok(ptr) => ptr, Err(err) => { info!("{:?} is not aligned to {:?}", ptr, align); match err.kind { @@ -403,21 +407,16 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> required.bytes(), has.bytes()), self.path), _ => return validation_failure!( - "dangling (out-of-bounds) reference (might be NULL at \ - run-time)", + "dangling (not entirely in bounds) reference", self.path ), } } - } + }; // Recursive checking if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts { let place = self.ecx.ref_to_mplace(value)?; - // FIXME(RalfJ): check ZST for inbound pointers - if size != Size::ZERO { - // Non-ZST also have to be dereferencable - let ptr = try_validation!(place.ptr.to_ptr(), - "integer pointer in non-ZST reference", self.path); + if let Some(ptr) = ptr { // not a ZST // Skip validation entirely for some external statics let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id); if let Some(GlobalAlloc::Static(did)) = alloc_kind { @@ -429,18 +428,10 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> return Ok(()); } } - // Maintain the invariant that the place we are checking is - // already verified to be in-bounds. - try_validation!( - self.ecx.memory - .get(ptr.alloc_id)? - .check_bounds(self.ecx, ptr, size, CheckInAllocMsg::InboundsTest), - "dangling (not entirely in bounds) reference", self.path); } // Check if we have encountered this pointer+layout combination - // before. Proceed recursively even for integer pointers, no - // reason to skip them! They are (recursively) valid for some ZST, - // but not for others (e.g., `!` is a ZST). + // before. Proceed recursively even for ZST, no + // reason to skip them! E.g., `!` is a ZST and we want to validate it. let path = &self.path; ref_tracking.track(place, || { // We need to clone the path anyway, make sure it gets created @@ -499,14 +490,8 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> let bits = match value.to_bits_or_ptr(op.layout.size, self.ecx) { Err(ptr) => { if lo == 1 && hi == max_hi { - // only NULL is not allowed. - // We can call `check_align` to check non-NULL-ness, but have to also look - // for function pointers. - let non_null = - self.ecx.memory.check_align( - Scalar::Ptr(ptr), Align::from_bytes(1).unwrap() - ).is_ok(); - if !non_null { + // Only NULL is the niche. So make sure the ptr is NOT NULL. + if self.ecx.memory.ptr_may_be_null(ptr) { // These conditions are just here to improve the diagnostics so we can // differentiate between null pointers and dangling pointers if self.ref_tracking_for_consts.is_some() && From dcc8371d4e0eb2bf3151c6bba71dc3b23fd27688 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 16:31:16 +0200 Subject: [PATCH 2/8] move CheckInAllocMsg to more logical place --- src/librustc/mir/interpret/allocation.rs | 22 ---------------------- src/librustc/mir/interpret/mod.rs | 4 ++-- src/librustc/mir/interpret/pointer.rs | 24 +++++++++++++++++++++++- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/librustc/mir/interpret/allocation.rs b/src/librustc/mir/interpret/allocation.rs index 964929547b77b..b7abf89e61c95 100644 --- a/src/librustc/mir/interpret/allocation.rs +++ b/src/librustc/mir/interpret/allocation.rs @@ -22,28 +22,6 @@ pub enum InboundsCheck { MaybeDead, } -/// Used by `check_in_alloc` to indicate context of check -#[derive(Debug, Copy, Clone, RustcEncodable, RustcDecodable, HashStable)] -pub enum CheckInAllocMsg { - MemoryAccessTest, - NullPointerTest, - PointerArithmeticTest, - InboundsTest, -} - -impl Display for CheckInAllocMsg { - /// When this is printed as an error the context looks like this - /// "{test name} failed: pointer must be in-bounds at offset..." - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", match *self { - CheckInAllocMsg::MemoryAccessTest => "Memory access", - CheckInAllocMsg::NullPointerTest => "Null pointer test", - CheckInAllocMsg::PointerArithmeticTest => "Pointer arithmetic", - CheckInAllocMsg::InboundsTest => "Inbounds test", - }) - } -} - #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)] pub struct Allocation { /// The actual bytes of the allocation. diff --git a/src/librustc/mir/interpret/mod.rs b/src/librustc/mir/interpret/mod.rs index a36c788022955..4b1a69404dde6 100644 --- a/src/librustc/mir/interpret/mod.rs +++ b/src/librustc/mir/interpret/mod.rs @@ -19,10 +19,10 @@ pub use self::value::{Scalar, ScalarMaybeUndef, RawConst, ConstValue}; pub use self::allocation::{ InboundsCheck, Allocation, AllocationExtra, - Relocations, UndefMask, CheckInAllocMsg, + Relocations, UndefMask, }; -pub use self::pointer::{Pointer, PointerArithmetic}; +pub use self::pointer::{Pointer, PointerArithmetic, CheckInAllocMsg}; use std::fmt; use crate::mir; diff --git a/src/librustc/mir/interpret/pointer.rs b/src/librustc/mir/interpret/pointer.rs index 26002a411d4b6..f5c5469fe1b2d 100644 --- a/src/librustc/mir/interpret/pointer.rs +++ b/src/librustc/mir/interpret/pointer.rs @@ -5,9 +5,31 @@ use crate::ty::layout::{self, HasDataLayout, Size}; use rustc_macros::HashStable; use super::{ - AllocId, InterpResult, CheckInAllocMsg + AllocId, InterpResult, }; +/// Used by `check_in_alloc` to indicate context of check +#[derive(Debug, Copy, Clone, RustcEncodable, RustcDecodable, HashStable)] +pub enum CheckInAllocMsg { + MemoryAccessTest, + NullPointerTest, + PointerArithmeticTest, + InboundsTest, +} + +impl Display for CheckInAllocMsg { + /// When this is printed as an error the context looks like this + /// "{test name} failed: pointer must be in-bounds at offset..." + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", match *self { + CheckInAllocMsg::MemoryAccessTest => "Memory access", + CheckInAllocMsg::NullPointerTest => "Null pointer test", + CheckInAllocMsg::PointerArithmeticTest => "Pointer arithmetic", + CheckInAllocMsg::InboundsTest => "Inbounds test", + }) + } +} + //////////////////////////////////////////////////////////////////////////////// // Pointer arithmetic //////////////////////////////////////////////////////////////////////////////// From c12c8a78eaa0e2fccadbffde971fa9a39f7e7b42 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 16:42:51 +0200 Subject: [PATCH 3/8] clean up internals of pointer checks; make get_size_and_align also check for fn allocations --- src/librustc/mir/interpret/allocation.rs | 11 +- src/librustc/mir/interpret/mod.rs | 5 +- src/librustc/mir/interpret/pointer.rs | 2 +- src/librustc_mir/interpret/memory.rs | 153 ++++++++++------------- src/librustc_mir/interpret/mod.rs | 2 +- 5 files changed, 69 insertions(+), 104 deletions(-) diff --git a/src/librustc/mir/interpret/allocation.rs b/src/librustc/mir/interpret/allocation.rs index b7abf89e61c95..49ecfec74bb39 100644 --- a/src/librustc/mir/interpret/allocation.rs +++ b/src/librustc/mir/interpret/allocation.rs @@ -6,22 +6,13 @@ use super::{ use crate::ty::layout::{Size, Align}; use syntax::ast::Mutability; -use std::{iter, fmt::{self, Display}}; +use std::iter; use crate::mir; use std::ops::{Range, Deref, DerefMut}; use rustc_data_structures::sorted_map::SortedMap; -use rustc_macros::HashStable; use rustc_target::abi::HasDataLayout; use std::borrow::Cow; -/// Used by `check_bounds` to indicate whether the pointer needs to be just inbounds -/// or also inbounds of a *live* allocation. -#[derive(Debug, Copy, Clone, RustcEncodable, RustcDecodable, HashStable)] -pub enum InboundsCheck { - Live, - MaybeDead, -} - #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)] pub struct Allocation { /// The actual bytes of the allocation. diff --git a/src/librustc/mir/interpret/mod.rs b/src/librustc/mir/interpret/mod.rs index 4b1a69404dde6..1b294250aa3dc 100644 --- a/src/librustc/mir/interpret/mod.rs +++ b/src/librustc/mir/interpret/mod.rs @@ -17,10 +17,7 @@ pub use self::error::{ pub use self::value::{Scalar, ScalarMaybeUndef, RawConst, ConstValue}; -pub use self::allocation::{ - InboundsCheck, Allocation, AllocationExtra, - Relocations, UndefMask, -}; +pub use self::allocation::{Allocation, AllocationExtra, Relocations, UndefMask}; pub use self::pointer::{Pointer, PointerArithmetic, CheckInAllocMsg}; diff --git a/src/librustc/mir/interpret/pointer.rs b/src/librustc/mir/interpret/pointer.rs index f5c5469fe1b2d..a17bc1f67283d 100644 --- a/src/librustc/mir/interpret/pointer.rs +++ b/src/librustc/mir/interpret/pointer.rs @@ -1,4 +1,4 @@ -use std::fmt; +use std::fmt::{self, Display}; use crate::mir; use crate::ty::layout::{self, HasDataLayout, Size}; diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 05d110a4372a1..197e067f674ff 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -19,7 +19,7 @@ use syntax::ast::Mutability; use super::{ Pointer, AllocId, Allocation, GlobalId, AllocationExtra, InterpResult, Scalar, InterpError, GlobalAlloc, PointerArithmetic, - Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, InboundsCheck, + Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, InterpError::ValidationFailure, }; @@ -44,6 +44,17 @@ impl MayLeak for MemoryKind { } } +/// Used by `get_size_and_align` to indicate whether the allocation needs to be live. +#[derive(Debug, Copy, Clone)] +pub enum AllocCheck { + /// Allocation must be live and not a function pointer. + Dereferencable, + /// Allocations needs to be live, but may be a function pointer. + Live, + /// Allocation may be dead. + MaybeDead, +} + // `Memory` has to depend on the `Machine` because some of its operations // (e.g., `get`) call a `Machine` hook. pub struct Memory<'mir, 'tcx, M: Machine<'mir, 'tcx>> { @@ -248,66 +259,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { Ok(()) } - /// Checks that the pointer is aligned AND non-NULL. This supports ZSTs in two ways: - /// You can pass a scalar, and a `Pointer` does not have to actually still be allocated. - fn check_align( - &self, - ptr: Scalar, - required_align: Align - ) -> InterpResult<'tcx> { - // Check non-NULL/Undef, extract offset - let (offset, alloc_align) = match ptr.to_bits_or_ptr(self.pointer_size(), self) { - Err(ptr) => { - // check this is not NULL -- which we can ensure only if this is in-bounds - // of some (potentially dead) allocation. - let align = self.check_ptr_bounds(ptr, InboundsCheck::MaybeDead, - CheckInAllocMsg::NullPointerTest)?; - (ptr.offset.bytes(), align) - } - Ok(data) => { - // check this is not NULL - if data == 0 { - return err!(InvalidNullPointerUsage); - } - // the "base address" is 0 and hence always aligned - (data as u64, required_align) - } - }; - // Check alignment - if alloc_align.bytes() < required_align.bytes() { - return err!(AlignmentCheckFailed { - has: alloc_align, - required: required_align, - }); - } - if offset % required_align.bytes() == 0 { - Ok(()) - } else { - let has = offset % required_align.bytes(); - err!(AlignmentCheckFailed { - has: Align::from_bytes(has).unwrap(), - required: required_align, - }) - } - } - - /// Checks if the pointer is "in-bounds" of *some* (live or dead) allocation. Notice that - /// a pointer pointing at the end of an allocation (i.e., at the first *inaccessible* location) - /// *is* considered in-bounds! This follows C's/LLVM's rules. - /// `liveness` can be used to rule out dead allocations. Testing in-bounds with a dead - /// allocation is useful e.g. to exclude the possibility of this pointer being NULL. - /// If you want to check bounds before doing a memory access, call `check_ptr_access`. - fn check_ptr_bounds( - &self, - ptr: Pointer, - liveness: InboundsCheck, - msg: CheckInAllocMsg, - ) -> InterpResult<'tcx, Align> { - let (allocation_size, align) = self.get_size_and_align(ptr.alloc_id, liveness)?; - ptr.check_in_alloc(allocation_size, msg)?; - Ok(align) - } - /// Check if the given scalar is allowed to do a memory access of given `size` /// and `align`. On success, returns `None` for zero-sized accesses (where /// nothing else is left to do) and a `Pointer` to use for the actual access otherwise. @@ -350,18 +301,35 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { None } Err(ptr) => { - // Test bounds. - self.check_ptr_bounds( - ptr.offset(size, self)?, - InboundsCheck::Live, - CheckInAllocMsg::MemoryAccessTest, - )?; - // Test align and non-NULL. - self.check_align(ptr.into(), align)?; - // FIXME: Alignment check is too strict, depending on the base address that - // got picked we might be aligned even if this check fails. - // We instead have to fall back to converting to an integer and checking - // the "real" alignment. + let (allocation_size, alloc_align) = + self.get_size_and_align(ptr.alloc_id, AllocCheck::Dereferencable)?; + // Test bounds. This also ensures non-NULL. + // 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)?; + // Test align. Check this last; if both bounds and alignment are violated + // we want the error to be about the bounds. + if alloc_align.bytes() < align.bytes() { + // The allocation itself is not aligned enough. + // FIXME: Alignment check is too strict, depending on the base address that + // got picked we might be aligned even if this check fails. + // We instead have to fall back to converting to an integer and checking + // the "real" alignment. + return err!(AlignmentCheckFailed { + has: alloc_align, + required: align, + }); + } + let offset = ptr.offset.bytes(); + if offset % align.bytes() != 0 { + // The offset os not aligned enough. + let has = offset % align.bytes(); + return err!(AlignmentCheckFailed { + has: Align::from_bytes(has).unwrap(), + required: align, + }) + } // We can still be zero-sized in this branch, in which case we have to // return `None`. @@ -375,8 +343,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { &self, ptr: Pointer, ) -> bool { - self.check_ptr_bounds(ptr, InboundsCheck::MaybeDead, CheckInAllocMsg::NullPointerTest) - .is_err() + 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() } } @@ -515,13 +484,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } } - /// Obtain the size and alignment of an allocation, even if that allocation has been deallocated + /// Obtain the size and alignment of an allocation, even if that allocation has + /// been deallocated. /// - /// If `liveness` is `InboundsCheck::MaybeDead`, this function always returns `Ok` + /// If `liveness` is `AllocCheck::MaybeDead`, this function always returns `Ok`. pub fn get_size_and_align( &self, id: AllocId, - liveness: InboundsCheck, + liveness: AllocCheck, ) -> InterpResult<'static, (Size, Align)> { if let Ok(alloc) = self.get(id) { return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); @@ -531,7 +501,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { let alloc = self.tcx.alloc_map.lock().get(id); // Could also be a fn ptr or extern static match alloc { - Some(GlobalAlloc::Function(..)) => Ok((Size::ZERO, Align::from_bytes(1).unwrap())), + Some(GlobalAlloc::Function(..)) => { + if let AllocCheck::Dereferencable = liveness { + // The caller requested no function pointers. + err!(DerefFunctionPointer) + } else { + Ok((Size::ZERO, Align::from_bytes(1).unwrap())) + } + } // `self.get` would also work, but can cause cycles if a static refers to itself Some(GlobalAlloc::Static(did)) => { // The only way `get` couldn't have worked here is if this is an extern static @@ -545,15 +522,15 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { if let Ok(alloc) = self.get(id) { return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); } - match liveness { - InboundsCheck::MaybeDead => { - // Must be a deallocated pointer - self.dead_alloc_map.get(&id).cloned().ok_or_else(|| - ValidationFailure("allocation missing in dead_alloc_map".to_string()) - .into() - ) - }, - InboundsCheck::Live => err!(DanglingPointerDeref), + if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + self.dead_alloc_map.get(&id).copied().ok_or_else(|| + ValidationFailure("allocation missing in dead_alloc_map".to_string()) + .into() + ) + } else { + err!(DanglingPointerDeref) } }, } diff --git a/src/librustc_mir/interpret/mod.rs b/src/librustc_mir/interpret/mod.rs index 0293a8366d083..259bd6af0d5d4 100644 --- a/src/librustc_mir/interpret/mod.rs +++ b/src/librustc_mir/interpret/mod.rs @@ -24,7 +24,7 @@ pub use self::eval_context::{ pub use self::place::{Place, PlaceTy, MemPlace, MPlaceTy}; -pub use self::memory::{Memory, MemoryKind}; +pub use self::memory::{Memory, MemoryKind, AllocCheck}; pub use self::machine::{Machine, AllocMap, MayLeak}; From 9c32ede0992b360398241a3c794df09fd1cc24e1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 18:00:07 +0200 Subject: [PATCH 4/8] comment tweaks, better validation errors, update UI tests --- src/librustc_mir/interpret/memory.rs | 28 +++++++++---------- src/librustc_mir/interpret/operand.rs | 4 +-- src/librustc_mir/interpret/validity.rs | 24 ++++++++++++---- .../consts/const-eval/union-ub-fat-ptr.stderr | 6 ++-- 4 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 197e067f674ff..f9ab661ea87f9 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -20,7 +20,6 @@ use super::{ Pointer, AllocId, Allocation, GlobalId, AllocationExtra, InterpResult, Scalar, InterpError, GlobalAlloc, PointerArithmetic, Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, - InterpError::ValidationFailure, }; #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] @@ -260,13 +259,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } /// Check if the given scalar is allowed to do a memory access of given `size` - /// and `align`. On success, returns `None` for zero-sized accesses (where + /// and `align`. On success, returns `None` for zero-sized accesses (where /// nothing else is left to do) and a `Pointer` to use for the actual access otherwise. /// Crucially, if the input is a `Pointer`, we will test it for liveness /// *even of* the size is 0. /// /// Everyone accessing memory based on a `Scalar` should use this method to get the - /// `Pointer` they need. And even if you already have a `Pointer`, call this method + /// `Pointer` they need. And even if you already have a `Pointer`, call this method /// to make sure it is sufficiently aligned and not dangling. Not doing that may /// cause ICEs. pub fn check_ptr_access( @@ -292,9 +291,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { return err!(InvalidNullPointerUsage); } if bits % align.bytes() != 0 { - let bits_pow1 = 1 << bits.trailing_zeros(); + // The biggest power of two through which `bits` is divisible. + let bits_pow2 = 1 << bits.trailing_zeros(); return err!(AlignmentCheckFailed { - has: Align::from_bytes(bits_pow1).unwrap(), + has: Align::from_bytes(bits_pow2).unwrap(), required: align, }); } @@ -308,7 +308,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // checks for overflow. let end_ptr = ptr.offset(size, self)?; end_ptr.check_in_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?; - // Test align. Check this last; if both bounds and alignment are violated + // Test align. Check this last; if both bounds and alignment are violated // we want the error to be about the bounds. if alloc_align.bytes() < align.bytes() { // The allocation itself is not aligned enough. @@ -323,10 +323,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } let offset = ptr.offset.bytes(); if offset % align.bytes() != 0 { - // The offset os not aligned enough. - let has = offset % align.bytes(); + // The biggest power of two through which `offset` is divisible. + let bits_pow2 = 1 << offset.trailing_zeros(); return err!(AlignmentCheckFailed { - has: Align::from_bytes(has).unwrap(), + has: Align::from_bytes(bits_pow2).unwrap(), required: align, }) } @@ -520,15 +520,13 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { } _ => { if let Ok(alloc) = self.get(id) { - return Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)); + Ok((Size::from_bytes(alloc.bytes.len() as u64), alloc.align)) } - if let AllocCheck::MaybeDead = liveness { + else if let AllocCheck::MaybeDead = liveness { // Deallocated pointers are allowed, we should be able to find // them in the map. - self.dead_alloc_map.get(&id).copied().ok_or_else(|| - ValidationFailure("allocation missing in dead_alloc_map".to_string()) - .into() - ) + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) } else { err!(DanglingPointerDeref) } diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 2ed6917e9da56..c72078fa89cd2 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -228,10 +228,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { let ptr = match self.memory.check_ptr_access(ptr, mplace.layout.size, ptr_align)? { Some(ptr) => ptr, - None => return Ok(Some(ImmTy { + None => return Ok(Some(ImmTy { // zero-sized type imm: Immediate::Scalar(Scalar::zst().into()), layout: mplace.layout, - })), // zero-sized access + })), }; match mplace.layout.abi { diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 57c3fe4cb3e27..b2a159fef59c6 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -366,11 +366,15 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> match tail.sty { ty::Dynamic(..) => { let vtable = meta.unwrap(); - try_validation!(self.ecx.memory.check_ptr_access( - vtable, - 3*self.ecx.tcx.data_layout.pointer_size, // drop, size, align - self.ecx.tcx.data_layout.pointer_align.abi, - ), "dangling or unaligned vtable pointer", self.path); + try_validation!( + self.ecx.memory.check_ptr_access( + vtable, + 3*self.ecx.tcx.data_layout.pointer_size, // drop, size, align + self.ecx.tcx.data_layout.pointer_align.abi, + ), + "dangling or unaligned vtable pointer or too small vtable", + self.path + ); try_validation!(self.ecx.read_drop_type_from_vtable(vtable), "invalid drop fn in vtable", self.path); try_validation!(self.ecx.read_size_and_align_from_vtable(vtable), @@ -397,7 +401,10 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> let ptr: Option<_> = match self.ecx.memory.check_ptr_access(ptr, size, align) { Ok(ptr) => ptr, Err(err) => { - info!("{:?} is not aligned to {:?}", ptr, align); + info!( + "{:?} did not pass access check for size {:?}, align {:?}", + ptr, size, align + ); match err.kind { InterpError::InvalidNullPointerUsage => return validation_failure!("NULL reference", self.path), @@ -405,6 +412,11 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> return validation_failure!(format!("unaligned reference \ (required {} byte alignment but found {})", required.bytes(), has.bytes()), self.path), + InterpError::ReadBytesAsPointer => + return validation_failure!( + "integer pointer in non-ZST reference", + self.path + ), _ => return validation_failure!( "dangling (not entirely in bounds) reference", diff --git a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr index 5048a97d19514..19db90c1cb53e 100644 --- a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr +++ b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr @@ -42,7 +42,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/union-ub-fat-ptr.rs:97:1 | LL | const D: &dyn Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer or too small vtable | = 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 @@ -50,7 +50,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/union-ub-fat-ptr.rs:100:1 | LL | const E: &dyn Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer or too small vtable | = 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 @@ -58,7 +58,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/union-ub-fat-ptr.rs:103:1 | LL | const F: &dyn Trait = unsafe { DynTransmute { bad: BadDynRepr { ptr: &92, vtable: 3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-pointer vtable in fat pointer + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling or unaligned vtable pointer or too small vtable | = 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 From 9492038ae9fe1c559d890bd0f41dfe5f37030a03 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 18:06:11 +0200 Subject: [PATCH 5/8] make code more symmetric --- src/librustc_mir/interpret/traits.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/traits.rs b/src/librustc_mir/interpret/traits.rs index bbf063a18b695..5d2f268d26639 100644 --- a/src/librustc_mir/interpret/traits.rs +++ b/src/librustc_mir/interpret/traits.rs @@ -128,15 +128,17 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpretCx<'mir, 'tcx, M> { ) -> InterpResult<'tcx, (Size, Align)> { let pointer_size = self.pointer_size(); // We check for size = 3*ptr_size, that covers the drop fn (unused here), - // the size, and the align. + // the size, and the align (which we read below). let vtable = self.memory.check_ptr_access( vtable, 3*pointer_size, self.tcx.data_layout.pointer_align.abi, )?.expect("cannot be a ZST"); let alloc = self.memory.get(vtable.alloc_id)?; - let size = alloc.read_ptr_sized(self, vtable.offset(pointer_size, self)?)? - .to_bits(pointer_size)? as u64; + let size = alloc.read_ptr_sized( + self, + vtable.offset(pointer_size, self)? + )?.to_bits(pointer_size)? as u64; let align = alloc.read_ptr_sized( self, vtable.offset(pointer_size * 2, self)?, From 9954af4a9f76bd324a731b61feffb684a5572ce8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 20:08:55 +0200 Subject: [PATCH 6/8] deduplicate some code --- src/librustc_mir/interpret/memory.rs | 32 +++++++++++++--------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index f9ab661ea87f9..8b72c367e9f05 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -274,6 +274,19 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { size: Size, align: Align, ) -> InterpResult<'tcx, Option>> { + fn check_offset_align(offset: u64, align: Align) -> InterpResult<'static> { + if offset % align.bytes() == 0 { + Ok(()) + } else { + // The biggest power of two through which `offset` is divisible. + let offset_pow2 = 1 << offset.trailing_zeros(); + err!(AlignmentCheckFailed { + has: Align::from_bytes(offset_pow2).unwrap(), + required: align, + }) + } + } + // Normalize to a `Pointer` if we definitely need one. let normalized = if size.bytes() == 0 { // Can be an integer, just take what we got. @@ -290,14 +303,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { if bits == 0 { return err!(InvalidNullPointerUsage); } - if bits % align.bytes() != 0 { - // The biggest power of two through which `bits` is divisible. - let bits_pow2 = 1 << bits.trailing_zeros(); - return err!(AlignmentCheckFailed { - has: Align::from_bytes(bits_pow2).unwrap(), - required: align, - }); - } + check_offset_align(bits, align)?; None } Err(ptr) => { @@ -321,15 +327,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { required: align, }); } - let offset = ptr.offset.bytes(); - if offset % align.bytes() != 0 { - // The biggest power of two through which `offset` is divisible. - let bits_pow2 = 1 << offset.trailing_zeros(); - return err!(AlignmentCheckFailed { - has: Align::from_bytes(bits_pow2).unwrap(), - required: align, - }) - } + check_offset_align(ptr.offset.bytes(), align)?; // We can still be zero-sized in this branch, in which case we have to // return `None`. From 38104abf63fe70b63fe9d26de8ed3a8cab56f7b7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 23 Jun 2019 20:18:30 +0200 Subject: [PATCH 7/8] expand comment --- src/librustc_mir/interpret/memory.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 8b72c367e9f05..c3eec677a4850 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -289,7 +289,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { // Normalize to a `Pointer` if we definitely need one. let normalized = if size.bytes() == 0 { - // Can be an integer, just take what we got. + // Can be an integer, just take what we got. We do NOT `force_bits` here; + // if this is already a `Pointer` we want to do the bounds checks! sptr } else { // A "real" access, we must get a pointer. From 7e830286c7a0c19e57d1a09e9a0bde7933f68bb6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 24 Jun 2019 14:28:16 +0200 Subject: [PATCH 8/8] fix reoccurring typo --- src/librustc/mir/interpret/allocation.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/librustc/mir/interpret/allocation.rs b/src/librustc/mir/interpret/allocation.rs index 49ecfec74bb39..c8bf250d02b36 100644 --- a/src/librustc/mir/interpret/allocation.rs +++ b/src/librustc/mir/interpret/allocation.rs @@ -147,7 +147,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// even when new allocations are pushed to the `HashMap`. `copy_repeatedly` relies /// on that. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. fn get_bytes_internal( &self, cx: &impl HasDataLayout, @@ -174,7 +174,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Check that these bytes are initialized and not pointer bytes, and then return them /// as a slice. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes( &self, @@ -189,7 +189,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// It is the caller's responsibility to handle undefined and pointer bytes. /// However, this still checks that there are no relocations on the *edges*. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes_with_undef_and_ptr( &self, @@ -204,7 +204,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Just calling this already marks everything as defined and removes relocations, /// so be sure to actually put data there! /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn get_bytes_mut( &mut self, cx: &impl HasDataLayout, @@ -271,7 +271,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Writes `src` to the memory starting at `ptr.offset`. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_bytes( &mut self, cx: &impl HasDataLayout, @@ -286,7 +286,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Sets `count` bytes starting at `ptr.offset` with `val`. Basically `memset`. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_repeat( &mut self, cx: &impl HasDataLayout, @@ -309,7 +309,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// * in oder to obtain a `Pointer` we need to check for ZSTness anyway due to integer pointers /// being valid for ZSTs /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn read_scalar( &self, cx: &impl HasDataLayout, @@ -347,7 +347,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Read a pointer-sized scalar. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn read_ptr_sized( &self, cx: &impl HasDataLayout, @@ -364,7 +364,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// * in oder to obtain a `Pointer` we need to check for ZSTness anyway due to integer pointers /// being valid for ZSTs /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_scalar( &mut self, cx: &impl HasDataLayout, @@ -406,7 +406,7 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { /// Write a pointer-sized scalar. /// - /// It is the callers responsibility to check bounds and alignment beforehand. + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_ptr_sized( &mut self, cx: &impl HasDataLayout,