Skip to content

Make uninit checks stricter but avoid issues for old hyper #99389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
+289 −123
Closed

Make uninit checks stricter but avoid issues for old hyper #99389

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85545dd
Make uninit checks stricter but avoid issues for old hyper
5225225 Jul 17, 2022
c84936e
Fix docs
5225225 Jul 18, 2022
6770c21
Still do the type walking for zero init checks
5225225 Jul 18, 2022
a2f44ac
We ignore all references to size 0 align 1 data.
5225225 Jul 18, 2022
a007e7b
Reword explanation for ignoring ZST 1-aligned references
5225225 Jul 18, 2022
5ee555c
Only panic for refs to slices and str, not dyn trait
5225225 Jul 20, 2022
eb1b384
Adjust justification comment
5225225 Jul 20, 2022
a6ab54b
Change comment to mention vtable
5225225 Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions compiler/rustc_const_eval/src/might_permit_raw_init.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::const_eval::CompileTimeInterpreter;
use crate::interpret::{InterpCx, MemoryKind, OpTy};
use rustc_middle::ty::layout::LayoutCx;
use rustc_middle::ty::{layout::TyAndLayout, ParamEnv, TyCtxt};
use rustc_middle::ty::{
layout::{self, TyAndLayout},
ParamEnv, TyCtxt,
};
use rustc_session::Limit;
use rustc_target::abi::InitKind;

@@ -35,6 +38,6 @@ pub fn might_permit_raw_init<'tcx>(
cx.validate_operand(&ot).is_ok()
} else {
let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() };
ty.might_permit_raw_init(&layout_cx, kind)
layout::might_permit_raw_init(ty, &layout_cx, kind)
}
}
151 changes: 151 additions & 0 deletions compiler/rustc_middle/src/ty/layout.rs
View file
Original file line number Diff line number Diff line change
@@ -3492,3 +3492,154 @@ fn make_thin_self_ptr<'tcx>(
..tcx.layout_of(ty::ParamEnv::reveal_all().and(unit_ptr_ty)).unwrap()
}
}

