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

"constructing invalid value: wrong trait in wide pointer vtable" for seemingly identical traits #3541

Closed
Jules-Bertholet opened this issue May 3, 2024 · 9 comments · Fixed by rust-lang/rust#126232
Labels
A-validation Area: This affects enforcing the validity invariant, and related UB checking C-bug Category: This is a bug. I-false-UB Impact: makes Miri falsely report UB, i.e., a false positive (with default settings)

Comments

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented May 3, 2024

I don't have a minimal reduction atm, but the issue can be reproduced by cloning https://github.com/Jules-Bertholet/unsized-vec and running cargo miri test -p emplacable. This results in the following report of UB:

constructing invalid value: wrong trait in wide pointer vtable: expected `for<'b> std::ops::FnMut(std::alloc::Layout, <[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata, &'b mut (dyn std::ops::FnMut(*mut std::marker::PhantomData<[std::boxed::Box<i32>]>) + 'b))`, but encountered `for<'a> std::ops::FnMut<(std::alloc::Layout, usize, &'a mut (dyn std::ops::FnMut(*mut std::marker::PhantomData<[std::boxed::Box<i32>]>) + 'a))>`

But the two traits are the same! <[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata is usize, and for<'a> ... vs for<'b> ... should not make a difference either.

@rustbot label I-false-UB A-validation

@rustbot rustbot added A-validation Area: This affects enforcing the validity invariant, and related UB checking I-false-UB Impact: makes Miri falsely report UB, i.e., a false positive (with default settings) labels May 3, 2024
@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

Probably the use of != around here is wrong. @lcnr what is the proper way to compare two Option<ty::Binder<'tcx, ExistentialTraitRef<'tcx>>>?

@RalfJung RalfJung added the C-bug Category: This is a bug. label May 3, 2024
@lcnr
Copy link
Contributor

lcnr commented May 4, 2024

let ocx = ObligationCtxt::new(infcx);
let param_env = ty::ParamEnv::reveal_all(); // in an empty environment post borrowck
ocx.eq(param_env, lhs, rhs)?; // equate the two trait refs
ocx.select_all_or_error().is_empty() // and check any potential nested constraints

@RalfJung
Copy link
Member

RalfJung commented May 5, 2024

Thanks! We actually have a param_env in the interpreter so I guess it won't be reveal_all. But we don't have a infcx.

@lcnr
Copy link
Contributor

lcnr commented May 6, 2024

you can create a local one, given that the input should not refer to either placeholders or inference variables

@RalfJung
Copy link
Member

@lcnr any idea how to get a reproducible example for this? I tried this code but it doesn't trigger the error -- probably things get normalized earlier already so by the time we check the trait type, everything is already equal.

#![feature(unboxed_closures)]
#![feature(ptr_metadata)]

type T1<'a> = &'a dyn for<'b> Fn(usize);
type T2<'a> = &'a dyn for<'b> Fn<(<[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata,), Output=()>;

fn main() {
    let x: T1 = &|_| {};
    let y: T2 = unsafe { std::mem::transmute(x) };
    y(42);
}

@RalfJung
Copy link
Member

I don't have a minimal reduction atm, but the issue can be reproduced by cloning https://github.com/Jules-Bertholet/unsized-vec and running cargo miri test -p emplacable. This results in the following report of UB:

I get a build error instead:

error[E0283]: type annotations needed
   --> emplacable/src/lib.rs:196:75
    |
196 |         let wide_ptr: *mut str = ptr::from_raw_parts_mut(buf.as_mut_ptr().cast(), LEN);
    |                                  -----------------------                  ^^^^ cannot infer type of the type parameter `U` declared on the method `cast`
    |                                  |
    |                                  required by a bound introduced by this call
    |
    = note: cannot satisfy `_: core::ptr::Thin`
note: required by a bound in `core::ptr::from_raw_parts_mut`
   --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/metadata.rs:137:29
    |
136 | pub const fn from_raw_parts_mut<T: ?Sized>(
    |              ------------------ required by a bound in this function
137 |     data_pointer: *mut impl Thin,
    |                             ^^^^ required by this bound in `from_raw_parts_mut`
help: consider specifying the generic argument
    |
196 |         let wide_ptr: *mut str = ptr::from_raw_parts_mut(buf.as_mut_ptr().cast::<U>(), LEN);
    |                                                                               +++++
help: consider removing this method call, as the receiver has type `*mut core::mem::MaybeUninit<u8>` and `*mut core::mem::MaybeUninit<u8>: core::ptr::Thin` trivially holds
    |
196 -         let wide_ptr: *mut str = ptr::from_raw_parts_mut(buf.as_mut_ptr().cast(), LEN);
196 +         let wide_ptr: *mut str = ptr::from_raw_parts_mut(buf.as_mut_ptr(), LEN);
    |

@lcnr
Copy link
Contributor

lcnr commented Jun 10, 2024

@lcnr any idea how to get a reproducible example for this? I tried this code but it doesn't trigger the error -- probably things get normalized earlier already so by the time we check the trait type, everything is already equal.

#![feature(unboxed_closures)]
#![feature(ptr_metadata)]

type T1<'a> = &'a dyn for<'b> Fn(usize);
type T2<'a> = &'a dyn for<'b> Fn<(<[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata,), Output=()>;

fn main() {
    let x: T1 = &|_| {};
    let y: T2 = unsafe { std::mem::transmute(x) };
    y(42);
}

unfortunately not, my idea was something like the following, as types from the HIR get normalized, but that didn't seem to do the trick 🤔

#![feature(ptr_metadata)]
type T1<'a> = &'a dyn for<'b> Fn(usize);
type T2<'a> = &'a dyn for<'b> Fn(<[std::boxed::Box<i32>] as std::ptr::Pointee>::Metadata);

#[repr(transparent)]
struct Foo<'a> {
    x: &'a T2<'a>,
}

fn main() {
    let foo = Foo { x: &((&|_| ()) as &dyn Fn(usize)) };
    let x: T1<'_> = foo.x;
    x(42);
}

@RalfJung
Copy link
Member

RalfJung commented Jun 10, 2024

I got something by minimizing emplacable.

#![feature(ptr_metadata)]
// This test is the result of minimizing the `emplacable` crate to reproduce
// <https://github.com/rust-lang/miri/issues/3541>.

use std::{ops::FnMut, ptr::Pointee};

pub type EmplacerFn<'a, T> = dyn for<'b> FnMut(<T as Pointee>::Metadata) + 'a;

#[repr(transparent)]
pub struct Emplacer<'a, T>(EmplacerFn<'a, T>)
where
    T: ?Sized;

