do not accept out-of-bounds pointers in enum discriminants, they might be NULL

Author: RalfJung

src/librustc/ich/impls_ty.rs

@@ -451,7 +451,7 @@ impl_stable_hash_for!(
         FunctionRetMismatch(a, b),
         NoMirFor(s),
         UnterminatedCString(ptr),
-        PointerOutOfBounds { ptr, access, allocation_size },
+        PointerOutOfBounds { ptr, check, allocation_size },
         InvalidBoolOp(bop),
         Unimplemented(s),
         BoundsCheck { len, index },
@@ -471,6 +471,11 @@ impl_stable_hash_for!(
     }
 );
 
+impl_stable_hash_for!(enum mir::interpret::InboundsCheck {
+    Live,
+    MaybeDead
+});
+
 impl_stable_hash_for!(enum mir::interpret::Lock {
     NoLock,
     WriteLock(dl),

src/librustc/mir/interpret/allocation.rs

@@ -19,6 +19,14 @@ use mir;
 use std::ops::{Deref, DerefMut};
 use rustc_data_structures::sorted_map::SortedMap;
 
+/// 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)]
+pub enum InboundsCheck {
+    Live,
+    MaybeDead,
+}
+
 #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub struct Allocation<Tag=(),Extra=()> {
     /// The actual bytes of the allocation.

src/librustc/mir/interpret/error.rs

@@ -15,7 +15,7 @@ use ty::{Ty, layout};
 use ty::layout::{Size, Align, LayoutError};
 use rustc_target::spec::abi::Abi;
 
-use super::{Pointer, ScalarMaybeUndef};
+use super::{Pointer, InboundsCheck, ScalarMaybeUndef};
 
 use backtrace::Backtrace;
 
@@ -243,7 +243,7 @@ pub enum EvalErrorKind<'tcx, O> {
     InvalidDiscriminant(ScalarMaybeUndef),
     PointerOutOfBounds {
         ptr: Pointer,
-        access: bool,
+        check: InboundsCheck,
         allocation_size: Size,
     },
     InvalidNullPointerUsage,
@@ -457,9 +457,13 @@ impl<'tcx, O: fmt::Debug> fmt::Debug for EvalErrorKind<'tcx, O> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         use self::EvalErrorKind::*;
         match *self {
-            PointerOutOfBounds { ptr, access, allocation_size } => {
-                write!(f, "{} at offset {}, outside bounds of allocation {} which has size {}",
-                       if access { "memory access" } else { "pointer computed" },
+            PointerOutOfBounds { ptr, check, allocation_size } => {
+                write!(f, "Pointer must be in-bounds{} at offset {}, but is outside bounds of \
+                           allocation {} which has size {}",
+                       match check {
+                           InboundsCheck::Live => " and live",
+                           InboundsCheck::MaybeDead => "",
+                       },
                        ptr.offset.bytes(), ptr.alloc_id, allocation_size.bytes())
             },
             ValidationFailure(ref err) => {

src/librustc/mir/interpret/mod.rs

@@ -28,7 +28,7 @@ pub use self::error::{
 pub use self::value::{Scalar, ConstValue, ScalarMaybeUndef};
 
 pub use self::allocation::{
-    Allocation, AllocationExtra,
+    InboundsCheck, Allocation, AllocationExtra,
     Relocations, UndefMask,
 };
 

src/librustc_mir/interpret/memory.rs

@@ -28,7 +28,7 @@ use rustc_data_structures::fx::{FxHashSet, FxHashMap};
 use syntax::ast::Mutability;
 
 use super::{
-    Pointer, AllocId, Allocation, ConstValue, GlobalId, AllocationExtra,
+    Pointer, AllocId, Allocation, ConstValue, GlobalId, AllocationExtra, InboundsCheck,
     EvalResult, Scalar, EvalErrorKind, AllocType, PointerArithmetic,
     Machine, AllocMap, MayLeak, ScalarMaybeUndef, ErrorHandled,
 };
@@ -249,17 +249,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         // Check non-NULL/Undef, extract offset
         let (offset, alloc_align) = match ptr {
             Scalar::Ptr(ptr) => {
-                let (size, align) = self.get_size_and_align(ptr.alloc_id);
                 // check this is not NULL -- which we can ensure only if this is in-bounds
                 // of some (potentially dead) allocation.
-                if ptr.offset > size {
-                    return err!(PointerOutOfBounds {
-                        ptr: ptr.erase_tag(),
-                        access: true,
-                        allocation_size: size,
-                    });
-                };
-                // keep data for alignment check
+                self.check_bounds_ptr(ptr, InboundsCheck::MaybeDead)?;
+                // data required for alignment check
+                let (_, align) = self.get_size_and_align(ptr.alloc_id);
                 (ptr.offset.bytes(), align)
             }
             Scalar::Bits { bits, size } => {
@@ -293,18 +287,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
 
     /// Check 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.  The `access` boolean is just used
-    /// for the error message.
-    /// If you want to check bounds before doing a memory access, be sure to
-    /// check the pointer one past the end of your access, then everything will
-    /// work out exactly.
-    pub fn check_bounds_ptr(&self, ptr: Pointer<M::PointerTag>, access: bool) -> EvalResult<'tcx> {
-        let alloc = self.get(ptr.alloc_id)?;
-        let allocation_size = alloc.bytes.len() as u64;
+    /// in-bounds!  This follows C's/LLVM's rules.  `check` indicates whether we
+    /// additionally require the pointer to be pointing to a *live* (still allocated)
+    /// allocation.
+    /// If you want to check bounds before doing a memory access, better use `check_bounds`.
+    pub fn check_bounds_ptr(
+        &self,
+        ptr: Pointer<M::PointerTag>,
+        check: InboundsCheck,
+    ) -> EvalResult<'tcx> {
+        let allocation_size = match check {
+            InboundsCheck::Live => {
+                let alloc = self.get(ptr.alloc_id)?;
+                alloc.bytes.len() as u64
+            }
+            InboundsCheck::MaybeDead => {
+                self.get_size_and_align(ptr.alloc_id).0.bytes()
+            }
+        };
         if ptr.offset.bytes() > allocation_size {
             return err!(PointerOutOfBounds {
                 ptr: ptr.erase_tag(),
-                access,
+                check,
                 allocation_size: Size::from_bytes(allocation_size),
             });
         }
@@ -317,10 +321,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
         &self,
         ptr: Pointer<M::PointerTag>,
         size: Size,
-        access: bool
+        check: InboundsCheck,
     ) -> EvalResult<'tcx> {
         // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
-        self.check_bounds_ptr(ptr.offset(size, &*self)?, access)
+        self.check_bounds_ptr(ptr.offset(size, &*self)?, check)
     }
 }
 
@@ -626,7 +630,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
     ) -> EvalResult<'tcx, &[u8]> {
         assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`");
         self.check_align(ptr.into(), align)?;
-        self.check_bounds(ptr, size, true)?;
+        self.check_bounds(ptr, size, InboundsCheck::Live)?;
 
         if check_defined_and_ptr {
             self.check_defined(ptr, size)?;
@@ -677,7 +681,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
     ) -> EvalResult<'tcx, &mut [u8]> {
         assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`");
         self.check_align(ptr.into(), align)?;
-        self.check_bounds(ptr, size, true)?;
+        self.check_bounds(ptr, size, InboundsCheck::Live)?;
 
         self.mark_definedness(ptr, size, true)?;
         self.clear_relocations(ptr, size)?;

src/librustc_mir/interpret/operand.rs

@@ -19,7 +19,7 @@ use rustc::ty::layout::{self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerEx
 use rustc::mir::interpret::{
     GlobalId, AllocId,
     ConstValue, Pointer, Scalar,
-    EvalResult, EvalErrorKind
+    EvalResult, EvalErrorKind, InboundsCheck,
 };
 use super::{EvalContext, Machine, MemPlace, MPlaceTy, MemoryKind};
 pub use rustc::mir::interpret::ScalarMaybeUndef;
@@ -647,24 +647,27 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                 let variants_start = niche_variants.start().as_u32() as u128;
                 let variants_end = niche_variants.end().as_u32() as u128;
                 match raw_discr {
-                    ScalarMaybeUndef::Scalar(Scalar::Ptr(_)) => {
-                        // The niche must be just 0 (which a pointer value never is)
-                        assert!(niche_start == 0);
-                        assert!(variants_start == variants_end);
+                    ScalarMaybeUndef::Scalar(Scalar::Ptr(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).is_ok();
+                        if !ptr_valid {
+                            return err!(InvalidDiscriminant(raw_discr.erase_tag()));
+                        }
                         (dataful_variant.as_u32() as u128, dataful_variant)
                     },
                     ScalarMaybeUndef::Scalar(Scalar::Bits { bits: raw_discr, size }) => {
                         assert_eq!(size as u64, discr_val.layout.size.bytes());
-                        let discr = raw_discr.wrapping_sub(niche_start)
+                        let adjusted_discr = raw_discr.wrapping_sub(niche_start)
                             .wrapping_add(variants_start);
-                        if variants_start <= discr && discr <= variants_end {
-                            let index = discr as usize;
-                            assert_eq!(index as u128, discr);
+                        if variants_start <= adjusted_discr && adjusted_discr <= variants_end {
+                            let index = adjusted_discr as usize;
+                            assert_eq!(index as u128, adjusted_discr);
                             assert!(index < rval.layout.ty
                                 .ty_adt_def()
                                 .expect("tagged layout for non adt")
                                 .variants.len());
-                            (discr, VariantIdx::from_usize(index))
+                            (adjusted_discr, VariantIdx::from_usize(index))
                         } else {
                             (dataful_variant.as_u32() as u128, dataful_variant)
                         }

src/librustc_mir/interpret/validity.rs

@@ -17,7 +17,7 @@ use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf, VariantIdx};
 use rustc::ty;
 use rustc_data_structures::fx::FxHashSet;
 use rustc::mir::interpret::{
-    Scalar, AllocType, EvalResult, EvalErrorKind,
+    Scalar, AllocType, EvalResult, EvalErrorKind, InboundsCheck,
 };
 
 use super::{
@@ -394,7 +394,8 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                         }
                         // Maintain the invariant that the place we are checking is
                         // already verified to be in-bounds.
-                        try_validation!(self.ecx.memory.check_bounds(ptr, size, false),
+                        try_validation!(
+                            self.ecx.memory.check_bounds(ptr, size, InboundsCheck::Live),
                             "dangling (not entirely in bounds) reference", self.path);
                     }
                     // Check if we have encountered this pointer+layout combination

src/test/ui/consts/const-eval/ub-enum.rs

@@ -39,6 +39,7 @@ union TransmuteEnum2 {
     in3: (),
     out1: Enum2,
     out2: Wrap<Enum2>, // something wrapping the enum so that we test layout first, not enum
+    out3: Option<Enum2>,
 }
 const BAD_ENUM2: Enum2 = unsafe { TransmuteEnum2 { in1: 0 }.out1 };
 //~^ ERROR is undefined behavior
@@ -51,6 +52,10 @@ const BAD_ENUM4: Wrap<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out2 };
 const BAD_ENUM_UNDEF: [Enum2; 2] = [unsafe { TransmuteEnum2 { in3: () }.out1 }; 2];
 //~^ ERROR is undefined behavior
 
+// Pointer value in an enum with a niche that is not just 0.
+const BAD_ENUM_PTR: Option<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out3 };
+//~^ ERROR is undefined behavior
+
 // Invalid enum field content (mostly to test printing of paths for enum tuple
 // variants and tuples).
 union TransmuteChar {

src/test/ui/consts/const-eval/ub-enum.stderr

@@ -7,45 +7,53 @@ LL | const BAD_ENUM: Enum = unsafe { TransmuteEnum { a: &1 }.out };
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-enum.rs:43:1
+  --> $DIR/ub-enum.rs:44:1
    |
 LL | const BAD_ENUM2: Enum2 = unsafe { TransmuteEnum2 { in1: 0 }.out1 };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected a valid enum discriminant
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-enum.rs:45:1
+  --> $DIR/ub-enum.rs:46:1
    |
 LL | const BAD_ENUM3: Enum2 = unsafe { TransmuteEnum2 { in2: &0 }.out1 };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-enum.rs:47:1
+  --> $DIR/ub-enum.rs:48:1
    |
 LL | const BAD_ENUM4: Wrap<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out2 };
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected something that cannot possibly fail to be in the range 2..=2
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-enum.rs:51:1
+  --> $DIR/ub-enum.rs:52:1
    |
 LL | const BAD_ENUM_UNDEF: [Enum2; 2] = [unsafe { TransmuteEnum2 { in3: () }.out1 }; 2];
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes at [0], but expected a valid enum discriminant
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
 error[E0080]: it is undefined behavior to use this value
-  --> $DIR/ub-enum.rs:61:1
+  --> $DIR/ub-enum.rs:56:1
+   |
+LL | const BAD_ENUM_PTR: Option<Enum2> = unsafe { TransmuteEnum2 { in2: &0 }.out3 };
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected a valid enum discriminant
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
+
+error[E0080]: it is undefined behavior to use this value
+  --> $DIR/ub-enum.rs:66:1
    |
 LL | const BAD_ENUM_CHAR: Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b }));
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .<downcast-variant(Some)>.0.1, but expected something less or equal to 1114111
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior
 
-error: aborting due to 6 previous errors
+error: aborting due to 7 previous errors
 
 For more information about this error, try `rustc --explain E0080`.