/// Determines if this type permits "raw" initialization by just transmuting some
/// memory into an instance of `T`.
///
/// If called with InitKind::Uninit, this function must always panic if a 0x01 filled buffer would
/// cause LLVM UB.
///
/// This code is intentionally conservative, and will not detect
/// * making uninitialized types who have a full valid range (ints, floats, raw pointers)
/// * uninit `&T` where T has align 1 size 0 (only inside arrays).
/// * zero init enums where a discriminant with tag 0 exists, but is invalid to be zeroed
/// * zero init type that does not allow zero init (only inside arrays)
///
/// A strict form of these checks that uses const evaluation exists in
/// `rustc_const_eval::might_permit_raw_init`, and a tracking issue for making these checks
/// stricter is <https://github.com/rust-lang/rust/issues/66151>.
///
/// FIXME: Once all the conservatism is removed from here, and the checks are ran by default,
/// we can use the const evaluation checks always instead.
pub fn might_permit_raw_init<'tcx, C>(this: TyAndLayout<'tcx>, cx: &C, init_kind: InitKind) -> bool
where
C: HasDataLayout + ty::layout::HasParamEnv<'tcx> + ty::layout::HasTyCtxt<'tcx>,
{
return might_permit_raw_init_inner(this, cx, init_kind, false);

fn might_permit_raw_init_inner<'tcx, C>(
this: TyAndLayout<'tcx>,
cx: &C,
init_kind: InitKind,
inside_array: bool,
) -> bool
where
C: HasDataLayout + ty::layout::HasParamEnv<'tcx> + ty::layout::HasTyCtxt<'tcx>,
{
let scalar_allows_raw_init = move |s: Scalar| -> bool {
match init_kind {
InitKind::Zero => {
// The range must contain 0.
s.valid_range(cx).contains(0)
}
InitKind::Uninit => {
// FIXME(#66151) This should be "is the type a union", but that's pending on a
// resolution to https://github.com/rust-lang/unsafe-code-guidelines/issues/71
// And likely a large number of crates fail with those rules, though no crater
// run has been done yet.
//
// The range must include all values.
s.is_always_valid(cx)
}
}
};

// These are bypasses in order to try not to break quite as many crates so quickly.
// They are being done only if we're inside an array, to ensure we don't panic on *less*
// things than we did before this change.
// See: https://github.com/rust-lang/rust/pull/99389
if inside_array {
match init_kind {
// We panic if creating this type with all 0x01 bytes would
// cause LLVM UB.
//
// Therefore, in order for us to not panic, it must either be a
// reference to [T] where T has align 1 (where we don't statically know
// the size, so we don't emit any dereferenceable), or a reference to str
// which acts much like a [u8].
//
// We *do* need to panic for &dyn Trait, even though the layout of dyn Trait is
// size 0 align 1, because &dyn Trait holds a reference to the vtable, which
// must be aligned and dereferenceable.
//
// This even applies to *const dyn Trait, which holds a reference to the vtable
// and therefore must be valid, so 1-initialization is not okay there.
//
// If this bypass didn't exist, old versions of `hyper` with no semver compatible
// fix (0.11, 0.12, 0.13) would panic, as they make uninit &[u8] and &str.
InitKind::Uninit => {
if let ty::Ref(_, inner, _) = this.ty.kind() {
if let ty::Slice(slice_inner) = inner.kind() {
let penv = ty::ParamEnv::reveal_all().and(*slice_inner);

if let Ok(l) = cx.tcx().layout_of(penv) {
return l.layout.align().abi == Align::ONE;
}
}

if ty::Str == *inner.kind() {
return true;
}
}
}
// FIXME(#66151) We need to ignore all forms of zero data being made inside an
// array, because old versions of crossbeam make zeroed data, sometimes at an
// arbitrary type chosen by the user (such as for crossbeam-channel).
InitKind::Zero => {
return true;
}
}
}

// Check the ABI.
let valid = match this.abi {
Abi::Uninhabited => false, // definitely UB
Abi::Scalar(s) => scalar_allows_raw_init(s),
Abi::ScalarPair(s1, s2) => scalar_allows_raw_init(s1) && scalar_allows_raw_init(s2),
Abi::Vector { element: s, count } => count == 0 || scalar_allows_raw_init(s),
Abi::Aggregate { .. } => true, // Fields are checked below.
};
if !valid {
// This is definitely not okay.
return false;
}

// If we have not found an error yet, we need to recursively descend into fields.
match &this.fields {
FieldsShape::Primitive | FieldsShape::Union { .. } => {}
FieldsShape::Array { count, .. } => {
if *count > 0
&& !might_permit_raw_init_inner(
this.field(cx, 0),
cx,
init_kind,
/*inside_array*/ true,
)
{
// Found non empty array with a type that is unhappy about this kind of initialization
return false;
}
}
FieldsShape::Arbitrary { offsets, .. } => {
for idx in 0..offsets.len() {
if !might_permit_raw_init_inner(
this.field(cx, idx),
cx,
init_kind,
inside_array,
) {
// We found a field that is unhappy with this kind of initialization.
return false;
}
}
}
}

if matches!(this.variants, Variants::Multiple { .. }) && init_kind == InitKind::Uninit {
// All uninit enums are automatically invalid, even if their discriminant includes all values.
return false;
}

true
}
}
68 changes: 0 additions & 68 deletions compiler/rustc_target/src/abi/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1485,72 +1485,4 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
Abi::Aggregate { sized } => sized && self.size.bytes() == 0,
}
}

