RingBuffer allows sending Non-Send types across threads

[Qwaz]

Hello fellow Rustacean,
we (Rust group @sslab-gatech) are scanning Rust code on crates.io for potential memory safety and soundness bugs and found an issue in this crate which allows safe Rust code to exhibit an undefined behavior.

Issue Description

disrustor/src/ringbuffer.rs

Lines 48 to 49 in 966b949

	unsafe impl<T> Send for RingBuffer<T> {}
	unsafe impl<T> Sync for RingBuffer<T> {}

RingBuffer implements Send/Sync regardless of whether the internal type implements Send/Sync. This allows users to send a non-Send type across threads with RingBuffer.

RingBuffer's Send/Sync should probably have a trait bound on the internal type T, or if RingBuffer needs to implement Send/Sync and it is not meant to be created by the end user, new() should be hidden with pub(crate) or so.

Reproduction

Below is an example program that sends Non-Send type across threads using safe APIs of disrustor.

Show Detail

This example just shows that non-Send type is sent across the thread, but it is possible to demonstrate a data race with a more complicated example.

#![forbid(unsafe_code)]

#[macro_use]
extern crate static_assertions;

use disrustor::internal::RingBuffer;

use std::marker::PhantomData;
use std::thread;

struct NonSend {
    created_thread: thread::ThreadId,
    // Mark this struct as `NonSend`
    _marker: PhantomData<*mut ()>,
}

assert_not_impl_all!(NonSend: Send);

impl Default for NonSend {
    fn default() -> Self {
        NonSend {
            created_thread: thread::current().id(),
            _marker: PhantomData,
        }
    }
}

impl Drop for NonSend {
    fn drop(&mut self) {
        if thread::current().id() != self.created_thread {
            panic!("NonSend destructor is running on a wrong thread!");
        }
    }
}

fn main() {
    let buffer = RingBuffer::<NonSend>::new(1);

    let handle = thread::spawn(move || {
        drop(buffer);
    });

    handle.join().unwrap();
}

Output:

thread '<unnamed>' panicked at 'NonSend destructor is running on a wrong thread!', src/main.rs:47:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any', src/main.rs:59:19

Return code 101

Tested Environment

• Crate: disrustor
• Version: 0.2.0
• OS: Ubuntu 20.04.1 LTS
• Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)
• 3rd party dependencies:
  • static_assertions = { version = "1.1.0" }

[Qwaz]

disrustor/src/producer.rs

Lines 108 to 113 in 966b949

	let (start, end) = self.sequencer.next(iter.len());
	for (idx, item) in iter.enumerate() {
	let seq = start + idx as Sequence;
	let slot = unsafe { self.data_provider.get_mut(seq) };
	f(slot, seq, &item);
	}

Another data race issue has been found in RingBuffer's DataProvider implementation. It seems that, combined with Producer's write(), RingBuffer allows creating multiple mutable references to the same data region.

#![forbid(unsafe_code)]

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

use disrustor::internal::RingBuffer;
use disrustor::DisrustorBuilder;
use disrustor::EventProducer;

// A simple tagged union used to demonstrate problems with data races in Cell.
#[derive(Clone, Copy)]
enum RefOrInt {
    Ref(&'static u64),
    Int(u64),
}

static STATIC_INT: u64 = 123;

impl Default for RefOrInt {
    fn default() -> Self {
        RefOrInt::Ref(&STATIC_INT)
    }
}

fn main() {
    let provider = Arc::new(RingBuffer::<Cell<RefOrInt>>::new(1));
    let provider_cloned = provider.clone();

    thread::spawn(move || {
        let (_executor, producer) = DisrustorBuilder::new(provider_cloned)
            .with_spin_wait()
            .with_single_producer()
            .with_barrier(|_| {})
            .build();

        producer.write(std::iter::once(()), |slot, _seq, _item| loop {
            // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
            *slot.get_mut() = RefOrInt::Ref(&STATIC_INT);
            *slot.get_mut() = RefOrInt::Int(0xdeadbeef);
        });
    });

    let (_executor, producer) = DisrustorBuilder::new(provider.clone())
        .with_spin_wait()
        .with_single_producer()
        .with_barrier(|_| {})
        .build();

    producer.write(std::iter::once(()), |slot, _seq, _item| {
        loop {
            if let RefOrInt::Ref(addr) = slot.get() {
                // Hope that between the time we pattern match the object as a
                // `Ref`, it gets written to by the other thread.
                if addr as *const u64 == &STATIC_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)

[Shnatsel]

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

[sklose]

Hi everybody,

apologies for me getting back to you so late. Really appreciate the effort of finding this issue. For the short term I will try to see if I can unpublish the package from crates.io and put a warning in the README file. It will probably take me a while before I find some time to finish the long overdue refactoring of the API to make it sound.

-Sebastian

[sklose]

This has finally been fixed in version 0.3. The examples have been added as trybuild test cases here: https://github.com/sklose/disrustor/tree/master/tests/cve