diwic / reffers-rs

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

Unsound: can make `ARefss` contain a !Send, !Sync object. #7

Closed JOE1994 closed 3 years ago

JOE1994 commented 3 years ago

Hello 🦀 , while scanning crates.io, we (Rust group @sslab-gatech) have noticed a soundness/memory safety issue in this crate which allows safe Rust code to trigger undefined behavior.

Issue

It is possible to make ARefss contain a non-Send / non-Sync object, since there is no Send + Sync bound on V in the ARefss::map() function.

pub fn map<V: ?Sized, F: FnOnce(&U) -> &V>(self, f: F) -> ARefss<'a, V>

Proof of Concept

I wrote a short program that can trigger undefined behavior in safe Rust using this crate.

Test environment

Error message from the program

error: process didn't exit successfully: `target\release\examples\reffers.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
Segmentation fault
use std::cell::Cell;
use std::sync::Arc;

use reffers::aref::ARefss;

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

static X: u64 = 0;
fn main() {
    let arc_0 = Arc::new(ARefss::new(Arc::new(0)).map(|_| {
        // New item is totally unrelated to the previously stored item.
        // New item is allowed to be !Sync, !Send.
        Box::leak(Box::new(Cell::new(RefOrInt::Ref(&X))))
        // Box::leak(Box::new(std::rc::Rc::new(0)))
    }));
    let arc_child = Arc::clone(&arc_0);

    std::thread::spawn(move || {
        let arc_child = arc_child;

        let smuggled_cell = arc_child.as_ref();
        loop {
            smuggled_cell.set(RefOrInt::Int(0xdeadbeef));
            smuggled_cell.set(RefOrInt::Ref(&X));
        }
    });

    loop {
        if let RefOrInt::Ref(addr) = arc_0.get() {
            if addr as *const _ as usize == 0xdeadbeef {
                // Due to the data race, obtaining Ref(0xdeadbeef) is possible
                println!("Pointer is now: {:p}", addr);
                println!("Dereferencing addr will now segfault: {}", *addr);
            }
        }
    }
}

Thank you for checking out this issue 🦀

diwic commented 3 years ago

Cool! Thanks for discovering. I guess it's a first for everyone when someone else discovers an unsoundness issue in your own code ;-)

I believe the commit referenced above resolves the issue. Would you mind reviewing it and reopen it in case you disagree? Thanks!

JOE1994 commented 3 years ago

The fix looks good to me :+1: Thank you for your feedback!