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

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

Closed
JOE1994 opened this issue Dec 1, 2020 · 2 comments
Closed

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

JOE1994 opened this issue Dec 1, 2020 · 2 comments

Comments

@JOE1994
Copy link

JOE1994 commented Dec 1, 2020

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

  • OS: Windows
  • crate version : reffers-0.6.0
  • compiled with rustc 1.47.0 (18bf6b4f0 2020-10-07)
  • program exhibits a segmentation fault when compiled and run in release mode.

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 diwic closed this as completed in 6dd7ca0 Dec 1, 2020
@diwic
Copy link
Owner

diwic commented Dec 1, 2020

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
Copy link
Author

JOE1994 commented Dec 1, 2020

The fix looks good to me 👍 Thank you for your feedback!

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

No branches or pull requests

2 participants