Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unsound2 and fix for Send + 'static as found by SSLab-gatech #49

Merged
merged 2 commits into from
Nov 17, 2020

Conversation

Firstyear
Copy link
Member

Due to a missing Send + 'static bound, The map types were able to have incorrect data inserted - this previously affected EBRCell.

@JOE1994
Copy link

JOE1994 commented Nov 16, 2020

@Firstyear Thank you for your feedback in #48 !

On top of the example mentioned in #48 , here is another program that causes a segfault in debug mode,
even when tested with this branch. In the program below, enum-checking is bypassed by exploiting the data race on Cell.
Parent thread dereferences 0xDEADBEEF thinking it's a valid reference, resulting in a segmentation fault (core dumped).

Adding a Sync bound to V of impl Sync for ARCache<K, V> in this branch lets rustc refuse the example program
(crate builds successfully with the update). It'd be nice to add a Sync bound to V of impl Sync ARCache<K, V> to rule out potential soundness issues 🦀

use concread::arcache::ARCache;

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

#[derive(Debug, Clone, Copy)]
enum RefOrInt<'a> {
    Ref(&'a u64),
    Int(u64),
}

fn main() {
    static PARENT_STATIC: u64 = 1;

    // `Cell` is `Send` but not `Sync`.
    let item_not_sync = Cell::new(RefOrInt::Ref(&PARENT_STATIC));

    let cache = ARCache::<i32, Cell<RefOrInt>>::new_size(5, 5);
    let mut writer = cache.write();
    writer.insert(0, item_not_sync);
    writer.commit();

    let arc_parent = Arc::new(cache);
    let arc_child = arc_parent.clone();

    std::thread::spawn(move || {
        let arc_child = arc_child;
        // new `Reader` of `ARCache`
        let reader = arc_child.read();
        let ref_to_smuggled_cell = reader.get(&0).unwrap();

        static CHILD_STATIC: u64 = 1;
        loop {
            ref_to_smuggled_cell.set(RefOrInt::Ref(&CHILD_STATIC));
            ref_to_smuggled_cell.set(RefOrInt::Int(0xDEADBEEF));
        }
    });

    let reader = arc_parent.read();
    let ref_to_inner_cell = reader.get(&0).unwrap();
    loop {
        if let RefOrInt::Ref(addr) = ref_to_inner_cell.get() {
            if addr as *const _ as usize == 0xDEADBEEF {
                println!("We have bypassed enum checking");
                println!("Dereferencing `addr` will now segfault : {}", *addr);
            }
        }
    }
}

@Firstyear
Copy link
Member Author

I think you are correct. Overnight while thinking about the problem I realised that Sync is required too, so I'm glad you supplied a reproducer that lines up with what I was thinking. I think this actually affects all the data-structures in this library, so I'll add the Send + Sync bound to them all.

@Firstyear
Copy link
Member Author

Thank you again @JOE1994, this adds Sync + Send + 'static to the bounds of all datastructures, which prevents unsound2 and unsound3 from being able to be compiled. You can check this with cargo build --features unsoundness.

I really appreciate your attention to detail and assistance! Would you mind if I add you to a contributors file?

@JOE1994
Copy link

JOE1994 commented Nov 17, 2020

I really appreciate your attention to detail and assistance! Would you mind if I add you to a contributors file?

Of course! I'd be totally grateful 🙇‍♂️

@Firstyear
Copy link
Member Author

Great done. All the tests pass, and both unsound examples fail, so I will merge and close this. Again, thank you very much for your input and contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants