From 899bc14cc08777bd6c5738c5697082b1cfbc7755 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 22 Aug 2018 11:27:38 +0200 Subject: [PATCH] fix validating fat pointers to user-defined unsized types --- src/librustc_mir/interpret/place.rs | 2 + src/librustc_mir/interpret/validity.rs | 79 +++++++++----------------- src/test/ui/union-ub-fat-ptr.rs | 8 ++- src/test/ui/union-ub-fat-ptr.stderr | 34 ++++++----- 4 files changed, 56 insertions(+), 67 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 58c976086bb..91182edc2f5 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -774,6 +774,8 @@ pub fn unpack_unsized_mplace(&self, mplace: MPlaceTy<'tcx>) -> EvalResult<'tcx, let (size, align) = self.read_size_and_align_from_vtable(vtable)?; assert_eq!(size, layout.size); assert_eq!(align.abi(), layout.align.abi()); // only ABI alignment is preserved + // FIXME: More checks for the vtable? We could make sure it is exactly + // the one one would expect for this type. // Done! layout }, diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index c28c3b4d3f1..5eb32cd7eac 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -9,7 +9,7 @@ }; use super::{ - MPlaceTy, PlaceExtra, Machine, EvalContext + MPlaceTy, Machine, EvalContext }; macro_rules! validation_failure{ @@ -281,59 +281,32 @@ pub fn validate_mplace( }, layout::FieldPlacement::Arbitrary { ref offsets, .. } => { // Fat pointers need special treatment. - match dest.layout.ty.builtin_deref(true).map(|tam| &tam.ty.sty) { - | Some(ty::TyStr) - | Some(ty::TySlice(_)) => { - // check the length (for nicer error messages); must be valid even - // for a raw pointer. - let len_mplace = self.mplace_field(dest, 1)?; - let len = self.read_scalar(len_mplace.into())?; - let len = match len.to_bits(len_mplace.layout.size) { - Err(_) => return validation_failure!("length is not a valid integer", path), - Ok(len) => len as u64, - }; - // for safe ptrs, get the fat ptr, and recursively check it - if !dest.layout.ty.is_unsafe_ptr() { - let ptr = self.ref_to_mplace(self.read_value(dest.into())?)?; - assert_eq!(ptr.extra, PlaceExtra::Length(len)); - let unpacked_ptr = self.unpack_unsized_mplace(ptr)?; - if seen.insert(unpacked_ptr) { - todo.push((unpacked_ptr, path_clone_and_deref(path))); - } - } - }, - Some(ty::TyDynamic(..)) => { - // check the vtable (for nicer error messages); must be valid even for a - // raw ptr. - let vtable = self.read_scalar(self.mplace_field(dest, 1)?.into())?; - let vtable = match vtable.to_ptr() { - Err(_) => return validation_failure!("vtable address is not a pointer", path), - Ok(vtable) => vtable, - }; - // for safe ptrs, get the fat ptr, and recursively check it - if !dest.layout.ty.is_unsafe_ptr() { - let ptr = self.ref_to_mplace(self.read_value(dest.into())?)?; - assert_eq!(ptr.extra, PlaceExtra::Vtable(vtable)); - let unpacked_ptr = self.unpack_unsized_mplace(ptr)?; - if seen.insert(unpacked_ptr) { - todo.push((unpacked_ptr, path_clone_and_deref(path))); - } - // FIXME: More checks for the vtable... making sure it is exactly - // the one one would expect for this type. - } - }, - Some(ty) => - bug!("Unexpected fat pointer target type {:?}", ty), - None => { - // Not a pointer, perform regular aggregate handling below - for i in 0..offsets.len() { - let field = self.mplace_field(dest, i as u64)?; - path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); - self.validate_mplace(field, path, seen, todo)?; - path.truncate(path_len); + if dest.layout.ty.builtin_deref(true).is_some() { + // This is a fat pointer. + let ptr = match self.ref_to_mplace(self.read_value(dest.into())?) { + Ok(ptr) => ptr, + Err(ReadPointerAsBytes) => + return validation_failure!("fat pointer length is not a valid integer", path), + Err(ReadBytesAsPointer) => + return validation_failure!("fat pointer vtable is not a valid pointer", path), + Err(err) => return Err(err), + }; + let unpacked_ptr = self.unpack_unsized_mplace(ptr)?; + // for safe ptrs, recursively check it + if !dest.layout.ty.is_unsafe_ptr() { + if seen.insert(unpacked_ptr) { + todo.push((unpacked_ptr, path_clone_and_deref(path))); } - // FIXME: For a TyStr, check that this is valid UTF-8. - }, + } + } else { + // Not a pointer, perform regular aggregate handling below + for i in 0..offsets.len() { + let field = self.mplace_field(dest, i as u64)?; + path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); + self.validate_mplace(field, path, seen, todo)?; + path.truncate(path_len); + } + // FIXME: For a TyStr, check that this is valid UTF-8. } } } diff --git a/src/test/ui/union-ub-fat-ptr.rs b/src/test/ui/union-ub-fat-ptr.rs index dd9a74a790c..ffa824fc6af 100644 --- a/src/test/ui/union-ub-fat-ptr.rs +++ b/src/test/ui/union-ub-fat-ptr.rs @@ -37,6 +37,7 @@ union SliceTransmute { bad: BadSliceRepr, slice: &'static [u8], str: &'static str, + my_str: &'static Str, } #[repr(C)] @@ -70,6 +71,8 @@ union DynTransmute { trait Trait {} impl Trait for bool {} +struct Str(str); + // OK const A: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 1 } }.str}; // bad str @@ -78,6 +81,9 @@ impl Trait for bool {} // bad str const C: &str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.str}; //~^ ERROR this constant likely exhibits undefined behavior +// bad str in Str +const C2: &Str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.my_str}; +//~^ ERROR this constant likely exhibits undefined behavior // OK const A2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 1 } }.slice}; @@ -85,7 +91,7 @@ impl Trait for bool {} const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice}; //~^ ERROR this constant likely exhibits undefined behavior // bad slice -const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice}; +const C3: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice}; //~^ ERROR this constant likely exhibits undefined behavior // bad trait object diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index af85d1d39c4..85a514ea99e 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -1,5 +1,5 @@ error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/union-ub-fat-ptr.rs:76:1 + --> $DIR/union-ub-fat-ptr.rs:79:1 | LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access at offset N, outside bounds of allocation N which has size N @@ -7,31 +7,39 @@ LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: = 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:79:1 + --> $DIR/union-ub-fat-ptr.rs:82:1 | LL | const C: &str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.str}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered length is not a valid integer + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered fat pointer length is not a valid integer | = 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:85:1 | +LL | const C2: &Str = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.my_str}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered fat pointer length is not a valid integer + | + = 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:91:1 + | LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access at offset N, outside bounds of allocation N which has size N | = 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:88:1 + --> $DIR/union-ub-fat-ptr.rs:94:1 | -LL | const C2: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered length is not a valid integer +LL | const C3: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered fat pointer length is not a valid integer | = 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:92:1 + --> $DIR/union-ub-fat-ptr.rs:98:1 | LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ tried to access memory with alignment N, but alignment N is required @@ -39,7 +47,7 @@ LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, 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:95:1 + --> $DIR/union-ub-fat-ptr.rs:101:1 | LL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ a memory access tried to interpret some bytes as a pointer @@ -47,15 +55,15 @@ LL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtabl = 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:98:1 + --> $DIR/union-ub-fat-ptr.rs:104:1 | LL | const F: &Trait = unsafe { DynTransmute { bad: BadDynRepr { ptr: &92, vtable: 3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered vtable address is not a pointer + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered fat pointer length is not a valid integer | = 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:102:1 + --> $DIR/union-ub-fat-ptr.rs:108:1 | LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected something in the range 0..=1 @@ -63,13 +71,13 @@ LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; = 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:106:1 + --> $DIR/union-ub-fat-ptr.rs:112:1 | LL | const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors For more information about this error, try `rustc --explain E0080`. -- 2.44.0