Skip to content

Commit

Permalink
check that entire ref is in-bounds before recursing; add macro for va…
Browse files Browse the repository at this point in the history 
…lidation msgs on error
  • Loading branch information
@RalfJung
RalfJung committed Oct 2, 2018
1 parent 2731c7d commit 5c15b64
Show file tree
Hide file tree
Showing 7 changed files with 86 additions and 96 deletions.
15 changes: 10 additions & 5 deletions src/librustc_mir/interpret/memory.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
/// 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(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> {
pub fn check_bounds_ptr(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> {
let alloc = self.get(ptr.alloc_id)?;
let allocation_size = alloc.bytes.len() as u64;
if ptr.offset.bytes() > allocation_size {
Expand All @@ -296,6 +296,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
}
Ok(())
}

/// Check if the memory range beginning at `ptr` and of size `Size` is "in-bounds".
#[inline(always)]
pub fn check_bounds(&self, ptr: Pointer, size: Size, access: bool) -> 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)
}
}

/// Allocation accessors
Expand Down Expand Up @@ -524,8 +531,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)?;
// if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
self.check_bounds(ptr.offset(size, &*self)?, true)?;
self.check_bounds(ptr, size, true)?;

if check_defined_and_ptr {
self.check_defined(ptr, size)?;
Expand Down Expand Up @@ -569,8 +575,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)?;
// if ptr.offset is in bounds, then so is ptr (because offset checks for overflow)
self.check_bounds(ptr.offset(size, &self)?, true)?;
self.check_bounds(ptr, size, true)?;

self.mark_definedness(ptr, size, true)?;
self.clear_relocations(ptr, size)?;
Expand Down
116 changes: 40 additions & 76 deletions src/librustc_mir/interpret/validity.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use super::{
OpTy, MPlaceTy, Machine, EvalContext, ScalarMaybeUndef
};

macro_rules! validation_failure{
macro_rules! validation_failure {
($what:expr, $where:expr, $details:expr) => {{
let where_ = path_format($where);
let where_ = if where_.is_empty() {
Expand All @@ -49,6 +49,15 @@ macro_rules! validation_failure{
}};
}

macro_rules! try_validation {
($e:expr, $what:expr, $where:expr) => {{
match $e {
Ok(x) => x,
Err(_) => return validation_failure!($what, $where),
}
}}
}

/// We want to show a nice path to the invalid field for diagnotsics,
/// but avoid string operations in the happy case where no error happens.
/// So we track a `Vec<PathElem>` where `PathElem` contains all the data we
Expand Down Expand Up @@ -230,8 +239,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
path: &mut Vec<PathElem>,
ref_tracking: Option<&mut RefTracking<'tcx>>,
) -> EvalResult<'tcx> {
// Skip recursion for some external statics
if let Scalar::Ptr(ptr) = place.ptr {
// Before we do anything else, make sure this is entirely in-bounds.
if !place.layout.is_zst() {
let ptr = try_validation!(place.ptr.to_ptr(),
"integer pointer in non-ZST reference", path);
let size = self.size_and_align_of(place.extra, place.layout)?.0;
try_validation!(self.memory.check_bounds(ptr, size, false),
"dangling reference (not entirely in bounds)", path);
// Skip recursion for some external statics
let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id);
if let Some(AllocType::Static(did)) = alloc_kind {
// statics from other crates are already checked.
Expand All @@ -257,7 +272,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
Ok(())
}

/// This function checks the data at `op`.
/// This function checks the data at `op`. `op` is assumed to cover valid memory if it
/// is an indirect operand.
/// It will error if the bits at the destination do not match the ones described by the layout.
/// The `path` may be pushed to, but the part that is present when the function
/// starts must not be changed!
Expand Down Expand Up @@ -305,13 +321,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
let dest = match dest.layout.ty.sty {
ty::Dynamic(..) => {
let dest = dest.to_mem_place(); // immediate trait objects are not a thing
match self.unpack_dyn_trait(dest) {
Ok(res) => res.1.into(),
Err(_) =>
return validation_failure!(
"invalid vtable in fat pointer", path
),
}
try_validation!(self.unpack_dyn_trait(dest),
"invalid vtable in fat pointer", path).1.into()
}
_ => dest
};
Expand All @@ -337,20 +348,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
// expectation.
layout::Abi::Scalar(ref scalar_layout) => {
let size = scalar_layout.value.size(self);
let value = match self.read_value(dest) {
Ok(val) => val,
Err(err) => match err.kind {
EvalErrorKind::PointerOutOfBounds { .. } |
EvalErrorKind::ReadUndefBytes(_) =>
return validation_failure!(
"uninitialized or out-of-bounds memory", path
),
_ =>
return validation_failure!(
"unrepresentable data", path
),
}
};
let value = try_validation!(self.read_value(dest),
"uninitialized or unrepresentable data", path);
let scalar = value.to_scalar_or_undef();
self.validate_scalar(scalar, size, scalar_layout, &path, dest.layout.ty)?;
// Recursively check *safe* references
Expand All @@ -367,35 +366,24 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
if dest.layout.ty.builtin_deref(true).is_some() =>
{
// This is a fat pointer.
let ptr = match self.read_value(dest.into())
.and_then(|val| self.ref_to_mplace(val))
{
Ok(ptr) => ptr,
Err(_) =>
return validation_failure!(
"undefined location or metadata in fat pointer", path
),
};
let ptr = try_validation!(self.read_value(dest.into()),
"undefined location in fat pointer", path);
let ptr = try_validation!(self.ref_to_mplace(ptr),
"undefined metadata in fat pointer", path);
// check metadata early, for better diagnostics
match self.tcx.struct_tail(ptr.layout.ty).sty {
ty::Dynamic(..) => {
match ptr.extra.unwrap().to_ptr() {
Ok(_) => {},
Err(_) =>
return validation_failure!(
"non-pointer vtable in fat pointer", path
),
}
let vtable = try_validation!(ptr.extra.unwrap().to_ptr(),
"non-pointer vtable in fat pointer", path);
try_validation!(self.read_drop_type_from_vtable(vtable),
"invalid drop fn in vtable", path);
try_validation!(self.read_size_and_align_from_vtable(vtable),
"invalid size or align in vtable", path);
// FIXME: More checks for the vtable.
}
ty::Slice(..) | ty::Str => {
match ptr.extra.unwrap().to_usize(self) {
Ok(_) => {},
Err(_) =>
return validation_failure!(
"non-integer slice length in fat pointer", path
),
}
try_validation!(ptr.extra.unwrap().to_usize(self),
"non-integer slice length in fat pointer", path);
}
_ =>
bug!("Unexpected unsized type tail: {:?}",
Expand All @@ -418,23 +406,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
match dest.layout.ty.sty {
// Special handling for strings to verify UTF-8
ty::Str => {
match self.read_str(dest) {
Ok(_) => {},
Err(err) => match err.kind {
EvalErrorKind::PointerOutOfBounds { .. } |
EvalErrorKind::ReadUndefBytes(_) =>
// The error here looks slightly different than it does
// for slices, because we do not report the index into the
// str at which we are OOB.
return validation_failure!(
"uninitialized or out-of-bounds memory", path
),
_ =>
return validation_failure!(
"non-UTF-8 data in str", path
),
}
}
try_validation!(self.read_str(dest),
"uninitialized or non-UTF-8 data in str", path);
}
// Special handling for arrays/slices of builtin integer types
ty::Array(tys, ..) | ty::Slice(tys) if {
Expand Down Expand Up @@ -470,18 +443,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
"undefined bytes", path
)
},
EvalErrorKind::PointerOutOfBounds { allocation_size, .. } => {
// If the array access is out-of-bounds, the first
// undefined access is the after the end of the array.
let i = (allocation_size.bytes() * ty_size) as usize;
path.push(PathElem::ArrayElem(i));
},
_ => (),
// Other errors shouldn't be possible
_ => return Err(err),
}

return validation_failure!(
"uninitialized or out-of-bounds memory", path
)
}
}
},
Expand Down
8 changes: 6 additions & 2 deletions src/test/ui/consts/const-eval/ub-uninhabit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@
// except according to those terms.

union Foo {
a: u8,
a: usize,
b: Bar,
c: &'static Bar,
}

#[derive(Copy, Clone)]
enum Bar {}

const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b};
const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b };
//~^ ERROR this constant likely exhibits undefined behavior

const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c };
//~^ ERROR this constant likely exhibits undefined behavior

fn main() {
Expand Down
16 changes: 12 additions & 4 deletions src/test/ui/consts/const-eval/ub-uninhabit.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/ub-uninhabit.rs:19:1
--> $DIR/ub-uninhabit.rs:20:1
|
LL | const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type
LL | const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type
|
= 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 previous error
error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/ub-uninhabit.rs:23:1
|
LL | const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type at .<deref>
|
= 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 2 previous errors

For more information about this error, try `rustc --explain E0080`.
4 changes: 1 addition & 3 deletions src/test/ui/consts/const-eval/ub-usize-in-ref.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,13 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-pass

union Foo {
a: &'static u8,
b: usize,
}

// This might point to an invalid address, but that's the user's problem
const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a};
//~^ ERROR this constant likely exhibits undefined behavior

fn main() {
}
11 changes: 11 additions & 0 deletions src/test/ui/consts/const-eval/ub-usize-in-ref.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/ub-usize-in-ref.rs:16:1
|
LL | const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference
|
= 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 previous error

For more information about this error, try `rustc --explain E0080`.
12 changes: 6 additions & 6 deletions src/test/ui/union-ub-fat-ptr.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:87:1
|
LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or out-of-bounds memory at .<deref>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds)
|
= 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

Expand All @@ -26,7 +26,7 @@ error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:99:1
|
LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or out-of-bounds memory at .<deref>[1]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds)
|
= 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

Expand All @@ -42,15 +42,15 @@ error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:106:1
|
LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtable in fat pointer at .<deref>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in 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

error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:109:1
|
LL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust};
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtable in fat pointer at .<deref>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in 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

Expand Down Expand Up @@ -98,15 +98,15 @@ error[E0080]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:132:1
|
LL | const J1: &str = unsafe { SliceTransmute { slice: &[0xFF] }.str };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-UTF-8 data in str at .<deref>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or non-UTF-8 data in str at .<deref>
|
= 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]: this constant likely exhibits undefined behavior
--> $DIR/union-ub-fat-ptr.rs:135:1
|
LL | const J2: &MyStr = unsafe { SliceTransmute { slice: &[0xFF] }.my_str };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-UTF-8 data in str at .<deref>.0
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or non-UTF-8 data in str at .<deref>.0
|
= 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

Expand Down

0 comments on commit 5c15b64

Please sign in to comment.