/// Determines if this type permits "raw" initialization by just transmuting some
/// memory into an instance of `T`.
///
/// `init_kind` indicates if the memory is zero-initialized or left uninitialized.
///
/// This code is intentionally conservative, and will not detect
/// * zero init of an enum whose 0 variant does not allow zero initialization
/// * making uninitialized types who have a full valid range (ints, floats, raw pointers)
/// * Any form of invalid value being made inside an array (unless the value is uninhabited)
///
/// A strict form of these checks that uses const evaluation exists in
/// `rustc_const_eval::might_permit_raw_init`, and a tracking issue for making these checks
/// stricter is <https://github.com/rust-lang/rust/issues/66151>.
///
/// FIXME: Once all the conservatism is removed from here, and the checks are ran by default,
/// we can use the const evaluation checks always instead.
pub fn might_permit_raw_init<C>(self, cx: &C, init_kind: InitKind) -> bool
where
Self: Copy,
Ty: TyAbiInterface<'a, C>,
C: HasDataLayout,
{
let scalar_allows_raw_init = move |s: Scalar| -> bool {
match init_kind {
InitKind::Zero => {
// The range must contain 0.
s.valid_range(cx).contains(0)
}
InitKind::Uninit => {
// The range must include all values.
s.is_always_valid(cx)
}
}
};

// Check the ABI.
let valid = match self.abi {
Abi::Uninhabited => false, // definitely UB
Abi::Scalar(s) => scalar_allows_raw_init(s),
Abi::ScalarPair(s1, s2) => scalar_allows_raw_init(s1) && scalar_allows_raw_init(s2),
Abi::Vector { element: s, count } => count == 0 || scalar_allows_raw_init(s),
Abi::Aggregate { .. } => true, // Fields are checked below.
};
if !valid {
// This is definitely not okay.
return false;
}

// If we have not found an error yet, we need to recursively descend into fields.
match &self.fields {
FieldsShape::Primitive | FieldsShape::Union { .. } => {}
FieldsShape::Array { .. } => {
// FIXME(#66151): For now, we are conservative and do not check arrays by default.
}
FieldsShape::Arbitrary { offsets, .. } => {
for idx in 0..offsets.len() {
if !self.field(cx, idx).might_permit_raw_init(cx, init_kind) {
// We found a field that is unhappy with this kind of initialization.
return false;
}
}
}
}

// FIXME(#66151): For now, we are conservative and do not check `self.variants`.
true
}
}
186 changes: 133 additions & 53 deletions src/test/ui/intrinsics/panic-uninitialized-zeroed.rs
View file
Original file line number Diff line number Diff line change
@@ -64,6 +64,31 @@ enum ZeroIsValid {
One(NonNull<()>) = 1,
}

#[rustfmt::skip]
#[allow(dead_code)]
enum SoManyVariants {
A1, A2, A3, A4, A5, A6, A7, A8, A9, A10, A11, A12, A13, A14, A15, A16, A17,
A18, A19, A20, A21, A22, A23, A24, A25, A26, A27, A28, A29, A30, A31, A32,
A33, A34, A35, A36, A37, A38, A39, A40, A41, A42, A43, A44, A45, A46, A47,
A48, A49, A50, A51, A52, A53, A54, A55, A56, A57, A58, A59, A60, A61, A62,
A63, A64, A65, A66, A67, A68, A69, A70, A71, A72, A73, A74, A75, A76, A77,
A78, A79, A80, A81, A82, A83, A84, A85, A86, A87, A88, A89, A90, A91, A92,
A93, A94, A95, A96, A97, A98, A99, A100, A101, A102, A103, A104, A105, A106,
A107, A108, A109, A110, A111, A112, A113, A114, A115, A116, A117, A118, A119,
A120, A121, A122, A123, A124, A125, A126, A127, A128, A129, A130, A131, A132,
A133, A134, A135, A136, A137, A138, A139, A140, A141, A142, A143, A144, A145,
A146, A147, A148, A149, A150, A151, A152, A153, A154, A155, A156, A157, A158,
A159, A160, A161, A162, A163, A164, A165, A166, A167, A168, A169, A170, A171,
A172, A173, A174, A175, A176, A177, A178, A179, A180, A181, A182, A183, A184,
A185, A186, A187, A188, A189, A190, A191, A192, A193, A194, A195, A196, A197,
A198, A199, A200, A201, A202, A203, A204, A205, A206, A207, A208, A209, A210,
A211, A212, A213, A214, A215, A216, A217, A218, A219, A220, A221, A222, A223,
A224, A225, A226, A227, A228, A229, A230, A231, A232, A233, A234, A235, A236,
A237, A238, A239, A240, A241, A242, A243, A244, A245, A246, A247, A248, A249,
A250, A251, A252, A253, A254, A255, A256,
}

