Miri panics on sketchy code

[idubrov]

cargo +nightly miri crashes on the following sketchy code:

trait Empty {}

#[repr(transparent)]
pub struct FunnyPointer(dyn Empty);

#[repr(C)]
pub struct Meta {
    drop_fn: fn(&mut ()),
    size: usize,
    align: usize,
}

impl Meta {
    pub fn new() -> Self {
        Meta {
            drop_fn: |_| {},
            size: 0,
            align: 1,
        }
    }
}

#[repr(C)]
pub struct FatPointer {
    pub data: *const (),
    pub vtable: *const (),
}

impl FunnyPointer {
    pub unsafe fn from_data_ptr(data: &String, ptr: *const Meta) -> &Self {
        let obj = FatPointer {
            data: data as *const _ as *const (),
            vtable: ptr as *const _ as *const (),
        };
        let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj);
        &*obj
    }
}

fn main() {
    unsafe {
        let meta = Meta::new();
        let hello = "hello".to_string();
        let _raw: &FunnyPointer = FunnyPointer::from_data_ptr(&hello, &meta as *const _);
    }
}

Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d2a12549c324df2b61cd5c118abd0789

Backtrace:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', /rustc/27d6f55f47e8875e71083a28ed84ea5a88e1b596/src/libcore/macros/mod.rs:15:40
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: rustc_driver::report_ice
   6: std::panicking::rust_panic_with_hook
   7: rust_begin_unwind
   8: core::panicking::panic_fmt
   9: core::panicking::panic
  10: rustc_mir::interpret::traits::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::read_drop_type_from_vtable
  11: rustc_mir::interpret::validity::ValidityVisitor<M>::check_wide_ptr_meta
  12: rustc_mir::interpret::visitor::ValueVisitor::walk_value
  13: rustc_mir::interpret::validity::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::validate_operand
  14: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::copy_op_transmute
  15: rustc_mir::interpret::intrinsics::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::emulate_intrinsic
  16: miri::shims::intrinsics::EvalContextExt::call_intrinsic
  17: rustc_mir::interpret::terminator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_fn_call
  18: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::run
  19: miri::eval::eval_main
  20: rustc::ty::context::tls::enter_global
  21: <miri::MiriCompilerCalls as rustc_driver::Callbacks>::after_analysis
  22: rustc_interface::interface::run_compiler_in_existing_thread_pool
  23: std::thread::local::LocalKey<T>::with
  24: scoped_tls::ScopedKey<T>::set
  25: syntax::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.41.0-nightly (27d6f55f4 2019-12-11) running on x86_64-apple-darwin

note: compiler flags: -Z always-encode-mir -Z mir-emit-retag -Z mir-opt-level=0 -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack
error: could not compile `miri-crash`.

[idubrov]

Changing drop_fn to drop_fn: extern "C" fn(&mut ()), seems to remove that panic.

[RalfJung]

My best guess is that this panics here:

https://github.com/rust-lang/rust/blob/f284f8b4be3a899bf2ecc03e2a1589f486b62a9f/src/librustc_mir/interpret/traits.rs#L143

But that should only panic if your drop-fn has a non-ptr argument... but a reference is a ptr.
The ABI really shouldn't make a difference. Odd.

[RalfJung]

The ICE will be fixed by rust-lang/rust#67254. But the code will be considered UB then, because the drop fn is a closure and not a "normal" function.

Generally, code that makes any assumptions about vtable layout has unspecified behavior at best -- vtable layout is not fixed and can change any time.

[idubrov]

Thanks!

Yeah, what I'm experimenting here is unspecified at best. Without custom DSTs, I'm trying to see how much I can get away with by stuffing my extra metadata at the end of an "empty" VTable (VTable of trait object without any functions). 😅

"Custom" references in Rust via structs (like struct Ref<'a>(&'a T, Metadata); and struct RefMut<'a>(&'a mut T, Metadata);) have pretty terrible ergonomics in general.

So here I am, abusing trait objects. Hoping that one day once we maybe get custom DSTs, that won't be needed.

Getting miri not bark at me is not really a proof it's okay to do that, of course, but gives me a bit of comfort.

[RalfJung]

Well, here's the variant that doesn't make Miri bark:

trait Empty {}

#[repr(transparent)]
pub struct FunnyPointer(dyn Empty);

#[repr(C)]
pub struct Meta {
    drop_fn: fn(*mut ()),
    size: usize,
    align: usize,
}

fn nop(_x: *mut ()) {}

impl Meta {
    pub fn new() -> Self {
        Meta {
            drop_fn: nop,
            size: 0,
            align: 1,
        }
    }
}

#[repr(C)]
pub struct FatPointer {
    pub data: *const (),
    pub vtable: *const (),
}

impl FunnyPointer {
    pub unsafe fn from_data_ptr(data: &String, ptr: *const Meta) -> &Self {
        let obj = FatPointer {
            data: data as *const _ as *const (),
            vtable: ptr as *const _ as *const (),
        };
        let obj = std::mem::transmute::<FatPointer, *mut FunnyPointer>(obj);
        &*obj
    }
}

fn main() {
    unsafe {
        let meta = Meta::new();
        let hello = "hello".to_string();
        let _raw: &FunnyPointer = FunnyPointer::from_data_ptr(&hello, &meta as *const _);
    }
}

[RalfJung]

I would accept a PR against rustc that makes closures work there. There's no fundamental reason why they don't work -- the actual function call logic supports them property, but some of our kind-of hacky vtable management (where the panic occurs) does not. But I am not sure if that's worth it. ;)

The ICE is still definitely a bug, though.