Skip to content

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

@joshuayanovski-okta

Description

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions