Lib.rs

› AtomicOption › Reviews
RUSTSEC-2020-0113 on 2020-10-31: AtomicOption should have Send + Sync bound on its type argument.

In the affected versions of this crate, AtomicOption<T> unconditionally implements Sync.

This allows programmers to move non-Sync types across thread boundaries (e.g. Rc<T>, Arc<Cell<T>>), which can lead to data races and undefined behavior. It is also possible to send non-Send types like std::sync::MutexGuard to other threads, which can lead to undefined behavior.

CVE-2020-36219

GHSA-8gf5-q9p9-wvmc

This review is from Crev, a distributed system for code reviews. To add your review, set up cargo-crev.

0.1.2 (current) Rating: Negative + Unmaintained Thoroughness: High Understanding: Medium

by yvt on 2021-09-18

This crate provides an atomic cell type named AtomicOption to store Box<T>.

This crate is severely flawed and unsound but not malicious, hence the negative rating.

Marking as unmaintained because of several serious issues being left unfixed for at least a year.

Major issues

Underconstrained memory ordering

AtomicOption's methods allow the caller to specify memory orderings weaker than what would be safe. Consider the following two processes:

   // Process 1:
1  let mut boxed = Box::new(1);
2  *boxed = 2;
3  atom_opt.swap(boxed, Ordering::Relaxed);

                         // Process 2:
                      4  let boxed = atom_opt.take(Ordering::Relaxed).unwrap();
                      5  dbg!(*boxed);

Process 1 performs two writes on *boxed at line numbers 1 and 2. Supposing Process 2 observes the boxed sent by Process 1 at line number 4, it performs one read on *boxed at line number 5. Since the writes and read are not ordered properly, the read creates an undef value.

There's a three year old GitHub issue reporting this problem.

Pointer-to-integer transmutation

This crate uses pointer-to-integer transmutation, which has unresolved issues regarding pointer provenance1. This could be safely avoided by replacing the internally-used AtomicUsize with AtomicPtr.

Incorrect Sync bounds

AtomicOption unconditionally implements Sync, which allows T: !Send to be sent to another thread through &AtomicOption. There's a RustSec advisory regarding this issue.

Minor issues

The documentation of the unsafe fn AtomicOption::from_raw doesn't describe the safe usage.

The use of SeqCst in AtomicOption::drop suggests the author's lack of understanding in atomics and memory ordering.

Good things

The crate is quite small as it contains only about 200 lines (including comments) of Rust code.

Crates in the crates.io registry are tarball snapshots uploaded by crates' publishers. The registry is not using crates' git repositories. There is absolutely no guarantee that the repository URL declared by the crate belongs to the crate, or that the code in the repository is the code inside the published tarball.

To review the actual code of the crate, it's best to use cargo crev open atomic-option. Alternatively, you can download the tarball of atomic-option v0.1.2 or view the source online.