Built a crate to safely share `Box` and `Vec` manually

code review
1

Repository: GitHub - newDINO/manual-share: A crate that helps sharing rust Box and Vec with other threads. · GitHub

Documentation: manual_share - Rust

Currently not published to crates.io, hoping for some code review.

2

I wouldn’t use this myself, since I don’t love how leak-prone it is… but giving feedback anyway:

  • “No overflow check is needed because creating more than usize::MAX SharedBoxRef is impossible.” Nope, very much possible: create and mem::forget SharedBoxRefs in a loop. Even on 64-bit targets, the compiler might be able to optimize “loop usize::MAX times, one refcount increment per iteration” into “set the refcount to usize::MAX”. Then one more call triggers overflow and unsoundness.
  • There are Send impls, but you’re missing Sync impls. (This isn’t unsound, just unnecessarily restrictive to always be !Sync.)
3

Just gave a quick look at SharedBox:

  • SharedBox should require T: Send + Sync in order to be Send, because you can create a ref, send the SharedBox and then you can access T from multiple threads
  • SharedBox::try_return uses ptr equality which can return true for a SharedBoxRef created for a different SharedBox if T is a ZST
4

(Edit: I was beat to the punch here.) Also, ZSTs might be A Problem for your usage of ptr::eq. ZSTs are… somewhat special, so I’m not sure how to trigger UB with that. At the very least, a panic can occur after the borrow count underflows. Many std collections have different behavior for ZSTs, gated with an if.

(For code style: I’d recommend use imports instead of long qualified paths everywhere, which are usually not used in Rust outside of macros.)

5

one could model ZSTs as having their SharedBoxRefs share all the SharedBoxes with the same address, so they are kinda pooling.
this perfectly sound, because no matter the safety invariants that may exist on a ZST, if there are 2 at the same address, they are completely identical, and switching from one to the other is a sound noop.
it may be unexpected to someone reading the code though.

the only way for there to not be a ZST to reference would be for all SharedBoxes to be freed, which (excluding overflow, but including underflow as long as it doesn't go the whole way to zero), can only happen if all SharedBoxRef get returned (not necessarily to their source).
if a SharedBox underflows the situation is still salvageable by making it make SharedBoxRefs until it gets back to 0, and using these on the other SharedBoxes

6

Thanks for responding.
The first one is indeed an issue that can trigger use-after-free (After overflowing, the count becomes zero and the SharedBox can be dropped).
For the second one I'll implement Sync for all types where T: Sync.

7

Thanks for responding.
I don't know what you mean by:

because you can create a ref, send the SharedBox and then you can access T from multiple threads

If you mean create multiple SharedBoxRef, it already requires T: Sync to be Send. If what you mean is using an Arc to wrap it. It also requires SharedBox to be Sync to be Send, which I plan to implement for T: Sync.Thus, no need T: Send + Sync to implement Send for SharedBox.

ptr equality of ZST is actually a problem I haven't thought about. It can indeed cause a lot of issues and may require special treatment.

8

I would argue, the initial claim/motivation to prevent a move is flawed.

While it is technically correct, that creating an Arc from a Box implies moving data into a new allocation, that is no concern in real code.

If someone would like to share a value anyway, one would normally creating an Arc directly, something like:

   let x = Arc::new(SomeThing::new(...));

There is virtually no case to do instead:

   let boxed = Box::new(SomeThing::new(...));
   let x = Arc::new(*boxed);

So for practical code, I don't see the claim of a saved move, because your SharedBox does need one move, too.

9

Thanks for responding.
I think an easy way to deal with ZST is:
when size_of::<T>() == 0, borrow count doesn't increase or decrease.

But this is a completely different behavior compared to non-ZSTs. So the user should be informed.

10

Thanks for responding, but I don't fully understand this. Creating SharedBox just use the allocation of the previous Box, no data copy here.

11 
let x = SharedBox::new(SomeThing::new(...));

is effectively:

let boxed = Box::new(SomeThing::new(...)));
let x = SharedBox::from_box(boxed);

This is one move (SomeThing is moved into the box allocation).

But this is also only one move:

let x = Arc::new(SomeThing::new(...)));

So, no move saved.

12

This is indeed one move (for both SharedBox and Arc).

13

My case is, in real code there is no:

let boxed = Box::new(...);
let x = Arc::new(*boxed);

which is two moves.

Virtually everybody will just write:

let x = Arc::new(...);

which is one move, because, when it is known, that a value should be shared between threads, why create a Box first and convert it into an Arc, when you can just create the Arc directly? There is no reason for a Box.

So I argue, the motivating claim for SharedBox, to save a move (in real, practical code), is flawed. This problem doesn't exist.

14

From this point, the initial motive is indeed flawed. Maybe I should delete it from the docs. This leaves the crate's reason to exist to the pros listed in the documentation.

15

that would not be sound. this would allow SharedBoxRefs to exist, after the SharedBox has been destroyed.

simply checking for overflow and underflow would be good enough.

you could get away with only checking for underflow for ZST types

16

After reviewing your model. I think it is far more reasonable. I think I'll adopt it.
But it is still slightly different behavior, documentation about ZST treatment should be added.

17

for sure

18

you forgot to add ?Sized on SharedBox and SharedBoxRef, which seems like a quite regratable oversight.
vecause type + Metadata uniquely define size of ?Sized types, two pointers can only compare equal if both pointees are ZST or neither are, which is what we want.

allowing the comparison to succeed with only one of the two pointing to a ZST would be unsound.

i would recommend you check the length for SharedVec, because two Vec<ZST>s of different length are noticeably different and having a ref "switch" from one to another could be dangerous, although i am not completely sure how.

what i am sure of is that for the same reason SharedVecMut is unsound in the presence of ZSTs, which requires a smarter check to fix

19

I currently don't know how to implement DST and fat pointer in stable rust (Box<[T]> can still be converted to SharedVec, but losing the ability for Box<dyn Trait> is bad).

For SharedVec<ZST>, adding a length check seems good.

For SharedVecMut, I need to store the original length to check if it is converted to a Vec with different length to due ZST.

20

Hey! I just took a look through the repo, and honestly, this is a really solid piece of engineering. Manual concurrency wrappers are usually a minefield, but your safety invariants look watertight.

A couple of things that stood out to me:

First off, your Send and Sync trait bounds are spot on. A lot of people mess up by forgetting that &mut [T] allows a thread to write a new T into the slice. Since that T will eventually be dropped by the SharedVecMut back on the original thread, requiring T: Send for SharedVecPart is exactly the right call.

Also, I absolutely love the way you handled Zero-Sized Types (ZSTs). Since ZST pointers are essentially identical dangling pointers, the standard pointer equality checks don't stop cross-contamination between different SharedVecs. Using the checked_sub(1) on the borrow count as an underflow defense is super clever. Even if someone maliciously swapped ZST parts, the math guarantees that the total drops will equal the total creations, so it stays 100% memory safe.

Your Drop implementations also look great. Intentionally leaking the memory when borrow_count > 0 is definitely the right move to prevent use-after-free bugs.

The only tiny piece of feedback I have is maybe adding a quick note to the docs for try_into_vec(). Right now, if it returns Err(self) because there are still active borrows, and the user just ignores or drops that error, it’ll trigger the panic-on-drop mechanism. That's totally correct behavior, but it might catch someone off guard if they aren't expecting a panic from dropping an error!

Really great work on this, it's a super clean zero-overhead alternative to Arc<Vec<T>>!