Skip to content
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.

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

panic-on-uninit: adjust checks to 0x01-filling #101061

Merged
merged 1 commit into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions compiler/rustc_const_eval/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ extern crate rustc_middle;
pub mod const_eval;
mod errors;
pub mod interpret;
mod might_permit_raw_init;
pub mod transform;
pub mod util;

Expand Down Expand Up @@ -61,7 +60,6 @@ pub fn provide(providers: &mut Providers) {
const_eval::deref_mir_constant(tcx, param_env, value)
};
providers.permits_uninit_init =
|tcx, ty| might_permit_raw_init::might_permit_raw_init(tcx, ty, InitKind::Uninit);
providers.permits_zero_init =
|tcx, ty| might_permit_raw_init::might_permit_raw_init(tcx, ty, InitKind::Zero);
|tcx, ty| util::might_permit_raw_init(tcx, ty, InitKind::UninitMitigated0x01Fill);
providers.permits_zero_init = |tcx, ty| util::might_permit_raw_init(tcx, ty, InitKind::Zero);
}
44 changes: 0 additions & 44 deletions compiler/rustc_const_eval/src/might_permit_raw_init.rs

This file was deleted.

151 changes: 151 additions & 0 deletions compiler/rustc_const_eval/src/util/might_permit_raw_init.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
use rustc_middle::ty::layout::{LayoutCx, LayoutOf, TyAndLayout};
use rustc_middle::ty::{ParamEnv, TyCtxt};
use rustc_session::Limit;
use rustc_target::abi::{Abi, FieldsShape, InitKind, Scalar, Variants};

use crate::const_eval::CompileTimeInterpreter;
use crate::interpret::{InterpCx, MemoryKind, OpTy};

/// 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. We assume
/// uninitialized memory is mitigated by filling it with 0x01, which reduces the chance of causing
/// LLVM UB.
///
/// By default we check whether that operation would cause *LLVM UB*, i.e., whether the LLVM IR we
/// generate has UB or not. This is a mitigation strategy, which is why we are okay with accepting
/// Rust UB as long as there is no risk of miscompilations. The `strict_init_checks` can be set to
/// do a full check against Rust UB instead (in which case we will also ignore the 0x01-filling and
/// to the full uninit check).
pub fn might_permit_raw_init<'tcx>(
tcx: TyCtxt<'tcx>,
ty: TyAndLayout<'tcx>,
kind: InitKind,
) -> bool {
if tcx.sess.opts.unstable_opts.strict_init_checks {
might_permit_raw_init_strict(ty, tcx, kind)
} else {
let layout_cx = LayoutCx { tcx, param_env: ParamEnv::reveal_all() };
might_permit_raw_init_lax(ty, &layout_cx, kind)
}
}

/// Implements the 'strict' version of the `might_permit_raw_init` checks; see that function for
/// details.
fn might_permit_raw_init_strict<'tcx>(
ty: TyAndLayout<'tcx>,
tcx: TyCtxt<'tcx>,
kind: InitKind,
) -> bool {
let machine = CompileTimeInterpreter::new(
Limit::new(0),
/*can_access_statics:*/ false,
/*check_alignment:*/ true,
);

let mut cx = InterpCx::new(tcx, rustc_span::DUMMY_SP, ParamEnv::reveal_all(), machine);

let allocated = cx
.allocate(ty, MemoryKind::Machine(crate::const_eval::MemoryKind::Heap))
.expect("OOM: failed to allocate for uninit check");

if kind == InitKind::Zero {
cx.write_bytes_ptr(
allocated.ptr,
std::iter::repeat(0_u8).take(ty.layout.size().bytes_usize()),
)
.expect("failed to write bytes for zero valid check");
}

let ot: OpTy<'_, _> = allocated.into();

// Assume that if it failed, it's a validation failure.
// This does *not* actually check that references are dereferenceable, but since all types that
// require dereferenceability also require non-null, we don't actually get any false negatives
// due to this.
cx.validate_operand(&ot).is_ok()
}

