Skip to content

Commit 2d81989

Browse files
committed
Auto merge of #54955 - RalfJung:miri-validate2, r=oli-obk
miri engine: Fix run-time validation This fixes all false positives that came up when actually enabling this in miri. r? @oli-obk
2 parents fb3b47a + 6426cbe commit 2d81989

16 files changed

+329
-123
lines changed

Diff for: src/Cargo.lock

+4-16
Original file line numberDiff line numberDiff line change
@@ -825,16 +825,6 @@ name = "getopts"
825825
version = "0.2.17"
826826
source = "registry+https://github.com/rust-lang/crates.io-index"
827827

828-
[[package]]
829-
name = "getset"
830-
version = "0.0.6"
831-
source = "registry+https://github.com/rust-lang/crates.io-index"
832-
dependencies = [
833-
"proc-macro2 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)",
834-
"quote 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)",
835-
"syn 0.13.11 (registry+https://github.com/rust-lang/crates.io-index)",
836-
]
837-
838828
[[package]]
839829
name = "git2"
840830
version = "0.7.5"
@@ -1303,7 +1293,7 @@ dependencies = [
13031293
"compiletest_rs 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)",
13041294
"env_logger 0.5.12 (registry+https://github.com/rust-lang/crates.io-index)",
13051295
"log 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)",
1306-
"vergen 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
1296+
"vergen 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
13071297
]
13081298

13091299
[[package]]
@@ -3072,13 +3062,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
30723062

30733063
[[package]]
30743064
name = "vergen"
3075-
version = "2.0.0"
3065+
version = "3.0.3"
30763066
source = "registry+https://github.com/rust-lang/crates.io-index"
30773067
dependencies = [
30783068
"bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
30793069
"chrono 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)",
3080-
"error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
3081-
"getset 0.0.6 (registry+https://github.com/rust-lang/crates.io-index)",
3070+
"failure 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
30823071
]
30833072

30843073
[[package]]
@@ -3245,7 +3234,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
32453234
"checksum futures 0.1.21 (registry+https://github.com/rust-lang/crates.io-index)" = "1a70b146671de62ec8c8ed572219ca5d594d9b06c0b364d5e67b722fc559b48c"
32463235
"checksum fwdansi 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "34dd4c507af68d37ffef962063dfa1944ce0dd4d5b82043dbab1dabe088610c3"
32473236
"checksum getopts 0.2.17 (registry+https://github.com/rust-lang/crates.io-index)" = "b900c08c1939860ce8b54dc6a89e26e00c04c380fd0e09796799bd7f12861e05"
3248-
"checksum getset 0.0.6 (registry+https://github.com/rust-lang/crates.io-index)" = "54c7f36a235738bb25904d6a2b3dbb28f6f5736cd3918c4bf80d6bb236200782"
32493237
"checksum git2 0.7.5 (registry+https://github.com/rust-lang/crates.io-index)" = "591f8be1674b421644b6c030969520bc3fa12114d2eb467471982ed3e9584e71"
32503238
"checksum git2-curl 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "b502f6b1b467957403d168f0039e0c46fa6a1220efa2adaef25d5b267b5fe024"
32513239
"checksum glob 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)" = "8be18de09a56b60ed0edf84bc9df007e30040691af7acd1c41874faac5895bfb"
@@ -3419,7 +3407,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
34193407
"checksum utf8-ranges 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "fd70f467df6810094968e2fce0ee1bd0e87157aceb026a8c083bcf5e25b9efe4"
34203408
"checksum vcpkg 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)" = "def296d3eb3b12371b2c7d0e83bfe1403e4db2d7a0bba324a12b21c4ee13143d"
34213409
"checksum vec_map 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "05c78687fb1a80548ae3250346c3db86a80a7cdd77bda190189f2d0a0987c81a"
3422-
"checksum vergen 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "9a16834fc61e1492c07dae49b6c14b55f8b1d43a5f5f9e9a2ecc063f47b9f93c"
3410+
"checksum vergen 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)" = "1b9696d96ec5d68984d060af80d7ba0ed4eb533978a0efb05ed4b8465f20d04f"
34233411
"checksum version_check 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)" = "7716c242968ee87e5542f8021178248f267f295a5c4803beae8b8b7fd9bc6051"
34243412
"checksum void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d"
34253413
"checksum wait-timeout 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)" = "b9f3bf741a801531993db6478b95682117471f76916f5e690dd8d45395b09349"

