Unsoundness problem in Node::tree

[lwz23]

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 Node {
    // The actual tree we belong to. This is unsafe!!
    pub tree: *mut Slab<Node>,

    /// Our Id
    pub id: usize,
    /// Our parent's ID
    pub parent: Option<usize>,
................................
}
impl Node {
    pub fn tree(&self) -> &Slab<Node> {
        unsafe { &*self.tree }
    }
...........................
}

Considering that tree is a pub field, I assume that users can directly manipulate this field. This potential situation could result in self.tree being a null pointer, and directly dereferencing it might trigger undefined behavior (UB).
PoC:

extern crate blitz_dom;
extern crate style;

use blitz_dom::node::Node;
use std::ptr;
use style::{
    data::ElementData,
    properties::{parse_style_attribute, PropertyDeclarationBlock},
    servo_arc::Arc as ServoArc,
    shared_lock::{Locked, SharedRwLock},
    stylesheets::CssRuleType,
};
use blitz_dom::NodeData;
fn main() {
    let node = Node::new(ptr::null_mut(),0,SharedRwLock::new(),NodeData::Document);
    let _=node.tree();
}

If there is no external using for Node, maybe it should not be marked as pub, at least for its field should not be mark as pub.

[nicoburns]

Good point. I'll make it private. I think it was initially private, but was made public to support a cross-crate access that is no longer necessary.

[lwz23]

Hello again, I think maybe current fix is not sufficient :(
because I notice the following function:

blitz/packages/blitz-dom/src/node.rs

Line 97 in 017f8ea

pub fn new(tree: *mut Slab<Node>, id: usize, guard: SharedRwLock, data: NodeData) -> Self {

the user can still pass a Null pointer through the new method
maybe we should mark new method as private or add check to make sure the pointer is not null?

[nicoburns]

I've made the new function private in e12a3c2. But I'll leave this open for now in case there are other issues. That tree pointer is definitely a source of unsafety I'd like to eliminate at some point. But Stylo's design makes that tricky.

[lwz23]

Thanks, I will keep search if there are other cases. If I found, I will report it in this issue.

[lwz23]

Also, given that this is an Unsound issue, I'm not sure if we should report it to RustSec?

[nicoburns]

Also, given that this is an Unsound issue, I'm not sure if we should report it to RustSec?

I think we should probably avoid that for now. Blitz doesn't have any real users yet, so they won't have run into it. And our wrapper libraries were not triggering the unsafety.

[lwz23]

Ok, I'm not quite familiar with the requirements of the report, thanks for clarification.