impl<'a, T> Emplacer<'a, T>
where
    T: ?Sized,
{
    pub unsafe fn from_fn<'b>(emplacer_fn: &'b mut EmplacerFn<'a, T>) -> &'b mut Self {
        unsafe { &mut *((emplacer_fn as *mut EmplacerFn<'a, T>) as *mut Self) }
    }
}

pub fn box_new_with<T>()
where
    T: ?Sized,
{
    let emplacer_closure = &mut |_meta| {
        unreachable!();
    };

    unsafe { Emplacer::<T>::from_fn(emplacer_closure) };
}

fn main() {
    box_new_with::<[Box<i32>]>();
}

@Jules-Bertholet thanks for making this a small self-contained crate, minimization would probably not have been feasible otherwise. :)

@Jules-Bertholet
Copy link
Contributor Author

I get a build error instead

Such is the way of nightly-only crates… It's fixed now

rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 12, 2024
Rollup merge of rust-lang#126232 - RalfJung:dyn-trait-equality, r=oli-obk

interpret: dyn trait metadata check: equate traits in a proper way

Hopefully fixes rust-lang/miri#3541... unfortunately we don't have a testcase.

The first commit is just a refactor without functional change.

r? `@oli-obk`
github-actions bot pushed a commit that referenced this issue Jun 13, 2024
interpret: dyn trait metadata check: equate traits in a proper way

Hopefully fixes #3541... unfortunately we don't have a testcase.

The first commit is just a refactor without functional change.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-validation Area: This affects enforcing the validity invariant, and related UB checking C-bug Category: This is a bug. I-false-UB Impact: makes Miri falsely report UB, i.e., a false positive (with default settings)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants