ensure that integers cast to pointers will never point at a valid alloc, not even the zst alloc #81

oli-obk · 2016-11-10T18:20:53Z

fixes #77

…oc, not even the zst alloc

oli-obk · 2016-11-10T18:21:18Z

src/memory.rs

@@ -73,7 +73,7 @@ impl Pointer {
    // FIXME(solson): Integer pointers should use u64, not usize. Target pointers can be larger
    // than host usize.
    pub fn from_int(i: usize) -> Self {
-        Pointer::new(ZST_ALLOC_ID, i)
+        Pointer::new(NEVER_ALLOC_ID, i)


so I did this change to fix the assume bug, which then caused drop impls to fail: https://github.com/solson/miri/pull/81/files#diff-8f4a840e817b018e1d74153888641b27L642 and int to pointer cast error messages to show up as dangling pointers

Seems fine. I might have a plan to refactor Pointer::from_int out of existence, but I haven't taken a close enough look yet.

fixes rust-lang#76

solson · 2016-11-11T18:47:45Z

Sorry for the delay. I'll try to do a full review this evening.

oli-obk · 2016-11-11T20:56:58Z

No hurry. I'm not gonna get to it before monday

solson

Looks good, I just have a few questions.

solson · 2016-11-12T04:51:57Z

src/error.rs

@@ -61,7 +61,7 @@ impl<'tcx> Error for EvalError<'tcx> {
            EvalError::DanglingPointerDeref =>
                "dangling pointer was dereferenced",
            EvalError::InvalidFunctionPointer =>
-                "tried to use a pointer as a function pointer",
+                "tried to use an integer pointer as a function pointer",


Why doesn't this apply to general pointers casted to function pointers any more? The name InvalidFunctionPointer seems too general for the new error message.

@oli-obk pinging to make sure you don't miss this.

Nothing changed here, the error message was outdated way before this pr, we can adjust the variant name to match that

What kind of error do we actually get when treating a regular pointer as a function pointer?

let b = Box::new(42); let g = unsafe { std::mem::transmute::<&usize, &fn(i32)>(&b) }; (*g)(42) //~ ERROR tried to use an integer pointer or a dangling pointer as a function pointer

We can't do transmute(42) because primvals panic in some cases right now. I think we should eliminate the expect functions and have the try functions return an EvalResult instead.

10: 0x55e4c4ab7a18 - <core::option::Option<T>>::expect::hda65b98614e3f1e8 at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libcore/option.rs:293 11: 0x55e4c4bf53c3 - miri::primval::PrimVal::expect_fn_ptr::h87524394959696a3 at /home/ws/ca8159/Projects/rust/miri/src/primval.rs:198 12: 0x55e4c4ba9ae7 - miri::interpreter::terminator::<impl miri::interpreter::EvalContext<'a, 'tcx>>::eval_terminator::h1e9da0fab75dcb3c at /home/ws/ca8159/Projects/rust/miri/src/interpreter/terminator/mod.rs:88 13: 0x55e4c4b960c5 - miri::interpreter::step::<impl miri::interpreter::EvalContext<'a, 'tcx>>::terminator::h0a43b45ccf9ebe32 at /home/ws/ca8159/Projects/rust/miri/src/interpreter/step.rs:98 14: 0x55e4c4b95659 - miri::interpreter::step::<impl miri::interpreter::EvalContext<'a, 'tcx>>::step::ha153e9fdef721ba9 at /home/ws/ca8159/Projects/rust/miri/src/interpreter/step.rs:69 15: 0x55e4c4be5fb8 - miri::interpreter::eval_main::h502fc7147a3a0d58 at /home/ws/ca8159/Projects/rust/miri/src/interpreter/mod.rs:1664 16: 0x55e4c4a8c843 - <miri::MiriCompilerCalls as rustc_driver::CompilerCalls<'a>>::build_controller::{{closure}}::h01b87c9edeea1ee3 at /home/ws/ca8159/Projects/rust/miri/src/bin/miri.rs:72

solson · 2016-11-12T04:59:54Z

src/memory.rs

@@ -73,7 +73,7 @@ impl Pointer {
    // FIXME(solson): Integer pointers should use u64, not usize. Target pointers can be larger
    // than host usize.
    pub fn from_int(i: usize) -> Self {
-        Pointer::new(ZST_ALLOC_ID, i)
+        Pointer::new(NEVER_ALLOC_ID, i)


Seems fine. I might have a plan to refactor Pointer::from_int out of existence, but I haven't taken a close enough look yet.

solson · 2016-11-12T05:04:19Z

src/interpreter/mod.rs

@@ -523,7 +519,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
                                self.write_value(value, dest, value_ty)?;
                            } else {
                                assert_eq!(operands.len(), 0);
-                                let zero = self.isize_primval(0);
+                                let value_size = self.type_size(dest_ty).expect("pointer types are sized");
+                                let zero = PrimVal::from_int_with_size(0, value_size);


Why this change? I think we can assume isize is pointer sized, and isize_primval does mean the target isize.

We don't want an isize here. The rawnullablepointer opt also happens for nonpointers

Ahh, right, for NonZero things that aren't pointer-sized? I hate the RawNullablePointer names in layout...

solson · 2016-11-12T05:06:57Z

src/interpreter/mod.rs


                                // FIXME(solson)
                                let dest = self.force_allocation(dest)?.to_ptr();

                                let dest = dest.offset(offset.bytes() as isize);
-                                try!(self.memory.write_isize(dest, 0));
+                                let dest_size = self.type_size(ty).unwrap_or(self.memory.pointer_size());


In what cases can this default to self.memory.pointer_size()?

In the unsized case. Since any unsized value is a pointer to the actual sized value and this optimization only cares about the pointer part, not the extra part

What unsized case? ty is the type of the non-zero field that is used for the discriminant in this optimization, right? How can that field be unsized?

EDIT: Nevermind, I now see the conversation lower down; let's continue there.

solson · 2016-11-12T05:11:41Z

src/interpreter/terminator/mod.rs

                let nonnull = adt_ptr.offset(offset.bytes() as isize);
-                self.read_nonnull_discriminant_value(nonnull, nndiscr)?
+                // only the pointer part of a fat pointer is used for this space optimization
+                let discr_size = self.type_size(ty).unwrap_or(self.memory.pointer_size());


Ditto here. When can the nonnull field be a unsized?

It can't be, but the MIR ends up giving us an index path that will give us the first field of the fat pointer, which ends up being te unsized value. Not sure if the code giving us the type path is wrong or if this is intended. I'll check and get back to you

The first field of the fat pointer is the thin data pointer. This is a GEP field path, it will not deref nested pointers, just offset into a flat structure.

Ah, I see. It sounds like we'd need to adjust the type, then, to match @eddyb's point. Alternately, maybe it can return the size instead of the type?

eddyb · 2016-11-13T19:06:49Z

src/interpreter/mod.rs

+                    match (field_index, &ty.sty) {
+                        (1, &ty::TySlice(_)) => Ok(self.tcx.types.usize),
+                        (1, &ty::TyTrait(_)) |
+                        (0, _) => Ok(self.tcx.mk_imm_ptr(self.tcx.types.never)),


I'd use a pointer to u8 to avoid any other implications. Btw you need to use struct_tail to get the right ty to even check here, e.g. for Struct<str> - oh you also need to handle str.
Also I don't think the sized case above makes sense? We only treat a fat pointer like a pair.

eddyb · 2016-11-13T20:48:47Z

src/interpreter/mod.rs

-                    }
+                assert!(!self.type_is_sized(ty));
+                match (field_index, &self.tcx.struct_tail(ty).sty) {
+                    (1, &ty::TySlice(_)) => Ok(self.tcx.types.usize),


This should also mention TyStr... However, neither [T] nor str's length fields can be used for the optimization.
It's probably okay to keep it? I'm not sure. Also, the assert!(!self.type_is_sized(ty)); can be removed since you use struct_tail, all you have to do is match on TyTrait / TySlice / TyStr specifically for the pointer field.

It's probably okay to keep it?

I'd prefer to stick to cases we know can happen (field_index == 0, or more specifically the constant that says which fat pointer field is the thin pointer) and assert that nothing else happens. Since we can't use length fields, I'd remove any code that handles those cases. Vtable pointers seem plausible, but do we even use those?

I'd keep vtable pointers since we might use them whenever we get more thorough niche optimizations.

What about other values used as discriminants? The slice length might be used in optimizations for nonzst slices, which can't be usize::max_value() long.

Actually, I take back what I said since this is the general get_field_ty, not specific to the non-null optimizations.

The fact that this is the general get_field_ty also means we should guard against thin pointer types one way or another, perhaps by adding a match guard so thin pointers go to the "can't handle type" case.

@oli-obk Interesting point but we never considered it AFAIK.

The sized assertion does this check

oli-obk · 2016-11-13T20:57:18Z

we actually pass a lot more tests now: #55

solson · 2016-11-14T21:00:32Z

src/interpreter/mod.rs

-                        _ => bug!("invalid fat pointee type"),
-                    }
+                assert!(!self.type_is_sized(ty));
+                match (field_index, &self.tcx.struct_tail(ty).sty) {


I think the & here and on the patterns is redundant.

It's not, the value is not copyable.

@eddyb Is this a problem because of the tuple? Not being copyable for sure wouldn't be a problem when matching directly, because the patterns don't try to copy (or move, or bind at all) anything in the TyS.

In other words, the error is on building the tuple, not matching... you'd need language support for matching two things at once without a tuple to make it work the way I expected.

the FIXME was wrong here, there's no need for any special offsetting

oli-obk · 2016-11-15T13:14:23Z

ok, everything has been addressed and fixed. I did some changes to how Primval pointers are handled to make sure that there's no difference between reading a primval from memory and transmuting a primval

…ince tracing is slow)

solson · 2016-11-17T03:40:08Z

src/interpreter/mod.rs

-                    }
-
-                    _ => bug!("can't deref non pointer types"),
+                match self.tcx.struct_tail(pointee_type).sty {


This is so much clearer. :D

solson · 2016-11-17T03:45:50Z

src/memory.rs

-        if let Some(ptr) = val.try_as_ptr() {
-            return self.write_ptr(dest, ptr);
+        if let Some(alloc_id) = val.relocation {
+            return self.write_ptr(dest, Pointer::new(alloc_id, val.bits as usize));


Not your mistake, just making a note for posterity: this as usize is wrong when emulating a 64-bit target on a 32-bit host. I have a TODO on Pointer for changing the usize to u64.

solson · 2016-11-17T04:06:15Z

The trend toward making decisions in the code based on Tys rather than PrimValKind or other Miri things like relocations is a great trend. It will help with some of my refactorings and seems to be making the code clearer.

Release 1.0.2

ensure that integers cast to pointers will never point at a valid all…

921f5af

…oc, not even the zst alloc

oli-obk commented Nov 10, 2016

View reviewed changes

oli-obk added 3 commits November 11, 2016 13:07

type_size now returns None for unsized types

b2d476e

report the bad integer size instead of just the fact that it is bad

1c40fb0

fix null optimizations for smaller than pointer enums

d42a7d0

fixes rust-lang#76

solson reviewed Nov 12, 2016

View reviewed changes

fix field indexing into fat pointers

75f56eb

eddyb reviewed Nov 13, 2016

View reviewed changes

cannot index into non-fat-pointers

f71c31c

eddyb reviewed Nov 13, 2016

View reviewed changes

also address TyStr in the null pointer optimization

2c34d65

solson reviewed Nov 14, 2016

View reviewed changes

oli-obk added 3 commits November 15, 2016 14:11

add test for int -> fn ptr cast

511fa40

minor fixes

4a39c22

the FIXME was wrong here, there's no need for any special offsetting

make sure ByVal pointers act just like ByRef to a pointer

14ff641

oli-obk added 9 commits November 15, 2016 14:33

print traces only when not running on the rust run-pass test suite (s…

13f22f8

…ince tracing is slow)

fix writing int->ptr transmuted primvals to memory

f77a0ab

add more atomic intrinsics

e2091ff

erase all lifetimes from function types before creating pointers to them

1549c2d

fix creation of simd types

4748587

don't print in the binop tests

5ee75c0

allow zsts in the zero case of a nullable pointer optimized enum

1c5c6cd

implement fn item -> trait object conversion

64155ff

merge closures and function and implement some closure vtable cases

fd68670

solson approved these changes Nov 17, 2016

View reviewed changes

solson merged commit 9322dec into rust-lang:master Nov 17, 2016

erickt pushed a commit to erickt/miri that referenced this pull request Feb 4, 2022

Merge pull request rust-lang#81 from cuviper/release-1.0.2

ee07695

Release 1.0.2

ensure that integers cast to pointers will never point at a valid alloc, not even the zst alloc #81

ensure that integers cast to pointers will never point at a valid alloc, not even the zst alloc #81

Conversation

oli-obk commented Nov 10, 2016

oli-obk Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solson commented Nov 11, 2016

oli-obk commented Nov 11, 2016

solson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solson Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Nov 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solson commented Nov 17, 2016

oli-obk Nov 10, 2016 •

edited

Loading

solson Nov 13, 2016 •

edited

Loading

eddyb Nov 13, 2016 •

edited

Loading