Make keys Send and Sync, remove Clone
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 42
- PR merge metrics
- No merged PRs in 30d
Description
To err on the safe side, we initially made our key objects neither `Send` nor `Sync`. However, BoringSSL provides concurrency semantics for key objects that allow us to relax these restrictions. In particular, key objects' reference counts are incremented and decremented atomically, and all BoringSSL operations on keys have well-defined concurrency safety semantics.
I propose that we implement `Sync` on key objects, and use BoringSSL's definition of which functions are mutating to decide which methods to have take a `&mut self` vs a `&self` receiver.
## An aside on `Clone` and reference counting
Currently, keys are reference counted, and cloning a Mundane key object obtains a new reference-counted reference to the same underlying object. Unfortunately, we cannot implement either `Send` or `Sync` so long as it's possible to obtain multiple references to the same underlying object.
If reference-counted key objects are `Send`, then different clones could be sent to different threads, and we'd have no way of preventing those two separate clones from being operated on using `&mut self` methods concurrently, which would be unsound.
If reference-counted key objects are `Sync`, then different clones owned by one thread could be accessed concurrently from different threads, and we'd have no way of preventing those two separate clones from being operated on using `&mut self` methods concurrently, which would be unsound.
Thus, I conclude that **we must remove the ability to clone key objects**. The primary benefit to reference counting in BoringSSL is to be able to use keys concurrently from multiple threads at once. Since Rust's lifetime system allows us to share references across threads safely, we get the same advantage even without reference counting. Even if reference counting were desired, we could put a Mundane key object inside of an `Rc` or an `Arc` and get the same effect.
Thus, the concrete tasks are:
- [ ] Remove the ability to `Clone` key objects
- [ ] Audit the codebase to ensure that all mutating BoringSSL functions are exposed via `&mut self` methods, and document our strategy and reasoning in code comments
- [ ] Implement `Send` and `Sync` on key objects
Old (incorrect) text:
> BoringSSL key types are reference-counted, and use reference counting to implement `Clone`. While the reference counting itself is thread-safe (see `CRYPTO_refcount_xxx`, [crypto/internal.h](https://boringssl.googlesource.com/boringssl/+/master/crypto/internal.h)), it's not clear that all operations on keys are also thread-safe. In other words, having two key objects in different threads which are both references to the same underlying BoringSSL object may mean that calling methods on those objects concurrently is unsound. As a result, our key objects do not implement `Send`.
>
> Eventually, we will want to identify which methods are thread-safe and which are not. This is not only a prerequisite for making our key objects `Send`, it's also a prerequisite for making them `Sync`. However, we can much more easily unblock making our key objects `Send` by just not implementing `Clone` so that a given key object is always the only reference to its underlying BoringSSL object.
Contributor guide
Research direction
Start by locating the key object implementations and reviewing BoringSSL's concurrency and reference-counting guidance in crypto/internal.h. Audit each key operation against the issue's mutable-versus-shared receiver requirement, then remove Clone and add Send and Sync. Done means the audit is documented in code comments and all three listed tasks are complete.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cryptography
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100