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

Lint on some incorrect uses of mem::zeroed / mem::uninitialized #63346

Merged
merged 9 commits into from
Aug 11, 2019
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
3 changes: 3 additions & 0 deletions src/libcore/mem/maybe_uninit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::mem::ManuallyDrop;
/// ever gets used to access memory:
///
/// ```rust,no_run
/// # #![allow(invalid_value)]
/// use std::mem::{self, MaybeUninit};
///
/// let x: &i32 = unsafe { mem::zeroed() }; // undefined behavior!
Expand All @@ -27,6 +28,7 @@ use crate::mem::ManuallyDrop;
/// always be `true` or `false`. Hence, creating an uninitialized `bool` is undefined behavior:
///
/// ```rust,no_run
/// # #![allow(invalid_value)]
/// use std::mem::{self, MaybeUninit};
///
/// let b: bool = unsafe { mem::uninitialized() }; // undefined behavior!
Expand All @@ -40,6 +42,7 @@ use crate::mem::ManuallyDrop;
/// which otherwise can hold any *fixed* bit pattern:
///
/// ```rust,no_run
/// # #![allow(invalid_value)]
/// use std::mem::{self, MaybeUninit};
///
/// let x: i32 = unsafe { mem::uninitialized() }; // undefined behavior!
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,8 @@ pub const fn needs_drop<T>() -> bool {
///
/// *Incorrect* usage of this function: initializing a reference with zero.
///
/// ```no_run
/// ```rust,no_run
/// # #![allow(invalid_value)]
/// use std::mem;
///
/// let _x: &i32 = unsafe { mem::zeroed() }; // Undefined behavior!
Expand Down
7 changes: 5 additions & 2 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1842,7 +1842,8 @@ pub struct VariantDef {
pub ctor_kind: CtorKind,
/// Flags of the variant (e.g. is field list non-exhaustive)?
flags: VariantFlags,
/// Recovered?
/// Variant is obtained as part of recovering from a syntactic error.
/// May be incomplete or bogus.
pub recovered: bool,
}

Expand Down Expand Up @@ -1949,7 +1950,7 @@ pub struct FieldDef {
pub struct AdtDef {
/// `DefId` of the struct, enum or union item.
pub did: DefId,
/// Variants of the ADT. If this is a struct or enum, then there will be a single variant.
/// Variants of the ADT. If this is a struct or union, then there will be a single variant.
pub variants: IndexVec<self::layout::VariantIdx, VariantDef>,
/// Flags of the ADT (e.g. is this a struct? is this non-exhaustive?)
flags: AdtFlags,
Expand Down Expand Up @@ -2565,6 +2566,8 @@ impl<'tcx> AdtDef {
}

impl<'tcx> FieldDef {
/// Returns the type of this field. The `subst` is typically obtained
/// via the second field of `TyKind::AdtDef`.
pub fn ty(&self, tcx: TyCtxt<'tcx>, subst: SubstsRef<'tcx>) -> Ty<'tcx> {
tcx.type_of(self.did).subst(tcx, subst)
}
Expand Down
14 changes: 12 additions & 2 deletions src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ pub enum TyKind<'tcx> {
Never,

/// A tuple type. For example, `(i32, bool)`.
/// Use `TyS::tuple_fields` to iterate over the field types.
Tuple(SubstsRef<'tcx>),

/// The projection of an associated type. For example,
Expand Down Expand Up @@ -1723,8 +1724,8 @@ impl<'tcx> TyS<'tcx> {
})
})
}
ty::Tuple(tys) => tys.iter().any(|ty| {
ty.expect_ty().conservative_is_privately_uninhabited(tcx)
ty::Tuple(..) => self.tuple_fields().any(|ty| {
ty.conservative_is_privately_uninhabited(tcx)
}),
ty::Array(ty, len) => {
match len.try_eval_usize(tcx, ParamEnv::empty()) {
Expand Down Expand Up @@ -2103,6 +2104,15 @@ impl<'tcx> TyS<'tcx> {
}
}

/// Iterates over tuple fields.
/// Panics when called on anything but a tuple.
pub fn tuple_fields(&self) -> impl DoubleEndedIterator<Item=Ty<'tcx>> {
match self.sty {
Tuple(substs) => substs.iter().map(|field| field.expect_ty()),
_ => bug!("tuple_fields called on non-tuple"),
}
}

/// If the type contains variants, returns the valid range of variant indices.
/// FIXME This requires the optimized MIR in the case of generators.
#[inline]
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -845,15 +845,15 @@ impl<'tcx> ty::TyS<'tcx> {
ty: Ty<'tcx>,
) -> Representability {
match ty.sty {
Tuple(ref ts) => {
Tuple(..) => {
// Find non representable
fold_repr(ts.iter().map(|ty| {
fold_repr(ty.tuple_fields().map(|ty| {
is_type_structurally_recursive(
tcx,
sp,
seen,
representable_cache,
ty.expect_ty(),
ty,
)
}))
}
Expand Down Expand Up @@ -1095,7 +1095,7 @@ fn needs_drop_raw<'tcx>(tcx: TyCtxt<'tcx>, query: ty::ParamEnvAnd<'tcx, Ty<'tcx>
// state transformation pass
ty::Generator(..) => true,

ty::Tuple(ref tys) => tys.iter().map(|k| k.expect_ty()).any(needs_drop),
ty::Tuple(..) => ty.tuple_fields().any(needs_drop),

// unions don't have destructors because of the child types,
// only if they manually implement `Drop` (handled above).
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/ty/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ fn push_subtypes<'tcx>(stack: &mut TypeWalkerStack<'tcx>, parent_ty: Ty<'tcx>) {
ty::GeneratorWitness(ts) => {
stack.extend(ts.skip_binder().iter().cloned().rev());
}
ty::Tuple(ts) => {
stack.extend(ts.iter().map(|k| k.expect_ty()).rev());
ty::Tuple(..) => {
stack.extend(parent_ty.tuple_fields().rev());
}
ty::FnDef(_, substs) => {
stack.extend(substs.types().rev());
Expand Down
91 changes: 90 additions & 1 deletion src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

use rustc::hir::def::{Res, DefKind};
use rustc::hir::def_id::{DefId, LOCAL_CRATE};
use rustc::ty::{self, Ty, TyCtxt};
use rustc::ty::{self, Ty, TyCtxt, layout::VariantIdx};
use rustc::{lint, util};
use hir::Node;
use util::nodemap::HirIdSet;
Expand Down Expand Up @@ -1862,3 +1862,92 @@ impl EarlyLintPass for IncompleteFeatures {
});
}
}

declare_lint! {
pub INVALID_VALUE,
Copy link
Member Author

Choose a reason for hiding this comment

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

I used the "validity" terminology here. Does that seem okay?

Warn,
"an invalid value is being created (such as a NULL reference)"
}

declare_lint_pass!(InvalidValue => [INVALID_VALUE]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &hir::Expr) {

const ZEROED_PATH: &[Symbol] = &[sym::core, sym::mem, sym::zeroed];
const UININIT_PATH: &[Symbol] = &[sym::core, sym::mem, sym::uninitialized];

/// Return `false` only if we are sure this type does *not*
/// allow zero initialization.
fn ty_maybe_allows_zero_init<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
use rustc::ty::TyKind::*;
match ty.sty {
// Primitive types that don't like 0 as a value.
Ref(..) | FnPtr(..) | Never => false,
Adt(..) if ty.is_box() => false,
// Recurse for some compound types.
Adt(adt_def, substs) if !adt_def.is_union() => {
match adt_def.variants.len() {
0 => false, // Uninhabited enum!
1 => {
// Struct, or enum with exactly one variant.
// Proceed recursively, check all fields.
let variant = &adt_def.variants[VariantIdx::from_u32(0)];
variant.fields.iter().all(|field| {
ty_maybe_allows_zero_init(
tcx,
field.ty(tcx, substs),
)
})
}
_ => true, // Conservative fallback for multi-variant enum.
}
}
Tuple(..) => {
// Proceed recursively, check all fields.
ty.tuple_fields().all(|field| ty_maybe_allows_zero_init(tcx, field))
}
// FIXME: Would be nice to also warn for `NonNull`/`NonZero*`.
// FIXME: *Only for `mem::uninitialized`*, we could also warn for `bool`,
// `char`, and any multivariant enum.
// Conservative fallback.
_ => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check for types with rustc_layout_scalar_valid_range_start here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the future this should probably check for that attribute, yes.

}
}

if let hir::ExprKind::Call(ref path_expr, ref _args) = expr.node {
if let hir::ExprKind::Path(ref qpath) = path_expr.node {
if let Some(def_id) = cx.tables.qpath_res(qpath, path_expr.hir_id).opt_def_id() {
if cx.match_def_path(def_id, &ZEROED_PATH) ||
cx.match_def_path(def_id, &UININIT_PATH)
{
// This conjures an instance of a type out of nothing,
// using zeroed or uninitialized memory.
// We are extremely conservative with what we warn about.
let conjured_ty = cx.tables.expr_ty(expr);

if !ty_maybe_allows_zero_init(cx.tcx, conjured_ty) {
cx.struct_span_lint(
INVALID_VALUE,
expr.span,
&format!(
"the type `{}` does not permit {}",
conjured_ty,
if cx.match_def_path(def_id, &ZEROED_PATH) {
"zero-initialization"
} else {
"being left uninitialized"
}
),
)
.note("this means that this code causes undefined behavior \
when executed")
.help("use `MaybeUninit` instead")
.emit();
}
}
}
}
}
}
}
1 change: 1 addition & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ macro_rules! late_lint_mod_passes {
UnreachablePub: UnreachablePub,

ExplicitOutlivesRequirements: ExplicitOutlivesRequirements,
InvalidValue: InvalidValue,
]);
)
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/shim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ fn build_clone_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, self_ty: Ty<'tcx>) -
substs.upvar_tys(def_id, tcx)
)
}
ty::Tuple(tys) => builder.tuple_like_shim(dest, src, tys.iter().map(|k| k.expect_ty())),
ty::Tuple(..) => builder.tuple_like_shim(dest, src, self_ty.tuple_fields()),
_ => {
bug!("clone shim for `{:?}` which is not `Copy` and is not an aggregate", self_ty)
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/util/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -804,8 +804,8 @@ where
let tys : Vec<_> = substs.upvar_tys(def_id, self.tcx()).collect();
self.open_drop_for_tuple(&tys)
}
ty::Tuple(tys) => {
let tys: Vec<_> = tys.iter().map(|k| k.expect_ty()).collect();
ty::Tuple(..) => {
let tys: Vec<_> = ty.tuple_fields().collect();
self.open_drop_for_tuple(&tys)
}
ty::Adt(def, substs) => {
Expand Down
3 changes: 3 additions & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ symbols! {
match_beginning_vert,
match_default_bindings,
may_dangle,
mem,
member_constraints,
message,
meta,
Expand Down Expand Up @@ -695,6 +696,7 @@ symbols! {
underscore_imports,
underscore_lifetimes,
uniform_paths,
uninitialized,
universal_impl_trait,
unmarked_api,
unreachable_code,
Expand Down Expand Up @@ -726,6 +728,7 @@ symbols! {
windows,
windows_subsystem,
Yield,
zeroed,
}
}

Expand Down
58 changes: 58 additions & 0 deletions src/test/ui/lint/uninitialized-zeroed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// ignore-tidy-linelength
// This test checks that calling `mem::{uninitialized,zeroed}` with certain types results
// in a lint.

#![feature(never_type)]
#![allow(deprecated)]
#![deny(invalid_value)]

use std::mem::{self, MaybeUninit};

enum Void {}

struct Ref(&'static i32);

struct Wrap<T> { wrapped: T }

#[allow(unused)]
fn generic<T: 'static>() {
unsafe {
let _val: &'static T = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: &'static T = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: Wrap<&'static T> = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: Wrap<&'static T> = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized
}
}

fn main() {
unsafe {
let _val: ! = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: ! = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: (i32, !) = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: (i32, !) = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: Void = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: Void = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: &'static i32 = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: &'static i32 = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: Ref = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: Ref = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: fn() = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: fn() = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

let _val: Wrap<fn()> = mem::zeroed(); //~ ERROR: does not permit zero-initialization
let _val: Wrap<fn()> = mem::uninitialized(); //~ ERROR: does not permit being left uninitialized

// Some types that should work just fine.
let _val: Option<&'static i32> = mem::zeroed();
let _val: Option<fn()> = mem::zeroed();
let _val: MaybeUninit<&'static i32> = mem::zeroed();
let _val: bool = mem::zeroed();
let _val: i32 = mem::zeroed();
}
}
Loading