Maybe unsound in WeakPointer::new

[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 WeakPointer<T: ?Sized> {
    ptr: *mut T,
}

impl<T> WeakPointer<T> {
    pub fn new(ptr: *mut T) -> Self {
        WeakPointer { ptr }
    }
}

impl<T> Deref for WeakPointer<T> {
    type Target = T;

    fn deref(&self) -> &T {
        unsafe { &(*self.ptr) }
    }
}

impl<T> DerefMut for WeakPointer<T> {
    fn deref_mut(&mut self) -> &mut T {
        unsafe { &mut (*self.ptr) }
    }
}

Considering that new is also a pub function. I assume that users can directly call this function. This potential situation could result in *self.ptr being dereference 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 WeakPointer 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]

ping

[michelhe]

Hi, thanks. This is indeed unsafe and intended to be used solely in the context of this project, the reason for pub is merely do to how I divided the code into crates. They crates themselves are not published on crates.io or intended for consumption.

[lwz23]

Thanks for your reply. If is this case maybe define it in other crate and declear it as pub(crate) is more appropriate, it can avoid other potential unsafe usage with no additional cost. And it make the project structure clear. But I aggree it is inconvenient, if it won't publish to crates.io. it is ok to keep it the current state. But from the view of security, i still suggest fix it. Because it don't follow the Rust security policy. Any code declear as safe shouldn't cause UB in any case. Otherwise, it is considered as unsound.