AtomicBucket<T> unconditionally implements Send/Sync

[JOE1994]

Hello,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

AtomicBucket<T> automatically implements Send/Sync unconditionally, due to the fact that Block<T> unconditionally implements Send/Sync.

// https://docs.rs/metrics-util/0.6.2/src/metrics_util/bucket.rs.html#99-100
unsafe impl<T> Send for Block<T> {}
unsafe impl<T> Sync for Block<T> {}

Unconditional impl of Sync allows users to create data race to T: !Sync by using the AtomicBucket::data_with API to concurrently access &T from multiple threads.

Unconditional impl of Send allows users to send T: !Send to another thread by pushing T to AtomicBucket<T> and then moving it to another thread.

This issue exists in older versions too (v0.4.0-alpha.1),
but I used the newer version of the crate (v0.6.2) in the poc below,
as the older version fails to build on my pc at the moment..

Reproduction

Below is an example program that exhibits undefined behavior using safe APIs of metrics-util.

Show Detail

#![forbid(unsafe_code)]
use metrics_util::AtomicBucket;

use std::cell::Cell;
use std::sync::Arc;

#[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)]
enum RefOrInt {
    Ref(&'static u64),
    Int(u64),
}

// The bug triggered in this poc exists in older versions too (v0.4.0-alpha.1),
// but I used the newer version of the crate as the older version fails to build
// on my pc at the time..

static SOME_INT: u64 = 123;
fn main() {
    let cell = Cell::new(RefOrInt::Ref(&SOME_INT));
    let bucket = Arc::new(AtomicBucket::new());
    bucket.push(cell);

    let bucket_clone = bucket.clone();
    std::thread::spawn(move || {
        let bucket = bucket_clone;

        // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
        bucket.data_with(|arr| {
            for cell in arr.iter() {
                loop {
                    cell.set(RefOrInt::Ref(&SOME_INT));
                    cell.set(RefOrInt::Int(0xdeadbeef));
                }
            }
        });
    });

    bucket.data_with(|arr| {
        for cell in arr.iter() {
            loop {
                if let RefOrInt::Ref(addr) = cell.get() {
                    if addr as *const u64  == &SOME_INT as *const u64 {
                        continue;
                    }
                    println!("Pointer is now {:p}", addr);
                    println!("Dereferencing addr will now segfault: {}", *addr);
                }
            }
        }
    });
}

Output:

Pointer is now 0xdeadbeef

Terminated with signal 11 (SIGSEGV)

Tested Environment

• Crate: metrics-util
• Version: 0.6.2
• OS: Ubuntu 18.04.5 LTS
• Rustc version: rustc 1.50.0 (cb75ad5db 2021-02-10)

[tobz]

@JOE1994 Admittedly, I wasn't ever able to get the test case to hit the segfault. #191 should take care of this by constraining T, and indeed, the reproduction case doesn't even compile with the changes in place.

Really appreciate you filing this issue. Out of curiosity, what heuristic was used to discovered the code in this case? Just looking for unconstrained trait impls?

[JOE1994]

@tobz Thank you for your prompt feedback!

Out of curiosity, what heuristic was used to discovered the code in this case?

Yes, we look for unconstrained trait impls to discover such cases :)

[JOE1994]

May I ask if you could publish a minor release with the fix? Thank you :)

[tobz]

I think we can cut a minor release soon, yeah. There's a few big changes since the last releases so we just need to do a little bit of coordination.

I'll open another issue around cutting a release so I don't forget.

[tobz]

New releases including the fix have been cut: #201