Fix unsound use of Rooted<T> with RootKind::Traceable (#514)

* Add vtable to Rooted<T> when T has a Traceable RootKind value. Signed-off-by: Josh Matthews <[email protected]> * Update version number. Signed-off-by: Josh Matthews <[email protected]> * Automatically use Traceable-compatible Rooted with any T that implements TraceableTrace. Signed-off-by: Josh Matthews <[email protected]> * Make TraceableTrace::do_trace unsafe. Signed-off-by: Josh Matthews <[email protected]> --------- Signed-off-by: Josh Matthews <[email protected]>
servo · Oct 28, 2024 · f7c263b · f7c263b
1 parent a02aaf1
commit f7c263b
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 71 deletions.
diff --git a/mozjs-sys/Cargo.toml b/mozjs-sys/Cargo.toml
@@ -2,7 +2,7 @@
 name = "mozjs_sys"
 description = "System crate for the Mozilla SpiderMonkey JavaScript engine."
 repository.workspace = true
-version = "0.128.3-0"
+version = "0.128.3-1"
 authors = ["Mozilla"]
 links = "mozjs"
 build = "build.rs"

diff --git a/mozjs-sys/src/jsgc.rs b/mozjs-sys/src/jsgc.rs
@@ -2,91 +2,143 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use crate::glue::CallPropertyDescriptorTracer;
+use crate::jsapi::js::TraceValueArray;
 use crate::jsapi::JS;
 use crate::jsapi::{jsid, JSFunction, JSObject, JSScript, JSString, JSTracer};
 
 use crate::jsid::VoidId;
 use std::cell::UnsafeCell;
-use std::ffi::c_void;
+use std::ffi::{c_char, c_void};
 use std::mem;
 use std::ptr;
 
 /// A trait for JS types that can be registered as roots.
 pub trait RootKind {
-    #[allow(non_snake_case)]
-    /// Returns the rooting kind for `Self`.
-    fn rootKind() -> JS::RootKind;
+    type Vtable;
+    const VTABLE: Self::Vtable;
+    const KIND: JS::RootKind;
 }
 
 impl RootKind for *mut JSObject {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Object
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Object;
 }
 
 impl RootKind for *mut JSFunction {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Object
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Object;
 }
 
 impl RootKind for *mut JSString {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::String
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::String;
 }
 
 impl RootKind for *mut JS::Symbol {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Symbol
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Symbol;
 }
 
 impl RootKind for *mut JS::BigInt {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::BigInt
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::BigInt;
 }
 
 impl RootKind for *mut JSScript {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Script
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Script;
 }
 
 impl RootKind for jsid {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Id
-    }
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Id;
 }
 
 impl RootKind for JS::Value {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Value
+    type Vtable = ();
+    const VTABLE: Self::Vtable = ();
+    const KIND: JS::RootKind = JS::RootKind::Value;
+}
+
+impl<T: TraceableTrace> RootKind for T {
+    type Vtable = *const RootedVFTable;
+    const VTABLE: Self::Vtable = &<Self as TraceableTrace>::VTABLE;
+    const KIND: JS::RootKind = JS::RootKind::Traceable;
+}
+
+/// A vtable for use in RootedTraceable<T>, which must be present for stack roots using
+/// RootKind::Traceable. The C++ tracing implementation uses a virtual trace function
+/// which is only present for C++ Rooted<T> values that use the Traceable root kind.
+#[repr(C)]
+pub struct RootedVFTable {
+    #[cfg(windows)]
+    pub padding: [usize; 1],
+    #[cfg(not(windows))]
+    pub padding: [usize; 2],
+    pub trace: unsafe extern "C" fn(this: *mut c_void, trc: *mut JSTracer, name: *const c_char),
+}
+
+impl RootedVFTable {
+    #[cfg(windows)]
+    pub const PADDING: [usize; 1] = [0];
+    #[cfg(not(windows))]
+    pub const PADDING: [usize; 2] = [0, 0];
+}
+
+/// `Rooted<T>` with a T that uses the Traceable RootKind uses dynamic dispatch on the C++ side
+/// for custom tracing. This trait provides trace logic via a vtable when creating a Rust instance
+/// of the object.
+pub unsafe trait TraceableTrace: Sized {
+    const VTABLE: RootedVFTable = RootedVFTable {
+        padding: RootedVFTable::PADDING,
+        trace: Self::trace,
+    };
+
+    unsafe extern "C" fn trace(this: *mut c_void, trc: *mut JSTracer, _name: *const c_char) {
+        let rooted = this as *mut Rooted<Self>;
+        let rooted = rooted.as_mut().unwrap();
+        Self::do_trace(&mut rooted.ptr, trc);
     }
+
+    /// Used by `TraceableTrace` implementer to trace its contents.
+    /// Corresponds to virtual `trace` call in a `Rooted` that inherits from
+    /// StackRootedTraceableBase (C++).
+    unsafe fn do_trace(&mut self, trc: *mut JSTracer);
 }
 
