diwic / reffers-rs

Rust wrappers around references, boxes and Arcs.
68 stars 3 forks source link

Unsound `Send` implementation of `RMBA`, and the `Send`/`Sync` implementations of `arc::*` #9

Closed steffahn closed 2 years ago

steffahn commented 2 years ago

It implements Send for RMBA<'_, T> when T: Send, even though an RMBA could actually contain a &T, where T: !Sync.

Example exploitation:

use reffers::RMBA;

use std::{
    cell::Cell,
    thread,
};

fn main() {
    let x = &*Box::leak(Box::new(Cell::new(Ok::<_, &[()]>(&[1_u8, 2, 3, 4, 5][..]))));
    let r = RMBA::new(x); // r contains a reference to a Cell
    // r can be sent to another thread!
    thread::spawn(|| {
        let r = r;

    // remainder of code for exploiting a data race...

        let long_array = &[(); 10_000_000];
        loop {
            r.set(Err(long_array));
            r.set(Ok(&[1, 2, 3, 4, 5]));
        }
    });
    loop {
        if let Ok(s) = x.get() {
            if s.len() > 1000 {
                // prints lots of data it isn't supposed to access
                // (starting with 1, 2, 3, 4, 5), then segfaults
                println!("{s:?}");
            }
        }
    }
}

To fix this, the Send implementation should probably require T: Send + Sync.

steffahn commented 2 years ago

Similar issues with the Send implementation and Sync implementation of arc::Ref, arc::RefMut, arc::Strong, and arc::Weak. Compare the bounds in the standard library: https://doc.rust-lang.org/std/sync/struct.Arc.html#impl-Send

Example exploitation of the Ref: Send case… ```rs use reffers::arc::Ref; use std::{ cell::Cell, thread, }; fn main() { let x = Ref::<_, usize>::new(Cell::new(Ok::<_, &[()]>(&[1_u8, 2, 3, 4, 5][..]))); let r = x.clone(); // r can be sent to another thread! thread::spawn(|| { let r = r; // remainder of code for exploiting a data race... let long_array = &[(); 10_000_000]; loop { r.set(Err(long_array)); r.set(Ok(&[1, 2, 3, 4, 5])); } }); loop { if let Ok(s) = (*x).get() { if s.len() > 1000 { // prints lots of data it isn't supposed to access // (starting with 1, 2, 3, 4, 5), then segfaults println!("{s:?}"); } } } } ```
diwic commented 2 years ago

Hi @steffahn!

Thanks for scrutinizing the library. It was a long time since I last touched it...

I believed I have fixed the unsoundness you reported. Could you review the patches to see if they are correct before I release a new version? Thanks!

steffahn commented 2 years ago

Looks good upon first inspection. I’ll have to inspect the role of M in the arc to fully understand its role, but M: Send+Sync is not going to be unsound, at most too conservative. So looks good to me, I guess.