ReadHandle (and others) are unsound due to missing Send and Sync bounds
- Dominant language
- Rust
- Stars
- 576
- Forks
- 19
- PR merge metrics
- No merged PRs in 30d
Description
Unfortunately, due to an oversight, `Inner` doesn't inherit any requirements on `Send` / `Sync` to be `Send / Sync`, so `ReadHandle` is unconditionally `Send`. This is because AtomicPtr has no Send bound on its `T` (I understand why, because technically you can use it from safe code with a non-Send type), so the auto-Send/Sync bound didn't get applied for K, V, M, or S; but in practice, most unsafe code will actually dereference the pointer at some point so this is usually wrong in that case. It might be nice to have some on-by-default lint for use of AtomicPtr without manually checking the appropriate bounds).
Just not having bounds on `Send` wouldn't necessarily make `ReadHandle` unsound, but I believe it is. `ReadHandle` effectively owns a `MapReadRef` which is (AFAIK, correctly) only `Send` when K, V, M, and S are `Sync` (fortunately, I think they don't need to be `Send` since you can't get an owned instance of any of those out of a `MapReadRef` directly, so I believe that part is sound). Since `ReadHandle` owns (and provides access to) `MapReadRef`, it follows that `ReadHandle: Send` should at least require `K, V, M, S : Sync`.
There are a few other ways to access values of the generic types in question on `ReadHandle` besides via the `MapReadRef`, but from what I saw none of them provide owned access to any of K, V, M, or S (at least, not owned access that calls any safe trait methods or hands them to the user, though maybe internally it does something like that), so I think just the `Sync` bound on the type parameters is also sufficient for safety of `ReadHandle: Send`, not `Send+Sync` on the type parameters, which is actually somewhat unusual for stuff like this (`Sync` containing `!Sync` is much more common than `Send` containing `!Send`). The weak bounds are due (I think) to the fact that the reader types don't ever call `Drop` on values of the owned type, only the writer type does, and there's nothing like `Arc::make_mut` and friends, so evmap implements a "weaker" form of sharing than `Arc` does.
The unsoundness of at least one other type (`ReadHandleFactory`) follows from this same root cause of using an `AtomicPtr` (arguably, it's worse, since it's not only unconditionally `Send` but also unconditionally `Sync`) and needs to be fixed in the same way (`Arc` requires `T: Send+Sync` for both `Send` and `Sync`, and the `AtomicPtr` is in an `Arc`, so you don't need to explicitly `impl !Send` here).
More subtly, `WriteHandle` is also unsound for the same reason (this should get fixed automatically if `ReadHandle `is, since it owns a `ReadHandle`). At first, I actually thought `WriteHandle` *should* be able to be made sound without changing its `Send` requirements from what they currently are (which would require an explicit `Send` impl if the `ReadHandle` fix were added), because you can only write/drop through a unique `WriteHandle`, and you can't write at the same time as the `Deref` implementation on `WriteHandle` is active. Unfortunately, (1) we always create a ReadHandle on startup for some reason (despite the fact that WriteHandle already owns one internally?), and (2) even if we didn't, the `Deref` implementation on `WriteHandle` lets you clone the `ReadHandle` (though (2) is technically solvable by putting the relevant `Sync` bounds on the `Deref` implementation, or by not having the `Deref` implementation and exposing access to the non-`clone` / `factory` methods via wrappers instead). So `WriteHandle` should really just inherit the extra bounds from its owned `ReadHandle`.
There are probably a few other other types with the same issue.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.