rust-embedded / critical-section

Pluggable critical section
Apache License 2.0
104 stars 17 forks source link

`CriticalSection` should not be `Send` or `Sync` #55

Open matanui159 opened 1 day ago

matanui159 commented 1 day ago

Allowing the CriticalSection to be both Send and Sync allows multiple threads to gain access to a resource by sharing it between threads. For example with std this could be done without any unsafe code:

use std::{cell::Cell, thread};
use critical_section::{CriticalSection, Mutex};

static LIST: Mutex<Cell<Vec<i32>>> = Mutex::new(Cell::new(Vec::new()));

fn push_to_list(cs: CriticalSection, iter: impl IntoIterator<Item = i32>) {
    for item in iter {
        let mut list = LIST.borrow(cs).take();
        list.push(item);
        LIST.borrow(cs).set(list);
        thread::yield_now();
    }
}

fn main() {
    critical_section::with(|cs| thread::scope(|scope| {
        scope.spawn(|| {
            push_to_list(cs, 0..100);
        });
        push_to_list(cs, 100..200);
    }));

    critical_section::with(|cs| {
        println!("{:?}", LIST.borrow(cs).take());
    });
}

This causes undefined behaviour and often crashes:

csbug(12007,0x16b3cb000) malloc: *** error for object 0x11f606290: pointer being realloc'd was not allocated
csbug(12007,0x16b3cb000) malloc: *** set a breakpoint in malloc_error_break to debug
fish: Job 1, 'cargo run' terminated by signal SIGABRT (Abort)
jamesmunns commented 19 hours ago

This was discussed briefly in chat, it seems like we should ensure that the CS tokens are !Send and !Sync.

This could likely be done in a 1.2 release (and yank older versions), as it is a fix for soundness, and any uses like this would be unsound.