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

[joshuayanovski-okta]

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<T> 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.

[jonhoo]

Yes, I think you're completely right! Would you mind filing a PR?

[joshuayanovski-okta]

Good news... I just downloaded the repository to check, and it seems like this issue is already partially fixed (for ReadGuard) in the latest (0.11) version (due to jonhoo/left-right#75 being fixed for left-right). However, it still has some safety issues since I think it missed the root cause, namely ReadHandleFactory has the same issue (and maybe other stuff).

I'll submit a slightly cleaner fix for left-right to fix this for good, but I'm not totally sure how one would go about backporting this to 0.10, since there doesn't even appear to be a feature branch for it anymore (and you indicated you didn't feel like fixing this bug in 0.10 previously). Any suggestions? Is there a reason not to just make 0.11 the latest release and yank 0.10.x?

[jonhoo]

There should be a tag for 0.10, so you should be able to branch off of that. That said, unless you have a desperate need, I'd like to just fix this for 0.11 — there are other things broken in 0.10, and there's no LTS guarantee, so fixing on the latest version seems fine to me. I think I'm also okay landing evmap 0.11 at this point (with said fix) 👍

And yes, a fix to left-right would be lovely!

[banacer]

any 0.11 release coming soon?

[jonhoo]

@joshuayanovski-okta did you ever get a chance to put together a PR to fix this in 0.11?

@banacer There isn't a fix for this in the code base yet I don't think, but I'm happy to release 0.11 final once a fix lands!