jonhoo / jonhoo/evmap

ReadHandle (and others) are unsound due to missing Send and Sync bounds

Open
#12 5 comments 0 reactions 0 assignees View on GitHub
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.