Skip to content

RootedGuard<T> construction related unsoundness #564

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

Merged
merged 1 commit into from
Mar 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions mozjs-sys/src/jsimpls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,12 @@ impl JSNativeWrapper {
}

impl RootedBase {
unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext, kind: JS::RootKind) {
unsafe fn add_to_root_stack(this: *mut Self, cx: *mut JSContext, kind: JS::RootKind) {
let stack = Self::get_root_stack(cx, kind);
self.stack = stack;
self.prev = *stack;
(*this).stack = stack;
(*this).prev = *stack;

*stack = self as *mut _ as usize as _;
*stack = this as usize as _;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: This cast is a roundtrip. I assume the intent is to expose the provenance.

I don't believe that should be necessary, but given that there are currently a lot of uses of deref and deref_mut on this type, creating &T and &mut T types across GC barriers, I'm not willing to change it. It looks like the sort of thing someone did to stop the compiler from performing some optimizations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure there was a note in one of these cast hoops, but I cannot seem to find it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That usize -> _ cast was introduced in d40cfd5#diff-e27e06105da7e8ebf851404e43b36501f85aaf53097cdccf96b9f86c8a7cf1daR392 and I'm pretty sure it was needed since stackRoots_ is a u64 value, so it was doing usize -> u64.

}

unsafe fn remove_from_root_stack(&mut self) {
Expand Down Expand Up @@ -440,8 +440,9 @@ impl<T: RootKind> JS::Rooted<T> {
}
}

pub unsafe fn add_to_root_stack(&mut self, cx: *mut JSContext) {
self.base.add_to_root_stack(cx, T::KIND)
pub unsafe fn add_to_root_stack(this: *mut Self, cx: *mut JSContext) {
let base = unsafe { &raw mut (*this).base };
RootedBase::add_to_root_stack(base, cx, T::KIND)
}

pub unsafe fn remove_from_root_stack(&mut self) {
Expand Down
3 changes: 2 additions & 1 deletion mozjs/src/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use crate::rust::{HandleValue, MutableHandleValue};
use crate::rust::{ToBoolean, ToInt32, ToInt64, ToNumber, ToUint16, ToUint32, ToUint64};
use libc;
use log::debug;
use mozjs_sys::jsgc::Rooted;
use std::borrow::Cow;
use std::mem;
use std::rc::Rc;
Expand Down Expand Up @@ -664,7 +665,7 @@ struct ForOfIteratorGuard<'a> {
impl<'a> ForOfIteratorGuard<'a> {
fn new(cx: *mut JSContext, root: &'a mut ForOfIterator) -> Self {
unsafe {
root.iterator.add_to_root_stack(cx);
Rooted::add_to_root_stack(&raw mut root.iterator, cx);
}
ForOfIteratorGuard { root }
}
Expand Down
29 changes: 17 additions & 12 deletions mozjs/src/gc/root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,21 @@ use mozjs_sys::jsgc::ValueArray;
crown::unrooted_must_root_lint::allow_unrooted_interior
)]
pub struct RootedGuard<'a, T: 'a + RootKind> {
root: &'a mut Rooted<T>,
root: *mut Rooted<T>,
anchor: PhantomData<&'a mut Rooted<T>>,
}

impl<'a, T: 'a + RootKind> RootedGuard<'a, T> {
pub fn new(cx: *mut JSContext, root: &'a mut Rooted<T>, initial: T) -> Self {
root.ptr.write(initial);
unsafe {
root.add_to_root_stack(cx);
let root: *mut Rooted<T> = root;
Rooted::add_to_root_stack(root, cx);
RootedGuard {
root,
anchor: PhantomData,
}
}
RootedGuard { root }
}

pub fn handle(&'a self) -> Handle<'a, T> {
Expand All @@ -46,31 +51,31 @@ impl<'a, T: 'a + RootKind> RootedGuard<'a, T> {
T: Copy,
{
// SAFETY: The rooted value is initialized as long as we exist
unsafe { self.root.ptr.assume_init() }
unsafe { (*self.root).ptr.assume_init() }
}

pub fn set(&mut self, v: T) {
// SAFETY: The rooted value is initialized as long as we exist
unsafe {
// Make sure the drop impl for T is called
self.root.ptr.assume_init_drop()
(*self.root).ptr.assume_init_drop();
(*self.root).ptr.write(v);
}
self.root.ptr.write(v);
}
}

impl<'a, T: 'a + RootKind> Deref for RootedGuard<'a, T> {
type Target = T;
fn deref(&self) -> &T {
// SAFETY: The rooted value is initialized as long as we exist
unsafe { self.root.ptr.assume_init_ref() }
unsafe { (*self.root).ptr.assume_init_ref() }
}
}

impl<'a, T: 'a + RootKind> DerefMut for RootedGuard<'a, T> {
fn deref_mut(&mut self) -> &mut T {
// SAFETY: The rooted value is initialized as long as we exist
unsafe { self.root.ptr.assume_init_mut() }
unsafe { (*self.root).ptr.assume_init_mut() }
}
}

Expand All @@ -79,19 +84,19 @@ impl<'a, T: 'a + RootKind> Drop for RootedGuard<'a, T> {
// SAFETY: The rooted value is initialized as long as we exist
unsafe {
// Make sure the drop impl for T is called
self.root.ptr.assume_init_drop()
(*self.root).ptr.assume_init_drop();
(*self.root).ptr = MaybeUninit::zeroed();
}
self.root.ptr = MaybeUninit::zeroed();

unsafe {
self.root.remove_from_root_stack();
(*self.root).remove_from_root_stack();
}
}
}

impl<'a, const N: usize> From<&RootedGuard<'a, ValueArray<N>>> for JS::HandleValueArray {
fn from(array: &RootedGuard<'a, ValueArray<N>>) -> JS::HandleValueArray {
JS::HandleValueArray::from(&*array.root)
JS::HandleValueArray::from(unsafe { &*array.root })
}
}

Expand Down