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.

Sign up for GitHub

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 casts: do not blindly rely on dest type #72525

Merged

bors merged 5 commits into rust-lang:master from RalfJung:miri-cast-checks

May 25, 2020

Member

RalfJung commented May 24, 2020

Make sure that we notice when the MIR is bad and the casted-to and destination type are e.g. of different size, as suggested by @eddyb.

RalfJung added 2 commits

May 24, 2020 01:03


make some cast helpers infallible

fbc6f2c


Miri casts: do not blindly rely on dest type

b97ed1a

rust-highfive assigned cramertj

Collaborator

rust-highfive commented May 24, 2020

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive added the S-waiting-on-review label

Member Author

RalfJung commented May 24, 2020 •

edited

Loading

Dang, looks like this short-cut I took for unsizing coercions doesn't work in ui/associated-type-bounds/dyn-lcsit.rs...

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `&dyn Tr1<As1 = impl for<'a> Tr2<'a>> + std::marker::Sync`,
 right: `&dyn Tr1<As1 = cdef_et4::A> + std::marker::Sync`: mismatch of cast type &dyn Tr1<As1 = impl for<'a> Tr2<'a>> + std::marker::Sync and place type &dyn Tr1<As1 = cdef_et4::A> + std::marker::Sync', src/librustc_mir/interpret/cast.rs:28:17

This comment has been minimized.

Sign in to view


Fix unsizing casts

b9b1554

Member Author

RalfJung commented May 24, 2020

All right, fixed that.

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

  self.write_immediate(res, dest)?;
  }
+ Pointer(PointerCast::MutToConstPointer | PointerCast::ArrayToPointer) => {
+ // These are NOPs, but can be wide pointers.

Member

eddyb May 24, 2020

Not important but I wonder if "noop" or "no-op" is more common.

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

  dest: PlaceTy<'tcx, M::PointerTag>,
  ) -> InterpResult<'tcx> {
  use rustc_middle::mir::CastKind::*;