/// Implements the 'lax' (default) version of the `might_permit_raw_init` checks; see that function for
/// details.
fn might_permit_raw_init_lax<'tcx>(
this: TyAndLayout<'tcx>,
cx: &LayoutCx<'tcx, TyCtxt<'tcx>>,
init_kind: InitKind,
) -> bool {
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::UninitMitigated0x01Fill => {
// The range must include an 0x01-filled buffer.
let mut val: u128 = 0x01;
for _ in 1..s.size(cx).bytes() {
// For sizes >1, repeat the 0x01.
val = (val << 8) | 0x01;
}
s.valid_range(cx).contains(val)
}
}
};

// 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;
}

// Special magic check for references and boxes (i.e., special pointer types).
if let Some(pointee) = this.ty.builtin_deref(false) {
let pointee = cx.layout_of(pointee.ty).expect("need to be able to compute layouts");
// We need to ensure that the LLVM attributes `aligned` and `dereferenceable(size)` are satisfied.
if pointee.align.abi.bytes() > 1 {
// 0x01-filling is not aligned.
return false;
}
if pointee.size.bytes() > 0 {
// A 'fake' integer pointer is not sufficiently dereferenceable.
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 { .. } => {
// Arrays never have scalar layout in LLVM, so if the array is not actually
// accessed, there is no LLVM UB -- therefore we can skip this.
}
FieldsShape::Arbitrary { offsets, .. } => {
for idx in 0..offsets.len() {
if !might_permit_raw_init_lax(this.field(cx, idx), cx, init_kind) {
// We found a field that is unhappy with this kind of initialization.
return false;
}
}
}
}

match &this.variants {
Variants::Single { .. } => {
// All fields of this single variant have already been checked above, there is nothing
// else to do.
}
Variants::Multiple { .. } => {
// We cannot tell LLVM anything about the details of this multi-variant layout, so
// invalid values "hidden" inside the variant cannot cause LLVM trouble.
}
}

true
}
2 changes: 2 additions & 0 deletions compiler/rustc_const_eval/src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ mod alignment;
mod call_kind;
pub mod collect_writes;
mod find_self_call;
mod might_permit_raw_init;

pub use self::aggregate::expand_aggregate;
pub use self::alignment::is_disaligned;
pub use self::call_kind::{call_kind, CallDesugaringKind, CallKind};
pub use self::find_self_call::find_self_call;
pub use self::might_permit_raw_init::might_permit_raw_init;
70 changes: 1 addition & 69 deletions compiler/rustc_target/src/abi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,7 @@ pub struct PointeeInfo {
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum InitKind {
Zero,
Uninit,
UninitMitigated0x01Fill,
}

/// Trait that needs to be implemented by the higher-level type representation
Expand Down Expand Up @@ -1498,72 +1498,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)
Copy link
Member

Choose a reason for hiding this comment

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

Should we note references here? 0x1-filling is non-null, but still not dereferenceable (as mentioned) and also not aligned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot about alignment... it would be handled seamlessly here if we used 0x00..align as the niche, but right now we only have 0x00..0x01 so we'd not complain about e.g. &i32.

IMO the best way to fix that is to make the niche bigger...

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why the niche matters, when a 0x1-filled reference means it has pointer 0x0101_0101_0101_0101.

Copy link
Member

Choose a reason for hiding this comment

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

We might want to extend the niche of references from the current 0x00..0x01 to 0x00..align in the future, which would produce invalid values even with 0x01 filling.

Copy link
Member Author

Choose a reason for hiding this comment

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

might_permit_raw_init uses the same information that is used for niches to determine whether a given value is valid for a given type.

Currently, that information says that 0x0101_0101_0101_0101 is valid for &i32, which leads to might_permit_raw_init allowing &i32 to be left uninit with 0x01-filling.

If we extend the niche to take into account alignment, that information will say that 0x0101_0101_0101_0101 is not valid for &i32, and hence trying to create a 0x01-filled &i32 would be rejected (as it should).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but those extended niche values are still far from the filled value we're talking about, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we had a way to represent a niche that is "all the unaligned values", then that would be sufficient for this check -- 0x0101_0101_0101_0101 would be in the niche and hence disallowed for &i32.

But indeed we cannot even represent such niches, so contrary to what I said earlier we cannot rely on niches to detect the trouble around references.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, my last reply was having only seen what @Nilstrieb wrote - I guess GitHub failed to dynamically load @RalfJung's replies - but I think we're on the same page now.

So, is this something we should still worry about for LLVM UB in this context? We do tell LLVM about align, and their docs say, "If the pointer value does not have the specified alignment, poison value is returned or passed instead." But AIUI that's not UB until it's used in a particular way, so is it still a problem here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think either @Nilstrieb or @5225225 already had a version of might_permit_raw_init that takes into account reference type information somewhere.

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;
}
}
}

This? It's in my #99389 PR where we allowed 1-aligned references with no dereferencable (inside arrays only)

Copy link
Member Author

@RalfJung RalfJung Sep 8, 2022

Choose a reason for hiding this comment

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

@cuviper poison value basically means uninit memory, which the 0x01-filling is intended to avoid, so I am leaning towards saying that this is LLVM UB we should avoid. (In particular, align noundef would make it insta-UB -- and that is the combination of attributes we want.)

@5225225 yes, that one, thanks!

/// * 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
}
}
9 changes: 6 additions & 3 deletions src/test/ui/consts/assert-type-intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ fn main() {
use std::mem::MaybeUninit;

const _BAD1: () = unsafe {
MaybeUninit::<!>::uninit().assume_init();
intrinsics::assert_inhabited::<!>(); //~ERROR: any use of this value will cause an error
//~^WARN: previously accepted
};
const _BAD2: () = {
intrinsics::assert_uninit_valid::<bool>();
intrinsics::assert_uninit_valid::<!>(); //~ERROR: any use of this value will cause an error
//~^WARN: previously accepted
};
const _BAD3: () = {
intrinsics::assert_zero_valid::<&'static i32>();
intrinsics::assert_zero_valid::<&'static i32>(); //~ERROR: any use of this value will cause an error
//~^WARN: previously accepted
};
}
24 changes: 12 additions & 12 deletions src/test/ui/consts/assert-type-intrinsics.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,26 @@ error: any use of this value will cause an error
|
LL | const _BAD1: () = unsafe {
| ---------------
LL | MaybeUninit::<!>::uninit().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
LL | intrinsics::assert_inhabited::<!>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>
= note: `#[deny(const_err)]` on by default

error: any use of this value will cause an error
--> $DIR/assert-type-intrinsics.rs:17:9
--> $DIR/assert-type-intrinsics.rs:18:9
|
LL | const _BAD2: () = {
| ---------------
LL | intrinsics::assert_uninit_valid::<bool>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to leave type `bool` uninitialized, which is invalid
LL | intrinsics::assert_uninit_valid::<!>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error: any use of this value will cause an error
--> $DIR/assert-type-intrinsics.rs:20:9
--> $DIR/assert-type-intrinsics.rs:22:9
|
LL | const _BAD3: () = {
| ---------------
Expand All @@ -40,29 +40,29 @@ error: any use of this value will cause an error
|
LL | const _BAD1: () = unsafe {
| ---------------
LL | MaybeUninit::<!>::uninit().assume_init();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
LL | intrinsics::assert_inhabited::<!>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>
= note: `#[deny(const_err)]` on by default

Future breakage diagnostic:
error: any use of this value will cause an error
--> $DIR/assert-type-intrinsics.rs:17:9
--> $DIR/assert-type-intrinsics.rs:18:9
|
LL | const _BAD2: () = {
| ---------------
LL | intrinsics::assert_uninit_valid::<bool>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to leave type `bool` uninitialized, which is invalid
LL | intrinsics::assert_uninit_valid::<!>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ aborted execution: attempted to instantiate uninhabited type `!`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>
= note: `#[deny(const_err)]` on by default

Future breakage diagnostic:
error: any use of this value will cause an error
--> $DIR/assert-type-intrinsics.rs:20:9
--> $DIR/assert-type-intrinsics.rs:22:9
|
LL | const _BAD3: () = {
| ---------------
Expand Down
Loading