Diff for: src/librustc_mir/const_eval.rs

+10-5
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ use rustc::mir::interpret::{
3333
Scalar, Allocation, AllocId, ConstValue,
3434
};
3535
use interpret::{self,
36-
Place, PlaceTy, MemPlace, OpTy, Operand, Value,
36+
PlaceTy, MemPlace, OpTy, Operand, Value,
3737
EvalContext, StackPopCleanup, MemoryKind,
3838
snapshot,
3939
};
@@ -55,13 +55,14 @@ pub fn mk_borrowck_eval_cx<'a, 'mir, 'tcx>(
5555
let param_env = tcx.param_env(instance.def_id());
5656
let mut ecx = EvalContext::new(tcx.at(span), param_env, CompileTimeInterpreter::new(), ());
5757
// insert a stack frame so any queries have the correct substs
58+
// cannot use `push_stack_frame`; if we do `const_prop` explodes
5859
ecx.stack.push(interpret::Frame {
5960
block: mir::START_BLOCK,
6061
locals: IndexVec::new(),
6162
instance,
6263
span,
6364
mir,
64-
return_place: Place::null(tcx),
65+
return_place: None,
6566
return_to_block: StackPopCleanup::Goto(None), // never pop
6667
stmt: 0,
6768
});
@@ -82,7 +83,7 @@ pub fn mk_eval_cx<'a, 'tcx>(
8283
instance,
8384
mir.span,
8485
mir,
85-
Place::null(tcx),
86+
None,
8687
StackPopCleanup::Goto(None), // never pop
8788
)?;
8889
Ok(ecx)
@@ -187,7 +188,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
187188
cid.instance,
188189
mir.span,
189190
mir,
190-
Place::Ptr(*ret),
191+
Some(ret.into()),
191192
StackPopCleanup::None { cleanup: false },
192193
)?;
193194

@@ -342,7 +343,11 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx>
342343
type MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation<()>)>;
343344

