Pstd reference-counted slice

code review
1

I spent the last week or so writing this, made all kinds of blunders, I think I now have it correct.

The objective is a reference-counted slice (inc. string), which can be allocated not in Global, available on stable.

Here is the code. Are there still major problems with it? Any other comments? Thanks!

/// Reference-counted slice.
pub struct RcSlice<T, A: Allocator = Global> {
    nn: NonNull<RcSliceInner<A>>,
    pd: PhantomData<T>,
}

struct RcSliceInner<A: Allocator> {
    cc: usize,   // Clone count
    slen: usize, // Length of slice
    a: A,
}

impl<T, A: Allocator> RcSlice<T, A> {
    /// Create a RcSlice from s in specified allocator.
    pub fn new_in(s: &[T], a: A) -> Self
    where
        T: Clone,
    {
        unsafe {
            let slen = s.len();
            let (layout, off) = Self::layout(slen);

            let nn = a.allocate(layout).unwrap();
            let p = nn.as_ptr().cast::<RcSliceInner<A>>();

            let inner = RcSliceInner { cc: 0, slen, a };
            ptr::write(p, inner);

            // Initialise the slice of allocated memory.
            let to = (p as *mut u8).add(off) as *mut MaybeUninit<T>;
            let to = std::slice::from_raw_parts_mut(to, slen);
            to.write_clone_of_slice(s);

            Self {
                nn: NonNull::new_unchecked(p),
                pd: PhantomData,
            }
        }
    }

    /// Get the layout for the inner fields followed by a slice of size slen, and the offset of the slice.
    fn layout(slen: usize) -> (Layout, usize) {
        unsafe {
            Layout::new::<RcSliceInner<A>>()
                .extend(Layout::array::<T>(slen).unwrap_unchecked())
                .unwrap_unchecked()
        }
    }

    /// Get a NonNull pointer to the slice.
    fn slice(&self) -> NonNull<[T]> {
        unsafe {
            let p = self.nn.as_ptr();
            let slen = (*p).slen;
            let (_layout, off) = Self::layout(slen);
            let p = (p as *mut u8).add(off) as *mut T;
            let nn = NonNull::new_unchecked(p);
            NonNull::slice_from_raw_parts(nn, slen)
        }
    }
}

impl<T, A: Allocator> Clone for RcSlice<T, A> {
    fn clone(&self) -> Self {
        unsafe {
            let p = self.nn.as_ptr();
            (*p).cc += 1;
        }
        Self {
            nn: self.nn,
            pd: PhantomData,
        }
    }
}

impl<T, A: Allocator> Drop for RcSlice<T, A> {
    fn drop(&mut self) {
        unsafe {
            let p = self.nn.as_ptr();
            if (*p).cc == 0 {
                self.slice().drop_in_place();
                let slen = (*p).slen;
                let a = ptr::read(&(*p).a);
                let (layout, _off) = Self::layout(slen);
                let nn = NonNull::new_unchecked(p as *mut u8);
                a.deallocate(nn, layout)
            } else {
                (*p).cc -= 1;
            }
        }
    }
}

impl<T, A: Allocator> Deref for RcSlice<T, A> {
    type Target = [T];
    fn deref(&self) -> &[T] {
        unsafe { self.slice().as_ref() }
    }
}
2

I think it currently leaks memory if a slice element panics during drop_in_place. I maybe do not want to handle that, I consider such panics to be pathological, but I should probably document it ( or fix it ).

3

I think it would be a good idea to deny clippy::undocumented_unsafe_blocks and add safety comments to each unsafe block.

Also would probably be good to make every unsafe block as small as possible.

1 Like
4

Putting several lines, that do not constitute unsafe operations inside an unsafe block is a code smell to me. It is equivalent to wrapping the entire function body in a try block in programming languages with exception handling. Only expressions or statements that are unsafe should be within documented unsafe blocks.

2 Likes
5

When say 3 out of 6 lines are unsafe, I think it can end up making the code harder to read.

Safety comments seem a good idea, but I am not sure they help a lot (in this instance). Although maybe the process of writing them would help catch things.

The main potential mistakes here ( some of which I managed to make ) seem to be

(1) Failing to remember to drop things ( of course this is not unsafe, just wrong). It is quite easy to do this as in safe Rust you do not have to remember as it mostly happens automatically.

(2) Failing to clone things which need cloning. It is quite easy to do a copy when a clone is needed, for types that are not T:Copy.

(3) Not using write and read in the right places.

(4) Getting Layout wrong or getting pointer arithmetic wrong ( I think this is not so likely, I don’t remember doing this ).

(5) Borrow errors.

(6) Use after (or during) free.

If you have suitable tests, Miri can find most or all of these, but only if you think to write a test that will catch the mistake, but if you made the mistake you quite likely would not think to write the necessary test.