Memory fences and general improvements

Quoted from u/TDplay from this comment.

Todo

Quote

Line 245 in 89f3eb2

let weak =

Not entirely sure about this part. You seem to decrement and check the weak count, and then construct and drop a Weak (thus calling Weak::drop, which should also decrement and check the weak count).

It also seems like you only drop the data when the weak count reaches zero, which seems to defeat the point of supporting Weak pointers.

Another point is to perhaps consider using ptr::drop_in_place instead of ptr::read. It makes the intent more obvious, and it can make some optimisations more obvious to the compiler (since it doesn't need to reason about removing the copy). I think this would also allow you to support dynamically-sized types.

Trc::drop looks fine.

trc/src/trc.rs

Line 1148 in 89f3eb2

pub fn to_trc(this: &Self) -> Option<Trc<T>> {

Weak::to_trc has a race condition, leading to a possible use-after-free.

Initial state: We have a SharedTrc in thread A and a Weak in thread B.

Thread B calls Weak::to_trc

Weak::to_trc checks the strong count, and reads 1

Thread A drops the SharedTrc, decrementing the strong count.

SharedTrc::drop notices that the strong count is now zero, so it deallocates the data.

Weak::to_trc increments the strong count and constructs a Trc.

The Trc is now passed to user code, which access the contents, causing a use-after-free.

To resolve this, the zero-check and the increment need to be one atomic operation, and the increment needs to not happen if the zero check fails. fetch_update is perfect for this. Sadly, this is slower than fetch_add, but I can't see a way to avoid it, and it seems the standard library developers couldn't either.

trc/src/trc.rs

Line 1191 in 89f3eb2

if prev > MAX_REFCOUNT {

Clone for Weak technically has a race condition: if a program clones an absurd number of Weaks in different threads, with thread being pre-empted before it can call abort, it may be possible to overflow the weak count. In practice, I don't think any real program will ever do this, and the actual likelihood of this behaviour is slim.

That's the big stuff out of the way, on to the small nitpicks:

Clone is not implemented for SharedTrc, this feels like an oversight.

You might be able to support no_std. Add this to your lib.rs:

#![cfg_attr(not(test), no_std)]

extern crate alloc;
#[cfg(feature = "force_lock")]
extern crate std;

It will then throw errors at every use of std. Replace these with either core or alloc.

trc/src/trc.rs

Line 899 in 89f3eb2

    
           /// Create a new `Trc<T>` from the provided data. This is equivalent to calling `Trc::new` on the same data.

Documentation for Trc::cmp doesn't seem right.

trc/src/trc.rs

Line 793 in 89f3eb2

impl<T: Pointer> Pointer for Trc<T> {

Behaviour of the Pointer formatting seems a little strange. Trc is a pointer, so it should probably print the address the Trc points to. This would also be in-line with what the standard library smart pointers do: Arc, Rc, Box, &T, and &mut T all implement Pointer unconditionally.

trc/src/trc.rs

Line 1043 in 89f3eb2

let layout = unsafe { Layout::from_size_align_unchecked(size, align) };

There is no need for the unsafe call to Layout::from_size_align_unchecked, you can just use:

Layout::new::<SharedTrcInternal<T>>()

If you choose to support dynamically sized types, then you can instead use:

Layout::for_value

impl Clone for SharedTrc: Clones do not need any special sync because they can only be created from existing objects. However, refcount bounds checking is necessary.

Closed as all problems are fixed.