Possible Unsafe Memory Access Without Safety Justification in node_vec.rs

[yaokunzhang]

Issue Summary

Thank you for appreciating this excellent open-source project.
Our static analysis tool has identified potential safety issues in the node_vec.rs file. The node_vec.rs file contains potentially unsafe code patterns that bypass Rust’s safety guarantees without appropriate justification or documentation. These issues were detected by a static analysis tool and are primarily located in the non_null_node and make_node functions.

Details

non_null_node Function：

pub(crate) fn non_null_node(&self, index: usize) -> NonNull<Node<T>> {
    unsafe {
        NonNull::new_unchecked(
            self.buf.get_unchecked(index)
                .try_borrow_unguarded()
                .unwrap() as *const Node<T> as *mut Node<T>
        )
    }
}

Uses get_unchecked to bypass bounds checking

make_node Function：

pub(crate) fn make_node(
    &mut self,
    parent: Option<NonNull<Node<T>>>,
    index: usize,
    data: Data<T>,
    size: Size
) -> NonNull<Node<T>> {
    unsafe {
        let node = self.buf.get_unchecked_mut(index);
        // ...
    }
}

Uses get_unchecked_mut to skip bounds checking

Recommendation

To ensure memory safety and uphold Rust’s safety principles:

• Consider marking these functions as unsafe fn if the caller must uphold certain invariants.
• Add detailed documentation comments explaining the safety requirements for these functions.

[aminya]

The functions are private to the crate, and it seems all the callsites are accessing already existing indices. Is there an instance where the callsite is accessing an out of the bound element? However, given the number of calls to these functions, it makes sense to keep it checked to prevent potential mistakes causing memory access.

[yaokunzhang]

Thanks for your reply. As far as I know, generally Rust functions that need to guarantee certain safety invariants are marked as unsafe. This seems more idiomatic/standard.