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

miri: protect Move() function arguments during the call #113569

Merged
merged 9 commits into from
Jul 12, 2023
12 changes: 7 additions & 5 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use rustc_target::spec::abi::Abi as CallAbi;

use crate::errors::{LongRunning, LongRunningWarn};
use crate::interpret::{
self, compile_time_machine, AllocId, ConstAllocation, FnVal, Frame, ImmTy, InterpCx,
self, compile_time_machine, AllocId, ConstAllocation, FnArg, FnVal, Frame, ImmTy, InterpCx,
InterpResult, OpTy, PlaceTy, Pointer, Scalar,
};
use crate::{errors, fluent_generated as fluent};
Expand Down Expand Up @@ -201,7 +201,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
fn hook_special_const_fn(
&mut self,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx>],
args: &[FnArg<'tcx>],
dest: &PlaceTy<'tcx>,
ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
Expand All @@ -210,6 +210,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
if Some(def_id) == self.tcx.lang_items().panic_display()
|| Some(def_id) == self.tcx.lang_items().begin_panic_fn()
{
let args = self.copy_fn_args(args)?;
// &str or &&str
assert!(args.len() == 1);

Expand All @@ -236,8 +237,9 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {

return Ok(Some(new_instance));
} else if Some(def_id) == self.tcx.lang_items().align_offset_fn() {
let args = self.copy_fn_args(args)?;
// For align_offset, we replace the function call if the pointer has no address.
match self.align_offset(instance, args, dest, ret)? {
match self.align_offset(instance, &args, dest, ret)? {
ControlFlow::Continue(()) => return Ok(Some(instance)),
ControlFlow::Break(()) => return Ok(None),
}
Expand Down Expand Up @@ -293,7 +295,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
self.eval_fn_call(
FnVal::Instance(instance),
(CallAbi::Rust, fn_abi),
&[addr, align],
&[FnArg::Copy(addr), FnArg::Copy(align)],
/* with_caller_location = */ false,
dest,
ret,
Expand Down Expand Up @@ -427,7 +429,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
_abi: CallAbi,
args: &[OpTy<'tcx>],
args: &[FnArg<'tcx>],
dest: &PlaceTy<'tcx>,
ret: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction, // unwinding is not supported in consts
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,11 +682,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
return_to_block: StackPopCleanup,
) -> InterpResult<'tcx> {
trace!("body: {:#?}", body);
// Clobber previous return place contents, nobody is supposed to be able to see them any more
// This also checks dereferenceable, but not align. We rely on all constructed places being
// sufficiently aligned (in particular we rely on `deref_operand` checking alignment).
self.write_uninit(return_place)?;
// first push a stack frame so we have access to the local substs
// First push a stack frame so we have access to the local substs
let pre_frame = Frame {
body,
loc: Right(body.span), // Span used for errors caused during preamble.
Expand Down Expand Up @@ -805,6 +801,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
throw_ub_custom!(fluent::const_eval_unwind_past_top);
}

M::before_stack_pop(self, self.frame())?;

// Copy return value. Must of course happen *before* we deallocate the locals.
let copy_ret_result = if !unwinding {
let op = self
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use super::{
use crate::const_eval;
use crate::errors::{DanglingPtrInFinal, UnsupportedUntypedPointer};

pub trait CompileTimeMachine<'mir, 'tcx, T> = Machine<
pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
'mir,
'tcx,
MemoryKind = T,
Expand Down
30 changes: 25 additions & 5 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rustc_target::spec::abi::Abi as CallAbi;
use crate::const_eval::CheckAlignment;

use super::{
AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, Frame, ImmTy, InterpCx,
AllocBytes, AllocId, AllocRange, Allocation, ConstAllocation, FnArg, Frame, ImmTy, InterpCx,
InterpResult, MemoryKind, OpTy, Operand, PlaceTy, Pointer, Provenance, Scalar,
};

Expand Down Expand Up @@ -84,7 +84,7 @@ pub trait AllocMap<K: Hash + Eq, V> {

/// Methods of this trait signifies a point where CTFE evaluation would fail
/// and some use case dependent behaviour can instead be applied.
pub trait Machine<'mir, 'tcx>: Sized {
pub trait Machine<'mir, 'tcx: 'mir>: Sized {
/// Additional memory kinds a machine wishes to distinguish from the builtin ones
type MemoryKind: Debug + std::fmt::Display + MayLeak + Eq + 'static;

Expand Down Expand Up @@ -182,7 +182,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
ecx: &mut InterpCx<'mir, 'tcx, Self>,
instance: ty::Instance<'tcx>,
abi: CallAbi,
args: &[OpTy<'tcx, Self::Provenance>],
args: &[FnArg<'tcx, Self::Provenance>],
destination: &PlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
Expand All @@ -194,7 +194,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
ecx: &mut InterpCx<'mir, 'tcx, Self>,
fn_val: Self::ExtraFnVal,
abi: CallAbi,
args: &[OpTy<'tcx, Self::Provenance>],
args: &[FnArg<'tcx, Self::Provenance>],
destination: &PlaceTy<'tcx, Self::Provenance>,
target: Option<mir::BasicBlock>,
unwind: mir::UnwindAction,
Expand Down Expand Up @@ -418,6 +418,18 @@ pub trait Machine<'mir, 'tcx>: Sized {
Ok(())
}

/// Called on places used for in-place function argument and return value handling.
///
/// These places need to be protected to make sure the program cannot tell whether the
/// argument/return value was actually copied or passed in-place..
fn protect_in_place_function_argument(
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'm not happy with the name for this machine hook, since it is also used for return places ("output arguments"). Any suggestions?

Also I wonder if I should move the write_uninit out of this hook and into the place where it is called... can't imagine an implementation of this hook that wouldn't want to do write_uninit.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems similarly fragile to have to repeat it at both call sites, so 🤷 I don't have a preference.

Arguments seems fine, it's a bit odd from the high level language perspective, but at the MIR level it all becomes a bit confusing anyway.

ecx: &mut InterpCx<'mir, 'tcx, Self>,
place: &PlaceTy<'tcx, Self::Provenance>,
) -> InterpResult<'tcx> {
// Without an aliasing model, all we can do is put `Uninit` into the place.
ecx.write_uninit(place)
}

/// Called immediately before a new stack frame gets pushed.
fn init_frame_extra(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
Expand All @@ -439,6 +451,14 @@ pub trait Machine<'mir, 'tcx>: Sized {
Ok(())
}

/// Called just before the return value is copied to the caller-provided return place.
fn before_stack_pop(
_ecx: &InterpCx<'mir, 'tcx, Self>,
_frame: &Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
) -> InterpResult<'tcx> {
Ok(())
}

/// Called immediately after a stack frame got popped, but before jumping back to the caller.
/// The `locals` have already been destroyed!
fn after_stack_pop(
Expand Down Expand Up @@ -484,7 +504,7 @@ pub macro compile_time_machine(<$mir: lifetime, $tcx: lifetime>) {
_ecx: &mut InterpCx<$mir, $tcx, Self>,
fn_val: !,
_abi: CallAbi,
_args: &[OpTy<$tcx>],
_args: &[FnArg<$tcx>],
_destination: &PlaceTy<$tcx, Self::Provenance>,
_target: Option<mir::BasicBlock>,
_unwind: mir::UnwindAction,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_const_eval/src/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackP
pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};
pub use self::operand::{ImmTy, Immediate, OpTy, Operand};
pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy};
pub use self::terminator::FnArg;
pub use self::validity::{CtfeValidationMode, RefTracking};
pub use self::visitor::{MutValueVisitor, Value, ValueVisitor};

Expand Down
8 changes: 0 additions & 8 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,14 +575,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(op)
}

/// Evaluate a bunch of operands at once
pub(super) fn eval_operands(
&self,
ops: &[mir::Operand<'tcx>],
) -> InterpResult<'tcx, Vec<OpTy<'tcx, M::Provenance>>> {
ops.iter().map(|op| self.eval_operand(op, None)).collect()
}

fn eval_ty_constant(
&self,
val: ty::Const<'tcx>,
Expand Down
22 changes: 12 additions & 10 deletions compiler/rustc_const_eval/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -354,25 +354,27 @@ where
#[inline]
pub(super) fn get_place_alloc(
&self,
place: &MPlaceTy<'tcx, M::Provenance>,
mplace: &MPlaceTy<'tcx, M::Provenance>,
) -> InterpResult<'tcx, Option<AllocRef<'_, 'tcx, M::Provenance, M::AllocExtra, M::Bytes>>>
{
assert!(place.layout.is_sized());
assert!(!place.meta.has_meta());
let size = place.layout.size;
self.get_ptr_alloc(place.ptr, size, place.align)
let (size, _align) = self
.size_and_align_of_mplace(&mplace)?
.unwrap_or((mplace.layout.size, mplace.layout.align.abi));
// Due to packed places, only `mplace.align` matters.
self.get_ptr_alloc(mplace.ptr, size, mplace.align)
}

#[inline]
pub(super) fn get_place_alloc_mut(
&mut self,
place: &MPlaceTy<'tcx, M::Provenance>,
mplace: &MPlaceTy<'tcx, M::Provenance>,
) -> InterpResult<'tcx, Option<AllocRefMut<'_, 'tcx, M::Provenance, M::AllocExtra, M::Bytes>>>
{
assert!(place.layout.is_sized());
assert!(!place.meta.has_meta());
let size = place.layout.size;
self.get_ptr_alloc_mut(place.ptr, size, place.align)
let (size, _align) = self
.size_and_align_of_mplace(&mplace)?
.unwrap_or((mplace.layout.size, mplace.layout.align.abi));
// Due to packed places, only `mplace.align` matters.
self.get_ptr_alloc_mut(mplace.ptr, size, mplace.align)
}

/// Check if this mplace is dereferenceable and sufficiently aligned.
Expand Down