344345
const STATIC_KIND: Option<!> = None; // no copying of statics allowed
345-
const ENFORCE_VALIDITY: bool = false; // for now, we don't
346+
347+
#[inline(always)]
348+
fn enforce_validity(_ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool {
349+
false // for now, we don't enforce validity
350+
}
346351

347352
fn find_fn(
348353
ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>,

Diff for: src/librustc_mir/interpret/cast.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
322322
// For now, upcasts are limited to changes in marker
323323
// traits, and hence never actually require an actual
324324
// change to the vtable.
325-
self.copy_op(src, dest)
325+
let val = self.read_value(src)?;
326+
self.write_value(*val, dest)
326327
}
327328
(_, &ty::Dynamic(ref data, _)) => {
328329
// Initial cast from sized to dyn trait

Diff for: src/librustc_mir/interpret/eval_context.rs

+52-27
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ use rustc::mir::interpret::{
3131
use syntax::source_map::{self, Span};
3232

3333
use super::{
34-
Value, Operand, MemPlace, MPlaceTy, Place, ScalarMaybeUndef,
34+
Value, Operand, MemPlace, MPlaceTy, Place, PlaceTy, ScalarMaybeUndef,
3535
Memory, Machine
3636
};
3737

@@ -73,8 +73,9 @@ pub struct Frame<'mir, 'tcx: 'mir, Tag=()> {
7373
/// Work to perform when returning from this function
7474
pub return_to_block: StackPopCleanup,
7575

76-
/// The location where the result of the current stack frame should be written to.
77-
pub return_place: Place<Tag>,
76+
/// The location where the result of the current stack frame should be written to,
77+
/// and its layout in the caller.
78+
pub return_place: Option<PlaceTy<'tcx, Tag>>,
7879

7980
/// The list of locals for this stack frame, stored in order as
8081
/// `[return_ptr, arguments..., variables..., temporaries...]`.
@@ -97,7 +98,8 @@ pub struct Frame<'mir, 'tcx: 'mir, Tag=()> {
9798
#[derive(Clone, Debug, Eq, PartialEq, Hash)]
9899
pub enum StackPopCleanup {
99100
/// Jump to the next block in the caller, or cause UB if None (that's a function
100-
/// that may never return).
101+
/// that may never return). Also store layout of return place so
102+
/// we can validate it at that layout.
101103
Goto(Option<mir::BasicBlock>),
102104
/// Just do nohing: Used by Main and for the box_alloc hook in miri.
103105
/// `cleanup` says whether locals are deallocated. Static computation
@@ -330,22 +332,16 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
330332
}
331333

332334
/// Return the actual dynamic size and alignment of the place at the given type.
333-
/// Only the `meta` part of the place matters.
335+
/// Only the "meta" (metadata) part of the place matters.
336+
/// This can fail to provide an answer for extern types.
334337
pub(super) fn size_and_align_of(
335338
&self,
336339
metadata: Option<Scalar<M::PointerTag>>,
337340
layout: TyLayout<'tcx>,
338-
) -> EvalResult<'tcx, (Size, Align)> {
339-
let metadata = match metadata {
340-
None => {
341-
assert!(!layout.is_unsized());
342-
return Ok(layout.size_and_align())
343-
}
344-
Some(metadata) => {
345-
assert!(layout.is_unsized());
346-
metadata
347-
}
348-
};
341+
) -> EvalResult<'tcx, Option<(Size, Align)>> {
342+
if !layout.is_unsized() {
343+
return Ok(Some(layout.size_and_align()));
344+
}
349345
match layout.ty.sty {
350346
ty::Adt(..) | ty::Tuple(..) => {
351347
// First get the size of all statically known fields.
@@ -365,9 +361,11 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
365361
);
366362

367363
// Recurse to get the size of the dynamically sized field (must be
368-
// the last field).
364+
// the last field). Can't have foreign types here, how would we
365+
// adjust alignment and size for them?
369366
let field = layout.field(self, layout.fields.count() - 1)?;
370-
let (unsized_size, unsized_align) = self.size_and_align_of(Some(metadata), field)?;
367+
let (unsized_size, unsized_align) = self.size_and_align_of(metadata, field)?
368+
.expect("Fields cannot be extern types");
371369

372370
// FIXME (#26403, #27023): We should be adding padding
373371
// to `sized_size` (to accommodate the `unsized_align`
@@ -394,18 +392,22 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
394392
//
395393
// `(size + (align-1)) & -align`
396394

397-
Ok((size.abi_align(align), align))
395+
Ok(Some((size.abi_align(align), align)))
398396
}
399397
ty::Dynamic(..) => {
400-
let vtable = metadata.to_ptr()?;
398+
let vtable = metadata.expect("dyn trait fat ptr must have vtable").to_ptr()?;
401399
// the second entry in the vtable is the dynamic size of the object.
402-
self.read_size_and_align_from_vtable(vtable)
400+
Ok(Some(self.read_size_and_align_from_vtable(vtable)?))
403401
}
404402

405403
ty::Slice(_) | ty::Str => {
406-
let len = metadata.to_usize(self)?;
404+
let len = metadata.expect("slice fat ptr must have vtable").to_usize(self)?;
407405
let (elem_size, align) = layout.field(self, 0)?.size_and_align();
408-
Ok((elem_size * len, align))
406+
Ok(Some((elem_size * len, align)))
407+
}
408+
409+
ty::Foreign(_) => {
410+
Ok(None)
409411
}
410412

411413
_ => bug!("size_and_align_of::<{:?}> not supported", layout.ty),
@@ -415,7 +417,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
415417
pub fn size_and_align_of_mplace(
416418
&self,
417419
mplace: MPlaceTy<'tcx, M::PointerTag>
418-
) -> EvalResult<'tcx, (Size, Align)> {
420+
) -> EvalResult<'tcx, Option<(Size, Align)>> {
419421
self.size_and_align_of(mplace.meta, mplace.layout)
420422
}
421423

