diff --git a/src/librustc/mir/interpret/allocation.rs b/src/librustc/mir/interpret/allocation.rs index 8381125798a0d..c8bf250d02b36 100644 --- a/src/librustc/mir/interpret/allocation.rs +++ b/src/librustc/mir/interpret/allocation.rs @@ -6,44 +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::{Deref, DerefMut}; +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, -} - -/// 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. @@ -146,37 +115,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( - &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) - } - - /// Checks if the memory range beginning at `ptr` and of size `Size` is "in-bounds". - #[inline(always)] - pub fn check_bounds( +/// 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, - 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) + 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 } -} -/// 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 +146,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 caller's 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 +168,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 caller's responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes( &self, @@ -219,11 +183,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 caller's responsibility to check bounds and alignment beforehand. #[inline] pub fn get_bytes_with_undef_and_ptr( &self, @@ -232,11 +198,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 caller's responsibility to check bounds and alignment beforehand. pub fn get_bytes_mut( &mut self, cx: &impl HasDataLayout, @@ -244,18 +212,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 +240,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 +271,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 caller's responsibility to check bounds and alignment beforehand. pub fn write_bytes( &mut self, cx: &impl HasDataLayout, @@ -320,6 +285,8 @@ impl<'tcx, Tag: Copy, Extra: AllocationExtra> Allocation { } /// Sets `count` bytes starting at `ptr.offset` with `val`. Basically `memset`. + /// + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_repeat( &mut self, cx: &impl HasDataLayout, @@ -342,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 /// - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn read_scalar( &self, cx: &impl HasDataLayout, @@ -378,7 +345,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 caller's responsibility to check bounds and alignment beforehand. pub fn read_ptr_sized( &self, cx: &impl HasDataLayout, @@ -395,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 /// - /// Note: This function does not do *any* alignment checks, you need to do these before calling + /// It is the caller's responsibility to check bounds and alignment beforehand. pub fn write_scalar( &mut self, cx: &impl HasDataLayout, @@ -435,7 +404,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 caller's responsibility to check bounds and alignment beforehand. pub fn write_ptr_sized( &mut self, cx: &impl HasDataLayout, diff --git a/src/librustc/mir/interpret/mod.rs b/src/librustc/mir/interpret/mod.rs index a36c788022955..1b294250aa3dc 100644 --- a/src/librustc/mir/interpret/mod.rs +++ b/src/librustc/mir/interpret/mod.rs @@ -17,12 +17,9 @@ pub use self::error::{ pub use self::value::{Scalar, ScalarMaybeUndef, RawConst, ConstValue}; -pub use self::allocation::{ - InboundsCheck, Allocation, AllocationExtra, - Relocations, UndefMask, CheckInAllocMsg, -}; +pub use self::allocation::{Allocation, AllocationExtra, 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..a17bc1f67283d 100644 --- a/src/librustc/mir/interpret/pointer.rs +++ b/src/librustc/mir/interpret/pointer.rs @@ -1,13 +1,35 @@ -use std::fmt; +use std::fmt::{self, Display}; use crate::mir; 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 //////////////////////////////////////////////////////////////////////////////// diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index fbba8d10326a9..c6e762bddd4d9 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -442,7 +442,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..c3eec677a4850 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -19,8 +19,7 @@ use syntax::ast::Mutability; use super::{ Pointer, AllocId, Allocation, GlobalId, AllocationExtra, InterpResult, Scalar, InterpError, GlobalAlloc, PointerArithmetic, - Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, InboundsCheck, - InterpError::ValidationFailure, + Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, }; #[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)] @@ -44,6 +43,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,63 +258,93 @@ 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. - pub fn check_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, - 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_bounds_ptr(ptr, InboundsCheck::MaybeDead, - CheckInAllocMsg::NullPointerTest)?; - (ptr.offset.bytes(), align) + sptr: Scalar, + 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, + }) } - Ok(data) => { - // check this is not NULL - if data == 0 { + } + + // Normalize to a `Pointer` if we definitely need one. + let normalized = if size.bytes() == 0 { + // 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. + 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); } - // the "base address" is 0 and hence always aligned - (data as u64, required_align) + check_offset_align(bits, align)?; + None } - }; - // 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, - }) - } + Err(ptr) => { + 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, + }); + } + check_offset_align(ptr.offset.bytes(), align)?; + + // 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) } + } + }) } - /// 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( + /// Test if the pointer might be NULL. + pub fn ptr_may_be_null( &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) + ) -> 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() } } @@ -443,13 +483,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)); @@ -459,7 +500,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 @@ -471,17 +519,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)); + 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), + else if let AllocCheck::MaybeDead = liveness { + // Deallocated pointers are allowed, we should be able to find + // them in the map. + Ok(*self.dead_alloc_map.get(&id) + .expect("deallocated pointers should all be recorded in `dead_alloc_map`")) + } else { + err!(DanglingPointerDeref) } }, } @@ -629,24 +675,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 +703,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 +714,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/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}; diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index ec391b7bc5f02..c72078fa89cd2 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 { // zero-sized type imm: Immediate::Scalar(Scalar::zst().into()), layout: mplace.layout, - })); - } + })), + }; - // 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..5d2f268d26639 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,20 +117,28 @@ 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 (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)?, diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index e42c667fec9ef..b2a159fef59c6 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,16 @@ 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 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), @@ -384,16 +392,19 @@ 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); + info!( + "{:?} did not pass access check for size {:?}, align {:?}", + ptr, size, align + ); match err.kind { InterpError::InvalidNullPointerUsage => return validation_failure!("NULL reference", self.path), @@ -401,23 +412,23 @@ 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 (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 +440,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 +502,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() && 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