From 753ec83a05152e91b3a8feefb64d524b6da93677 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Thu, 12 Nov 2020 10:24:47 +0100 Subject: [PATCH] Fix crash with VRc::borrow() on wasm32 When converting a Weak into Weak, we were ignoring that while T inside VRcInner may be at a certain offset to satisfy alignment, the empty Dyn field may not and thus be at a different offset. On wasm32 for example the compiler generated component struct has a 16-byte alignment, while the Dyn has none. So converting the Weak to a VRc and then calling borrow() would use the wrong instance pointer. To fix this, this patch introduces an extra offset field that's preserved during the casting and that's used for borrow() as well as drop(). --- api/sixtyfps-cpp/include/vtable.h | 3 ++- helper_crates/vtable/src/vrc.rs | 36 ++++++++++++++++++++----------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/api/sixtyfps-cpp/include/vtable.h b/api/sixtyfps-cpp/include/vtable.h index f7678f3ff..b39e0319d 100644 --- a/api/sixtyfps-cpp/include/vtable.h +++ b/api/sixtyfps-cpp/include/vtable.h @@ -69,6 +69,7 @@ private: const VTable *vtable = &X::component_type; int strong_ref = 1; int weak_ref = 1; + std::uint16_t data_offset = offsetof(VRcInner, data); union { X data; Layout layout; @@ -86,7 +87,7 @@ public: ~VRc() { if (!--inner->strong_ref) { Layout layout = inner->vtable->drop_in_place({inner->vtable, &inner->data}); - layout.size += sizeof(const VTable *) + 2 * sizeof(int); + layout.size += inner->data_offset; layout.align = std::max(layout.align, alignof(VRcInner)); inner->layout = layout; if (!--inner->weak_ref) { diff --git a/helper_crates/vtable/src/vrc.rs b/helper_crates/vtable/src/vrc.rs index 3f54382c1..1002eafd9 100644 --- a/helper_crates/vtable/src/vrc.rs +++ b/helper_crates/vtable/src/vrc.rs @@ -50,18 +50,29 @@ impl core::convert::TryFrom for core::alloc::Layout { } #[repr(C)] -struct VRcInner<'vt, VTable: VTableMeta, X: ?Sized> { +struct VRcInner<'vt, VTable: VTableMeta, X> { vtable: &'vt VTable::VTable, /// The amount of VRc pointing to this object. When it reaches 0, the object will be droped strong_ref: Cell, /// The amount of VWeak +1. When it reaches 0, the memory will be deallocated. /// The +1 is there such as all the VRc together hold a weak reference to the memory weak_ref: Cell, + /// offset to the data from the beginning of VRcInner. This is needed to cast a VRcInner + /// to VRcInner as "dyn VRc" and then still be able to get the correct data pointer, + /// since the alignment of X may not be the same as u8. + data_offset: u16, /// Actual data, or an instance of `Self::Layout` iff `strong_ref == 0`. /// Can be seen as `union {data: X, layout: Layout}` (but that's not stable) data: X, } +impl<'vt, VTable: VTableMeta, X> VRcInner<'vt, VTable, X> { + fn data_ptr(&self) -> *const X { + let ptr = self as *const Self as *const u8; + unsafe { ptr.add(self.data_offset as usize) as *const X } + } +} + /// A reference counted pointer to an object matching the virtual table `T` /// /// Similar to [`std::rc::Rc`] where the `VTable` type parameter is a VTable struct @@ -74,17 +85,17 @@ struct VRcInner<'vt, VTable: VTableMeta, X: ?Sized> { /// safe to get a Pin reference with `borrow_pin`. /// - It is safe to pass it accross ffi boundaries. #[repr(transparent)] -pub struct VRc { +pub struct VRc { inner: NonNull>, } -impl Drop for VRc { +impl Drop for VRc { fn drop(&mut self) { unsafe { let inner = self.inner.as_ref(); inner.strong_ref.set(inner.strong_ref.get() - 1); if inner.strong_ref.get() == 0 { - let data = &inner.data as *const _ as *const u8 as *mut u8; + let data = inner.data_ptr() as *const _ as *const u8 as *mut u8; let mut layout = VTable::drop_in_place(inner.vtable, data); layout = core::alloc::Layout::new::>() .extend(layout.try_into().unwrap()) @@ -129,8 +140,11 @@ impl> VRc { vtable: X::static_vtable(), strong_ref: Cell::new(1), weak_ref: Cell::new(1), // All the VRc together hold a weak_ref to the memory + data_offset: 0, data, }); + (*mem).data_offset = + (&(*mem).data as *const _ as usize - mem as *const _ as usize) as u16; VRc { inner } } } @@ -146,7 +160,7 @@ impl> VRc { } } -impl VRc { +impl VRc { /// Create a Pinned reference to the inner. /// /// This is safe because we don't allow mutable reference to the inner @@ -160,7 +174,7 @@ impl VRc { let inner = this.inner.cast::>(); VRef::from_raw( NonNull::from(inner.as_ref().vtable), - NonNull::from(&inner.as_ref().data), + NonNull::from(&*inner.as_ref().data_ptr()), ) } } @@ -191,7 +205,7 @@ impl VRc { } } -impl Clone for VRc { +impl Clone for VRc { fn clone(&self) -> Self { let inner = unsafe { self.inner.as_ref() }; inner.strong_ref.set(inner.strong_ref.get() + 1); @@ -199,9 +213,7 @@ impl Clone for VRc*/> Deref - for VRc -{ +impl*/> Deref for VRc { type Target = X; fn deref(&self) -> &Self::Target { let inner = unsafe { self.inner.as_ref() }; @@ -217,7 +229,7 @@ impl*/> Der /// Can be constructed with [`VRc::downgrade`] and use [`VWeak::upgrade`] /// to re-create the original VRc. #[repr(transparent)] -pub struct VWeak { +pub struct VWeak { inner: Option>>, } @@ -237,7 +249,7 @@ impl Clone for VWeak { } } -impl Drop for VWeak { +impl Drop for VWeak { fn drop(&mut self) { if let Some(i) = self.inner { let inner = unsafe { i.as_ref() };