-impl RootKind for JS::PropertyDescriptor {
-    #[inline(always)]
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Traceable
+unsafe impl TraceableTrace for JS::PropertyDescriptor {
+    unsafe fn do_trace(&mut self, trc: *mut JSTracer) {
+        CallPropertyDescriptorTracer(trc, self);
     }
 }
 
+// The C++ representation of Rooted<T> inherits from StackRootedBase, which
+// contains the actual pointers that get manipulated. The Rust representation
+// also uses the pattern, which is critical to ensuring that the right pointers
+// to Rooted<T> values are used, since some Rooted<T> values are prefixed with
+// a vtable pointer, and we don't want to store pointers to that vtable where
+// C++ expects a StackRootedBase.
+#[repr(C)]
+#[derive(Debug)]
+pub struct RootedBase {
+    pub stack: *mut *mut RootedBase,
+    pub prev: *mut RootedBase,
+}
+
 // Annoyingly, bindgen can't cope with SM's use of templates, so we have to roll our own.
 #[repr(C)]
 #[derive(Debug)]
-pub struct Rooted<T> {
-    pub stack: *mut *mut Rooted<*mut c_void>,
-    pub prev: *mut Rooted<*mut c_void>,
+pub struct Rooted<T: RootKind> {
+    pub vtable: T::Vtable,
+    pub base: RootedBase,
     pub ptr: T,
 }
 
@@ -213,9 +265,9 @@ impl<const N: usize> ValueArray<N> {
     }
 }
 
-impl<const N: usize> RootKind for ValueArray<N> {
-    fn rootKind() -> JS::RootKind {
-        JS::RootKind::Traceable
+unsafe impl<const N: usize> TraceableTrace for ValueArray<N> {
+    unsafe fn do_trace(&mut self, trc: *mut JSTracer) {
+        TraceValueArray(trc, N, self.get_mut_ptr());
     }
 }
 

diff --git a/mozjs-sys/src/jsglue.cpp b/mozjs-sys/src/jsglue.cpp
@@ -943,6 +943,10 @@ void CallValueRootTracer(JSTracer* trc, JS::Value* valp, const char* name) {
   JS::TraceRoot(trc, valp, name);
 }
 
+void CallPropertyDescriptorTracer(JSTracer* trc, JS::PropertyDescriptor* desc) {
+  desc->trace(trc);
+}
+
 bool IsDebugBuild() {
 #ifdef JS_DEBUG
   return true;

diff --git a/mozjs-sys/src/jsimpls.rs b/mozjs-sys/src/jsimpls.rs
@@ -20,7 +20,7 @@ use crate::jsapi::JSPropertySpec_Kind;
 use crate::jsapi::JSPropertySpec_Name;
 use crate::jsapi::JS;
 use crate::jsapi::JS::Scalar::Type;
-use crate::jsgc::RootKind;
+use crate::jsgc::{RootKind, RootedBase};
 use crate::jsid::VoidId;
 use crate::jsval::UndefinedValue;
 
@@ -383,42 +383,49 @@ impl JSNativeWrapper {
     }
 }
 
-impl<T> JS::Rooted<T> {
-    pub fn new_unrooted() -> JS::Rooted<T> {
-        JS::Rooted {
-            stack: ptr::null_mut(),
-            prev: ptr::null_mut(),
-            ptr: unsafe { std::mem::zeroed() },
-        }
+impl RootedBase {
+    unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext, kind: JS::RootKind) {
+        let stack = Self::get_root_stack(cx, kind);
+        self.stack = stack;
+        self.prev = *stack;
+
+        *stack = self as *mut _ as usize as _;
     }
 
-    unsafe fn get_rooting_context(cx: *mut JSContext) -> *mut JS::RootingContext {
-        cx as *mut JS::RootingContext
+    unsafe fn remove_from_root_stack(&mut self) {
+        assert!(*self.stack == self as *mut _ as usize as _);
+        *self.stack = self.prev;
     }
 
-    unsafe fn get_root_stack(cx: *mut JSContext) -> *mut *mut JS::Rooted<*mut c_void>
-    where
-        T: RootKind,
-    {
-        let kind = T::rootKind() as usize;
+    unsafe fn get_root_stack(cx: *mut JSContext, kind: JS::RootKind) -> *mut *mut RootedBase {
+        let kind = kind as usize;
         let rooting_cx = Self::get_rooting_context(cx);
         &mut (*rooting_cx).stackRoots_[kind] as *mut _ as *mut _
     }
 
-    pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext)
-    where
-        T: RootKind,
-    {
-        let stack = Self::get_root_stack(cx);
-        self.stack = stack;
-        self.prev = *stack;
+    unsafe fn get_rooting_context(cx: *mut JSContext) -> *mut JS::RootingContext {
+        cx as *mut JS::RootingContext
+    }
+}
 
-        *stack = self as *mut _ as usize as _;
+impl<T: RootKind> JS::Rooted<T> {
+    pub fn new_unrooted() -> JS::Rooted<T> {
+        JS::Rooted {
+            vtable: T::VTABLE,
+            base: RootedBase {
+                stack: ptr::null_mut(),
+                prev: ptr::null_mut(),
+            },
+            ptr: unsafe { std::mem::zeroed() },
+        }
+    }
+
+    pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) {
+        self.base.add_to_root_stack(cx, T::KIND)
     }
 
     pub unsafe fn remove_from_root_stack(&mut self) {
-        assert!(*self.stack == self as *mut _ as usize as _);
-        *self.stack = self.prev;
+        self.base.remove_from_root_stack()
     }
 }