@@ -424,7 +426,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
424426
instance: ty::Instance<'tcx>,
425427
span: source_map::Span,
426428
mir: &'mir mir::Mir<'tcx>,
427-
return_place: Place<M::PointerTag>,
429+
return_place: Option<PlaceTy<'tcx, M::PointerTag>>,
428430
return_to_block: StackPopCleanup,
429431
) -> EvalResult<'tcx> {
430432
::log_settings::settings().indentation += 1;
@@ -509,15 +511,38 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
509511
}
510512
StackPopCleanup::None { cleanup } => {
511513
if !cleanup {
512-
// Leak the locals
514+
// Leak the locals. Also skip validation, this is only used by
515+
// static/const computation which does its own (stronger) final
516+
// validation.
513517
return Ok(());
514518
}
515519
}
516520
}
517-
// deallocate all locals that are backed by an allocation
521+
// Deallocate all locals that are backed by an allocation.
518522
for local in frame.locals {
519523
self.deallocate_local(local)?;
520524
}
525+
// Validate the return value.
526+
if let Some(return_place) = frame.return_place {
527+
if M::enforce_validity(self) {
528+
// Data got changed, better make sure it matches the type!
529+
// It is still possible that the return place held invalid data while
530+
// the function is running, but that's okay because nobody could have
531+
// accessed that same data from the "outside" to observe any broken
532+
// invariant -- that is, unless a function somehow has a ptr to
533+
// its return place... but the way MIR is currently generated, the
534+
// return place is always a local and then this cannot happen.
535+
self.validate_operand(
536+
self.place_to_op(return_place)?,
537+
&mut vec![],
538+
None,
539+
/*const_mode*/false,
540+
)?;
541+
}
542+
} else {
543+
// Uh, that shouln't happen... the function did not intend to return
544+
return err!(Unreachable);
545+
}
521546

522547
Ok(())
523548
}

Diff for: src/librustc_mir/interpret/intrinsics.rs

+1-5
Original file line numberDiff line numberDiff line change
@@ -151,11 +151,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
151151
self.write_scalar(val, dest)?;
152152
}
153153
"transmute" => {
154-
// Go through an allocation, to make sure the completely different layouts
155-
// do not pose a problem. (When the user transmutes through a union,
156-
// there will not be a layout mismatch.)
157-
let dest = self.force_allocation(dest)?;
158-
self.copy_op(args[0], dest.into())?;
154+
self.copy_op_transmute(args[0], dest)?;
159155
}
160156

161157
_ => return Ok(false),

Diff for: src/librustc_mir/interpret/machine.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized {
8686
const STATIC_KIND: Option<Self::MemoryKinds>;
8787

8888
/// Whether to enforce the validity invariant
89-
const ENFORCE_VALIDITY: bool;
89+
fn enforce_validity(ecx: &EvalContext<'a, 'mir, 'tcx, Self>) -> bool;
9090

9191
/// Called before a basic block terminator is executed.
9292
/// You can use this to detect endlessly running programs.

Diff for: src/librustc_mir/interpret/memory.rs

+24-1
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
650650
}
651651

652652
/// It is the caller's responsibility to handle undefined and pointer bytes.
653-
/// However, this still checks that there are no relocations on the egdes.
653+
/// However, this still checks that there are no relocations on the *egdes*.
654654
#[inline]
655655
fn get_bytes_with_undef_and_ptr(
656656
&self,
@@ -842,6 +842,29 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
842842
}
843843
}
844844

845+
pub fn check_bytes(
846+
&self,
847+
ptr: Scalar<M::PointerTag>,
848+
size: Size,
849+
allow_ptr_and_undef: bool,
850+
) -> EvalResult<'tcx> {
851+
// Empty accesses don't need to be valid pointers, but they should still be non-NULL
852+
let align = Align::from_bytes(1, 1).unwrap();
853+
if size.bytes() == 0 {
854+
self.check_align(ptr, align)?;
855+
return Ok(());
856+
}
857+
let ptr = ptr.to_ptr()?;
858+
// Check bounds, align and relocations on the edges
859+
self.get_bytes_with_undef_and_ptr(ptr, size, align)?;
860+
// Check undef and ptr
861+
if !allow_ptr_and_undef {
862+
self.check_defined(ptr, size)?;
863+
self.check_relocations(ptr, size)?;
864+
}
865+
Ok(())
866+
}
867+
845868
pub fn read_bytes(&self, ptr: Scalar<M::PointerTag>, size: Size) -> EvalResult<'tcx, &[u8]> {
846869
// Empty accesses don't need to be valid pointers, but they should still be non-NULL
847870
let align = Align::from_bytes(1, 1).unwrap();

0 commit comments

Comments
 (0)