#[track_caller]
fn test_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) {
let err = panic::catch_unwind(op).err();
assert_eq!(
@@ -72,6 +97,19 @@ fn test_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) {
);
}

#[track_caller]
// If strict mode is enabled, expect the msg. Otherwise, expect there to be no error.
fn test_strict_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) {
let err = panic::catch_unwind(op).err();

let expectation = if cfg!(strict) { Some(&msg) } else { None };

assert_eq!(
err.as_ref().and_then(|a| a.downcast_ref::<&str>()),
expectation
);
}

fn main() {
unsafe {
// Uninhabited types
@@ -221,6 +259,50 @@ fn main() {
"attempted to leave type `core::mem::manually_drop::ManuallyDrop<LR>` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<&'static [u8]>(),
"attempted to leave type `&[u8]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<&'static [u16]>(),
"attempted to leave type `&[u16]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<SoManyVariants>(),
"attempted to leave type `SoManyVariants` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<[&'static [u16]; 1]>(),
"attempted to leave type `[&[u16]; 1]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<[NonNull<()>; 1]>(),
"attempted to leave type `[core::ptr::non_null::NonNull<()>; 1]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<(&[u8], &str, &())>(),
"attempted to leave type `(&[u8], &str, &())` uninitialized, which is invalid"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one we could accept, if we wanted, by accepting all references to sized align-1-ZST... doesn't seem very important though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though allowing &[u8] and rejecting &() feels rather arbitrary so we should probably allow both then?

Copy link
Contributor Author

@5225225 5225225 Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do, but I figure since not many people are actually making a reference to a ZST, no harm in forbidding it now.

The changes here are rather arbitrary in their nature, we're only not panicking on the bits that we can't panic on just yet, while making the checks stricter where we can.

Making an uninit [bool; 1] will start to panic after this change, even if that's still not LLVM UB if we 0x01 fill, because there's not really a widely used crate that does that (inb4 some crate started doing that since the last crater run :p).

Hyper doesn't make uninit refs to sized ZSTs, so I don't think we need to allow them here, even if it is somewhat inconsistent to forbid them.

I'm fine with ignoring &() as well, I just don't feel it's needed.

);

test_panic_msg(
|| mem::uninitialized::<[&(); 1]>(),
"attempted to leave type `[&(); 1]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<[&dyn Send; 1]>(),
"attempted to leave type `[&dyn core::marker::Send; 1]` uninitialized, which is invalid"
);
test_panic_msg(
|| mem::uninitialized::<[*const dyn Send; 1]>(),
"attempted to leave type `[*const dyn core::marker::Send; 1]` uninitialized, which is invalid"
);

// Some things that should work.
let _val = mem::zeroed::<bool>();
let _val = mem::zeroed::<LR>();
@@ -230,63 +312,61 @@ fn main() {
let _val = mem::zeroed::<MaybeUninit<NonNull<u32>>>();
let _val = mem::zeroed::<[!; 0]>();
let _val = mem::zeroed::<ZeroIsValid>();
let _val = mem::zeroed::<SoManyVariants>();
let _val = mem::uninitialized::<[SoManyVariants; 0]>();
let _val = mem::uninitialized::<MaybeUninit<bool>>();
let _val = mem::uninitialized::<[!; 0]>();
let _val = mem::uninitialized::<()>();
let _val = mem::uninitialized::<ZeroSized>();

if cfg!(strict) {
test_panic_msg(
|| mem::uninitialized::<i32>(),
"attempted to leave type `i32` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<*const ()>(),
"attempted to leave type `*const ()` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::uninitialized::<[i32; 1]>(),
"attempted to leave type `[i32; 1]` uninitialized, which is invalid"
);

test_panic_msg(
|| mem::zeroed::<NonNull<()>>(),
"attempted to zero-initialize type `core::ptr::non_null::NonNull<()>`, which is invalid"
);

test_panic_msg(
|| mem::zeroed::<[NonNull<()>; 1]>(),
"attempted to zero-initialize type `[core::ptr::non_null::NonNull<()>; 1]`, which is invalid"
);

// FIXME(#66151) we conservatively do not error here yet (by default).
test_panic_msg(
|| mem::zeroed::<LR_NonZero>(),
"attempted to zero-initialize type `LR_NonZero`, which is invalid"
);

test_panic_msg(
|| mem::zeroed::<ManuallyDrop<LR_NonZero>>(),
"attempted to zero-initialize type `core::mem::manually_drop::ManuallyDrop<LR_NonZero>`, \
which is invalid"
);
} else {
// These are UB because they have not been officially blessed, but we await the resolution
// of <https://github.com/rust-lang/unsafe-code-guidelines/issues/71> before doing
// anything about that.
let _val = mem::uninitialized::<i32>();
let _val = mem::uninitialized::<*const ()>();

// These are UB, but best to test them to ensure we don't become unintentionally
// stricter.

// It's currently unchecked to create invalid enums and values inside arrays.
let _val = mem::zeroed::<LR_NonZero>();
let _val = mem::zeroed::<[LR_NonZero; 1]>();
let _val = mem::zeroed::<[NonNull<()>; 1]>();
let _val = mem::uninitialized::<[NonNull<()>; 1]>();
}
// These are UB because they have not been officially blessed, but we await the resolution
// of <https://github.com/rust-lang/unsafe-code-guidelines/issues/71> before doing
// anything about that.
test_strict_panic_msg(
|| mem::uninitialized::<i32>(),
"attempted to leave type `i32` uninitialized, which is invalid"
);

test_strict_panic_msg(
|| mem::uninitialized::<*const ()>(),
"attempted to leave type `*const ()` uninitialized, which is invalid"
);

test_strict_panic_msg(
|| mem::uninitialized::<[i32; 1]>(),
"attempted to leave type `[i32; 1]` uninitialized, which is invalid"
);

// These are UB, but making them panic breaks too many crates at the moment.
test_strict_panic_msg(
|| mem::zeroed::<[(&[u8], &str); 1]>(),
"attempted to zero-initialize type `[(&[u8], &str); 1]`, which is invalid"
);

test_strict_panic_msg(
|| mem::uninitialized::<[(&[u8], &str); 1]>(),
"attempted to leave type `[(&[u8], &str); 1]` uninitialized, which is invalid"
);

test_strict_panic_msg(
|| mem::zeroed::<[NonNull<()>; 1]>(),
"attempted to zero-initialize type `[core::ptr::non_null::NonNull<()>; 1]`, which is invalid"
);

test_strict_panic_msg(
|| mem::zeroed::<LR_NonZero>(),
"attempted to zero-initialize type `LR_NonZero`, which is invalid"
);

test_strict_panic_msg(
|| mem::zeroed::<[LR_NonZero; 1]>(),
"attempted to zero-initialize type `[LR_NonZero; 1]`, which is invalid"
);

test_strict_panic_msg(
|| mem::zeroed::<ManuallyDrop<LR_NonZero>>(),
"attempted to zero-initialize type `core::mem::manually_drop::ManuallyDrop<LR_NonZero>`, \
which is invalid"
);
}
}