- match kind {
+ // FIXME: In which cases should we trigger UB when the source is uninit?

Member

eddyb May 24, 2020

Hmm presumably the non-UB cases are at most those that slice/reuse bits? (so some pointer casts and integer truncations/zero-extensions)

Sign-extensions and any int <-> float casts should definitely be UB on uninit (and I'm guessing they are already?)

Member Author

RalfJung May 24, 2020

Sign-extensions and any int <-> float casts should definitely be UB on uninit (and I'm guessing they are already?)

Yeah anything that looks at the bits is already UB. But e.g. currently casting one raw ptr type to another would not be UB on uninitialized inputs. Maybe that's good, maybe not.

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

Comment on lines +40 to +41

		let v = self.read_immediate(src)?;
		self.write_immediate(*v, dest)?;

Member

eddyb May 24, 2020

Is this relying on the type/layout equality check being forgiving around raw pointers?

Member Author

RalfJung May 24, 2020

Hm... actually I am not sure I am happy with this. write_immediate cannot know the layout of the immediate so in particular when it is a ScalarPair it cannot know the offset of the 2nd field. We do check that both fields have the same size as what dest expects, but I am not sure if that is enough?

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

  self.write_immediate(res, dest)?;
  }
+ Pointer(PointerCast::MutToConstPointer | PointerCast::ArrayToPointer) => {

Member

eddyb May 24, 2020

ArrayToPointer is confusing, I'm guessing ArrayToElemPointer makes more sense (or should it be ArayRefToElemPointer? since a regular raw pointer cast shouldn't care about the pointee type changing).

Although PointerCast variants might not need the Pointer suffix. Anyway, this is not that important to this PR.

Member Author

RalfJung May 24, 2020

Yeah well I never understood most of these (and at some point whoever designed this clearly gave up and stuffed everything else into Misc).

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

Comment on lines +159 to +160

		assert_eq!(src.layout.size, 2 * self.memory.pointer_size());
		assert_eq!(dest_layout.size, self.memory.pointer_size());

Member

eddyb May 24, 2020

Uhh, this is a bit hacky, but I guess pre-existing. Should probably rely on wide pointers having their first field be a simple pointer. So really just need to extract the first field and then cast it as usual.

Member Author

RalfJung May 24, 2020

Well that's basically what we do except we also have a bunch of extra assertions?

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs Outdated Show resolved Hide resolved


use helper method for determining size of int type

7b98552

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs Outdated

Comment on lines 197 to 207

+ let size = match cast_ty.kind {
+ // FIXME: Isn't there a helper for this? The same pattern occurs below.
+ Int(t) => {
+ t.bit_width().map(Size::from_bits).unwrap_or_else(|| self.pointer_size())
+ }
+ Uint(t) => {
+ t.bit_width().map(Size::from_bits).unwrap_or_else(|| self.pointer_size())
+ }
+ RawPtr(_) => self.pointer_size(),
+ _ => bug!(),
+ };

Member

eddyb May 24, 2020

So much work to avoid calling layout_of 😁

Member Author

RalfJung May 24, 2020

I learned the hard way that layout_of is expensive...

eddyb reviewed

View reviewed changes

src/librustc_mir/interpret/cast.rs

Comment on lines +303 to +307

 trace!("Unsizing {:?} of type {} into {:?}", *src, src.layout.ty, cast_ty.ty);

 match (&src.layout.ty.kind, &cast_ty.ty.kind) {

 (&ty::Ref(_, s, _), &ty::Ref(_, c, _) | &ty::RawPtr(TypeAndMut { ty: c, .. }))

 | (&ty::RawPtr(TypeAndMut { ty: s, .. }), &ty::RawPtr(TypeAndMut { ty: c, .. })) => {

 self.unsize_into_ptr(src, dest, s, c)

Member

eddyb May 24, 2020

Hmm, in other parts of the compiler these are clearly marked "source" and "target", but I guess not consistently enough.

Either way, feel free to use "target" to be more symmetrical. (But we can leave it as-is)

Member Author

RalfJung May 24, 2020

"target" and "dest" are a bit too similar for my taste...

eddyb approved these changes

View reviewed changes

Member

eddyb left a comment

r=me with or without comments addressed (since this is mostly a refactor, with the only functional change AFAICT being which type is used as the cast target type)


comment nit

8b5ba4a

Member Author

RalfJung commented May 24, 2020

@bors r=eddyb

Contributor

bors commented May 24, 2020

📌 Commit 8b5ba4a has been approved by eddyb

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request


Rollup merge of rust-lang#72525 - RalfJung:miri-cast-checks, r=eddyb

8b65235

Miri casts: do not blindly rely on dest type

Make sure that we notice when the MIR is bad and the casted-to and destination type are e.g. of different size, as suggested by @eddyb.

Dylan-DPC-zz mentioned this pull request

Rollup of 4 pull requests #72555

Closed

RalfJung added a commit to RalfJung/rust that referenced this pull request


Rollup merge of rust-lang#72525 - RalfJung:miri-cast-checks, r=eddyb

13d467b

Miri casts: do not blindly rely on dest type

Make sure that we notice when the MIR is bad and the casted-to and destination type are e.g. of different size, as suggested by @eddyb.

This was referenced May 25, 2020

Rollup of 3 pull requests #72561

Closed

Rollup of 5 pull requests #72562

Merged

Member Author

RalfJung commented May 25, 2020

r? @eddyb (reflecting who already reviewed this)

rust-highfive assigned eddyb and unassigned cramertj

bors added a commit to rust-lang-ci/rust that referenced this pull request


Auto merge of rust-lang#72562 - RalfJung:rollup-2ngjgwi, r=RalfJung

ee6c0da

Rollup of 5 pull requests

Successful merges:

 - rust-lang#71940 (Add `len` and `slice_from_raw_parts` to `NonNull<[T]>`)
 - rust-lang#72525 (Miri casts: do not blindly rely on dest type)
 - rust-lang#72537 (Fix InlineAsmOperand expresions being visited twice during liveness checking)
 - rust-lang#72544 (librustc_middle: Rename upvars query to upvars_mentioned)
 - rust-lang#72551 (First draft documenting Debug stability.)

Failed merges:

r? @ghost

bors merged commit 4a5a655 into rust-lang:master

RalfJung deleted the miri-cast-checks branch

May 25, 2020 13:33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment