mozjs icon indicating copy to clipboard operation
mozjs copied to clipboard

Maybe unsound in CustomAutoRooterGuard::new

Open lwz23 opened this issue 11 months ago • 2 comments

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project. I notice the following code:

pub struct CustomAutoRooterGuard<'a, T: 'a + CustomTrace> {
    rooter: &'a mut CustomAutoRooter<T>,
}

impl<'a, T: 'a + CustomTrace> CustomAutoRooterGuard<'a, T> {
    pub fn new(cx: *mut JSContext, rooter: &'a mut CustomAutoRooter<T>) -> Self {
        unsafe {
            rooter.add_to_root_stack(cx);
        }
        CustomAutoRooterGuard { rooter }
    }
...........................

Considering that pub mod gc, and new is also a pub function. I assume that users can directly call this function. This potential situation could result in rooter.add_to_root_stack(cx) operating on a null pointer, and directly dereferencing it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue. I suggest Several possible fixes:

  1. If there is no external usage for CustomAutoRooterGuard or new, they should not marked as pub, at least its new should not marked as pub
  2. new method should add additional check for null pointer.
  3. mark new method as unsafe and proper doc to let users know that they should provide valid Pointers.

lwz23 avatar Dec 10 '24 09:12 lwz23

same to https://github.com/servo/mozjs/blob/e299b24d2d48e451a47994d7ad1283ea56ed653d/mozjs/src/gc/root.rs#L22

lwz23 avatar Dec 10 '24 09:12 lwz23

Agree: any method that accepts a raw pointer argument should be marked unsafe.

jdm avatar Dec 10 '24 13:12 jdm