mozjs
mozjs copied to clipboard
Maybe unsound in CustomAutoRooterGuard::new
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:
- If there is no external usage for
CustomAutoRooterGuardornew, they should not marked aspub, at least itsnewshould not marked aspub newmethod should add additional check for null pointer.- mark new method as unsafe and proper doc to let users know that they should provide valid Pointers.
same to https://github.com/servo/mozjs/blob/e299b24d2d48e451a47994d7ad1283ea56ed653d/mozjs/src/gc/root.rs#L22
Agree: any method that accepts a raw pointer argument should be marked unsafe.