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

Singleton needs bounds on its Send/Sync traits #10

Closed
ammaraskar opened this issue Nov 16, 2020 · 1 comment · Fixed by #11
Closed

Singleton needs bounds on its Send/Sync traits #10

ammaraskar opened this issue Nov 16, 2020 · 1 comment · Fixed by #11
Assignees
Labels
bug Something isn't working

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that Singleton implements Send and Sync unconiditionally:

// The Singleton need to implement Send & Sync to ensure cross core compile check mechanics
// this is safe as the inner RWLock ensures cross core safety
unsafe impl<T> Sync for Singleton<T> {}
unsafe impl<T> Send for Singleton<T> {}

This is guarded by a RwLock but violates RwLock's own assumptions that Send will have T: Send and Sync will be T: Send + Sync. The way this is structured right now allows for data races from safe Rust code, for example:

#![forbid(unsafe_code)]

use ruspiro_singleton::Singleton;

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

#[derive(Debug, Clone, Copy)]
enum RefOrInt<'a> { Ref(&'a u64), Int(u64) }
static SOME_INT: u64 = 123;

static STATIC_CELL : Singleton<Cell<RefOrInt>> = Singleton::lazy(&|| {
    Cell::new(RefOrInt::Ref(&SOME_INT))
});

fn main() {
    thread::spawn(move || {
        loop {
            STATIC_CELL.with_ref(|cell| {
                // Repeatedly write Ref(&addr) and Int(0xdeadbeef) into the cell.
                cell.set(RefOrInt::Ref(&SOME_INT));
                cell.set(RefOrInt::Int(0xdeadbeef));
            });
        }
    });

    STATIC_CELL.with_ref(|cell| {
        loop {
            if let RefOrInt::Ref(addr) = cell.get() {
                // Hope that between the time we pattern match the object as a
                // `Ref`, it gets written to by the other thread.
                if addr as *const u64 == &SOME_INT as *const u64 { continue; }
    
                // Due to the data race, obtaining Ref(0xdeadbeef) is possible
                println!("Pointer is now: {:p}", addr);
                println!("Dereferencing addr will now segfault: {}", *addr);
            }
        }
    });
}

This outputs:

Pointer is now: 0xdeadbeef

Return Code: -11 (SIGSEGV)
@2ndTaleStudio 2ndTaleStudio linked a pull request Nov 16, 2020 that will close this issue
@2ndTaleStudio 2ndTaleStudio added the bug Something isn't working label Nov 16, 2020
@2ndTaleStudio 2ndTaleStudio self-assigned this Nov 16, 2020
@2ndTaleStudio
Copy link
Collaborator

Thanks for your investigation and support. I actually did not considered the case someone would use types with interior mutability as types wrapped with a Singleton.

So I'll fix it as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants