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

Introduce stricter checks for might_permit_raw_init under a debug flag #97323

Merged
merged 1 commit into from
May 25, 2022
Merged
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
15 changes: 13 additions & 2 deletions compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
@@ -58,6 +58,7 @@ pub(crate) use llvm::codegen_llvm_intrinsic_call;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::subst::SubstsRef;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_target::abi::InitKind;

use crate::prelude::*;
use cranelift_codegen::ir::AtomicRmwOp;
@@ -671,7 +672,12 @@ fn codegen_regular_intrinsic_call<'tcx>(
return;
}

if intrinsic == sym::assert_zero_valid && !layout.might_permit_raw_init(fx, /*zero:*/ true) {
if intrinsic == sym::assert_zero_valid
&& !layout.might_permit_raw_init(
fx,
InitKind::Zero,
fx.tcx.sess.opts.debugging_opts.strict_init_checks) {

with_no_trimmed_paths!({
crate::base::codegen_panic(
fx,
@@ -682,7 +688,12 @@ fn codegen_regular_intrinsic_call<'tcx>(
return;
}

if intrinsic == sym::assert_uninit_valid && !layout.might_permit_raw_init(fx, /*zero:*/ false) {
if intrinsic == sym::assert_uninit_valid
&& !layout.might_permit_raw_init(
fx,
InitKind::Uninit,
fx.tcx.sess.opts.debugging_opts.strict_init_checks) {

with_no_trimmed_paths!({
crate::base::codegen_panic(
fx,
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ use rustc_span::source_map::Span;
use rustc_span::{sym, Symbol};
use rustc_symbol_mangling::typeid_for_fnabi;
use rustc_target::abi::call::{ArgAbi, FnAbi, PassMode};
use rustc_target::abi::{self, HasDataLayout, WrappingRange};
use rustc_target::abi::{self, HasDataLayout, InitKind, WrappingRange};
use rustc_target::spec::abi::Abi;

/// Used by `FunctionCx::codegen_terminator` for emitting common patterns
@@ -521,6 +521,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
source_info: mir::SourceInfo,
target: Option<mir::BasicBlock>,
cleanup: Option<mir::BasicBlock>,
strict_validity: bool,
) -> bool {
// Emit a panic or a no-op for `assert_*` intrinsics.
// These are intrinsics that compile to panics so that we can get a message
@@ -543,8 +544,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let layout = bx.layout_of(ty);
let do_panic = match intrinsic {
Inhabited => layout.abi.is_uninhabited(),
ZeroValid => !layout.might_permit_raw_init(bx, /*zero:*/ true),
UninitValid => !layout.might_permit_raw_init(bx, /*zero:*/ false),
ZeroValid => !layout.might_permit_raw_init(bx, InitKind::Zero, strict_validity),
UninitValid => !layout.might_permit_raw_init(bx, InitKind::Uninit, strict_validity),
};
if do_panic {
let msg_str = with_no_visible_paths!({
@@ -678,6 +679,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
source_info,
target,
cleanup,
self.cx.tcx().sess.opts.debugging_opts.strict_init_checks,
) {
return;
}
14 changes: 11 additions & 3 deletions compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ use rustc_middle::ty::layout::LayoutOf as _;
use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_span::symbol::{sym, Symbol};
use rustc_target::abi::{Abi, Align, Primitive, Size};
use rustc_target::abi::{Abi, Align, InitKind, Primitive, Size};

use super::{
util::ensure_monomorphic_enough, CheckInAllocMsg, ImmTy, InterpCx, Machine, OpTy, PlaceTy,
@@ -408,7 +408,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
)?;
}
if intrinsic_name == sym::assert_zero_valid
&& !layout.might_permit_raw_init(self, /*zero:*/ true)
&& !layout.might_permit_raw_init(
self,
InitKind::Zero,
self.tcx.sess.opts.debugging_opts.strict_init_checks,
)
{
M::abort(
self,
@@ -419,7 +423,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
)?;
}
if intrinsic_name == sym::assert_uninit_valid
&& !layout.might_permit_raw_init(self, /*zero:*/ false)
&& !layout.might_permit_raw_init(
self,
InitKind::Uninit,
self.tcx.sess.opts.debugging_opts.strict_init_checks,
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhat annoying to pass this everywhere, when self is also passed. Though HasDataLayout doesn't have a useful way to get at this.

maybe add a field to TargetDataLayout for that bool?

Idk, doesn't seem great either. I guess let's roll with the PR as it is.

)
{
M::abort(
self,
2 changes: 2 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
@@ -1495,6 +1495,8 @@ options! {
"hash algorithm of source files in debug info (`md5`, `sha1`, or `sha256`)"),
stack_protector: StackProtector = (StackProtector::None, parse_stack_protector, [TRACKED],
"control stack smash protection strategy (`rustc --print stack-protector-strategies` for details)"),
strict_init_checks: bool = (false, parse_bool, [TRACKED],
"control if mem::uninitialized and mem::zeroed panic on more UB"),
strip: Strip = (Strip::None, parse_strip, [UNTRACKED],
"tell the linker which information to strip (`none` (default), `debuginfo` or `symbols`)"),
split_dwarf_kind: SplitDwarfKind = (SplitDwarfKind::Split, parse_split_dwarf_kind, [UNTRACKED],
59 changes: 47 additions & 12 deletions compiler/rustc_target/src/abi/mod.rs
Original file line number Diff line number Diff line change
@@ -894,6 +894,15 @@ impl Scalar {
Scalar::Union { .. } => true,
}
}

/// Returns `true` if this type can be left uninit.
#[inline]
pub fn is_uninit_valid(&self) -> bool {
match *self {
Scalar::Initialized { .. } => false,
Scalar::Union { .. } => true,
}
}
}

/// Describes how the fields of a type are located in memory.
@@ -1355,6 +1364,14 @@ pub struct PointeeInfo {
pub address_space: AddressSpace,
}

/// Used in `might_permit_raw_init` to indicate the kind of initialisation
/// that is checked to be valid
#[derive(Copy, Clone, Debug)]
pub enum InitKind {
Zero,
Uninit,
}

/// Trait that needs to be implemented by the higher-level type representation
/// (e.g. `rustc_middle::ty::Ty`), to provide `rustc_target::abi` functionality.
pub trait TyAbiInterface<'a, C>: Sized {
@@ -1461,26 +1478,37 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {

/// Determines if this type permits "raw" initialization by just transmuting some
/// memory into an instance of `T`.
/// `zero` indicates if the memory is zero-initialized, or alternatively
/// left entirely uninitialized.
///
/// `init_kind` indicates if the memory is zero-initialized or left uninitialized.
///
/// `strict` is an opt-in debugging flag added in #97323 that enables more checks.
///
/// This is conservative: in doubt, it will answer `true`.
///
/// FIXME: Once we removed all the conservatism, we could alternatively
/// create an all-0/all-undef constant and run the const value validator to see if
/// this is a valid value for the given type.
pub fn might_permit_raw_init<C>(self, cx: &C, zero: bool) -> bool
pub fn might_permit_raw_init<C>(self, cx: &C, init_kind: InitKind, strict: bool) -> bool
where
Self: Copy,
Ty: TyAbiInterface<'a, C>,
C: HasDataLayout,
{
let scalar_allows_raw_init = move |s: Scalar| -> bool {
if zero {
// The range must contain 0.
s.valid_range(cx).contains(0)
} else {
// The range must include all values.
s.is_always_valid(cx)
match init_kind {
InitKind::Zero => {
// The range must contain 0.
s.valid_range(cx).contains(0)
}
InitKind::Uninit => {
if strict {
// The type must be allowed to be uninit (which means "is a union").
s.is_uninit_valid()
} else {
// The range must include all values.
s.is_always_valid(cx)
}
}
}
};

@@ -1500,12 +1528,19 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
// 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.
FieldsShape::Array { count, .. } => {
// FIXME(#66151): For now, we are conservative and do not check arrays by default.
if strict
&& *count > 0
&& !self.field(cx, 0).might_permit_raw_init(cx, init_kind, strict)
{
// 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 !self.field(cx, idx).might_permit_raw_init(cx, zero) {
if !self.field(cx, idx).might_permit_raw_init(cx, init_kind, strict) {
// We found a field that is unhappy with this kind of initialization.
return false;
}
44 changes: 38 additions & 6 deletions src/test/ui/intrinsics/panic-uninitialized-zeroed.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// run-pass
// needs-unwind
// ignore-wasm32-bare compiled with panic=abort by default
// revisions: mir thir
// revisions: mir thir strict
// [thir]compile-flags: -Zthir-unsafeck
// [strict]compile-flags: -Zstrict-init-checks
// ignore-tidy-linelength

// This test checks panic emitted from `mem::{uninitialized,zeroed}`.
@@ -54,6 +55,8 @@ enum LR_NonZero {
Right(num::NonZeroI64),
}

struct ZeroSized;

fn test_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) {
let err = panic::catch_unwind(op).err();
assert_eq!(
@@ -228,11 +231,40 @@ fn main() {
let _val = mem::zeroed::<[!; 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"
);

// 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 ()>();
test_panic_msg(
|| mem::zeroed::<[NonNull<()>; 1]>(),
"attempted to zero-initialize type `[core::ptr::non_null::NonNull<()>; 1]`, 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 ()>();
}
}
}