rusty_v8 icon indicating copy to clipboard operation
rusty_v8 copied to clipboard

Avoid creating a reference to uninitialized `T` when calculating offsets

Open y21 opened this issue 3 years ago • 0 comments

https://github.com/denoland/rusty_v8/blob/64ce32392a680695975a7d40921f11d1e4bbf7c5/src/inspector.rs#L275-L281

https://github.com/denoland/rusty_v8/blob/64ce32392a680695975a7d40921f11d1e4bbf7c5/src/inspector.rs#L538-L544

The call to .base() here on line 281 (and line 544) creates a reference to an uninitialized T, because base() is defined to take &self as its receiver. This is said to be UB by the documentation of MaybeUninit::as_ptr():

Gets a pointer to the contained value. Reading from this pointer or turning it into a reference is undefined behavior unless the MaybeUninit<T> is initialized

I'm not sure what the best way is to fix it: we cannot change the receiver to self: *const Self because that requires the nightly feature arbitrary_self_types. Just changing it to not take a receiver at all, and instead using some other identifier as its parameter (e.g. this: *const Self) makes the trait not object safe and that breaks ChannelBase' rust_vtable field: https://github.com/denoland/rusty_v8/blob/64ce32392a680695975a7d40921f11d1e4bbf7c5/src/inspector.rs#L258

We could make base and base_mut methods require Self: Sized so a &dyn ChannelImpl doesn't have those two methods, and I'm not sure if rust_vtable really needs base and base_mut methods, and if it doesn't, that might be a viable solution, i.e.

pub trait ChannelImpl: AsChannel {
  fn base(this: *const Self) -> *const ChannelBase where Self: Sized;
  fn base_mut(this: *mut Self) -> *mut ChannelBase where Self: Sized;
}

Then we can do what the get_offset_within_embedder methods do, without the reference:

let buf = std::mem::MaybeUninit::<T>::uninit(); 
let embedder_ptr: *const T = buf.as_ptr();
let base: *const Self = T::base(embedder_ptr);

Alternatively we could add another method, base_ptr, that has the same signature as base above, and keep base() and base_mut() as it is, so

pub trait ChannelImpl: AsChannel {
  fn base(&self) -> &ChannelBase;
  fn base_mut(&mut self) -> &mut ChannelBase;
  fn base_ptr(this: *const Self) -> *const ChannelBase where Self: Sized;
}

that we could then use in the get_offset_within_embedder methods like above.

Or lastly, we could just initialize it with dummy data (e.g. a null pointer).

Relevant PR: #954

y21 avatar May 04 '22 15:05 y21