ShmWriter allows sending non-Send type across threads

[Qwaz]

Hello fellow Rustacean,
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

kekbit/src/core/writer.rs

Line 82 in 00fc665

unsafe impl<H: Handler> Send for ShmWriter<H> {}

ShmWriter implements Send trait regardless of the inner type parameter H. This definition allows safe Rust code to send non-Send type across threads, which potentially causes a data race or undefined behavior.

H: Send trait bound should probably be added to ShmWriter's Send implementation. If all handlers are expected to be Send, then Send bound can be added to Handler trait's definition instead.

Reproduction

Below is an example program that shows non-Send type can be sent across threads using safe APIs of kekbit.

Show Detail

#![forbid(unsafe_code)]

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

use kekbit::api::Handler;
use kekbit::core::{shm_writer, Metadata, TickUnit::Nanos};

// non-Send type that panics when dropped in a wrong thread
struct NonSend {
    created_thread: thread::ThreadId,
    // Ensure `NonSend` type does not implement `Send` trait
    _marker: PhantomData<*mut ()>,
}

impl NonSend {
    pub fn new() -> 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!");
        }
    }
}

impl Handler for NonSend {}

fn main() {
    // Example code from: https://docs.rs/kekbit/0.3.3/kekbit/core/fn.shm_writer.html#examples
    const FOREVER: u64 = 99_999_999_999;
    let writer_id = 1850;
    let channel_id = 42;
    let capacity = 3000;
    let max_msg_len = 100;
    let metadata = Metadata::new(writer_id, channel_id, capacity, max_msg_len, FOREVER, Nanos);
    let test_tmp_dir = tempdir::TempDir::new("kekbit").unwrap();

    let writer = shm_writer(&test_tmp_dir.path(), &metadata, NonSend::new()).unwrap();

    let handle = thread::spawn(move || {
        // `NonSend` is sent to another thread via `ShmWriter`
        drop(writer);
    });

    handle.join().unwrap();
}

Output:

thread '<unnamed>' panicked at 'NonSend destructor is running on a wrong thread!', src/main.rs:44: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:68:19

Return code 101

Tested Environment

• Crate: kekbit
• Version: 0.3.3
• OS: Ubuntu 20.04.1 LTS
• Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)
• 3rd party dependencies:
  • tempdir = { version = "0.3.7" }

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

[motoras]

Issue fixed.