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

MIR validation: reject in-place argument/return for packed fields #115164

Merged
merged 1 commit into from
Aug 28, 2023
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
31 changes: 28 additions & 3 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use rustc_mir_dataflow::{Analysis, ResultsCursor};
use rustc_target::abi::{Size, FIRST_VARIANT};
use rustc_target::spec::abi::Abi;

use crate::util::is_within_packed;

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
enum EdgeKind {
Unwind,
Expand Down Expand Up @@ -93,6 +95,7 @@ impl<'tcx> MirPass<'tcx> for Validator {
cfg_checker.visit_body(body);
cfg_checker.check_cleanup_control_flow();

// Also run the TypeChecker.
for (location, msg) in validate_types(tcx, self.mir_phase, param_env, body) {
cfg_checker.fail(location, msg);
}
Expand Down Expand Up @@ -418,22 +421,42 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
self.check_unwind_edge(location, *unwind);

// The call destination place and Operand::Move place used as an argument might be
// passed by a reference to the callee. Consequently they must be non-overlapping.
// Currently this simply checks for duplicate places.
// passed by a reference to the callee. Consequently they must be non-overlapping
// and cannot be packed. Currently this simply checks for duplicate places.
self.place_cache.clear();
self.place_cache.insert(destination.as_ref());
if is_within_packed(self.tcx, &self.body.local_decls, *destination).is_some() {
// This is bad! The callee will expect the memory to be aligned.
self.fail(
location,
format!(
"encountered packed place in `Call` terminator destination: {:?}",
terminator.kind,
),
);
}
let mut has_duplicates = false;
for arg in args {
if let Operand::Move(place) = arg {
has_duplicates |= !self.place_cache.insert(place.as_ref());
if is_within_packed(self.tcx, &self.body.local_decls, *place).is_some() {
Copy link
Member Author

@RalfJung RalfJung Aug 24, 2023

Choose a reason for hiding this comment

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

@cjgillot right now this is more strict than your check in #115138. Your pass might introduce move of packed fields if the field happens to not require more alignment anyway (such as a u8 in a packed struct). However determining that requires a param_env and then I'd have to move this check into the TypeChecker... is it really worth allowing such moves?

Copy link
Contributor

Choose a reason for hiding this comment

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

My patch uses is_disaligned for simplicity. No objection to using is_with_packed: as simple and more secure.

// This is bad! The callee will expect the memory to be aligned.
self.fail(
location,
format!(
"encountered `Move` of a packed place in `Call` terminator: {:?}",
terminator.kind,
),
);
}
}
}

if has_duplicates {
self.fail(
location,
format!(
"encountered overlapping memory in `Call` terminator: {:?}",
"encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
terminator.kind,
),
);
Expand Down Expand Up @@ -532,6 +555,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
}
}

/// A faster version of the validation pass that only checks those things which may break when apply
/// generic substitutions.
pub fn validate_types<'tcx>(
tcx: TyCtxt<'tcx>,
mir_phase: MirPhase,
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_const_eval/src/util/alignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ where
false
}
_ => {
// We cannot figure out the layout. Conservatively assume that this is disaligned.
debug!("is_disaligned({:?}) - true", place);
true
}
}
}

fn is_within_packed<'tcx, L>(
pub fn is_within_packed<'tcx, L>(
tcx: TyCtxt<'tcx>,
local_decls: &L,
place: Place<'tcx>,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ mod check_validity_requirement;
mod compare_types;
mod type_name;

pub use self::alignment::is_disaligned;
pub use self::alignment::{is_disaligned, is_within_packed};
pub use self::check_validity_requirement::check_validity_requirement;
pub use self::compare_types::{is_equal_up_to_subtyping, is_subtype};
pub use self::type_name::type_name;
Expand Down
Loading