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

ShMem should probably require stored values to be Sync #2809

Open
langston-barrett opened this issue Jan 3, 2025 · 5 comments
Open

ShMem should probably require stored values to be Sync #2809

langston-barrett opened this issue Jan 3, 2025 · 5 comments
Labels
bug Something isn't working

Comments

@langston-barrett
Copy link
Contributor

ShMem can be used to create data-races if you use it to store non-Sync types such as Cell. For example, the assertion at the end of the following program fails:

use std::cell::Cell;

use libafl_bolts::os::{fork, ForkResult};
use libafl_bolts::shmem::{ShMem, ShMemProvider as _};

fn do_stuff(shmem: &impl ShMem) {
    let p = shmem.as_ptr_of::<Cell<usize>>().unwrap();
    for i in 0..128 {
        let val = unsafe { (*p).get() };
        println!("val = {val}");
        unsafe { (*p).set(val + 1) };
    }
}

fn main() {
    let mut shmem_provider = libafl_bolts::shmem::MmapShMemProvider::default();
    let shmem = shmem_provider.new_on_shmem(Cell::new(0usize)).unwrap();
    shmem_provider.pre_fork().unwrap();
    match unsafe { fork() }.unwrap() {
        ForkResult::Parent(handle) => {
            shmem_provider.post_fork(false).unwrap();
            do_stuff(&shmem);
            handle.status();
        }
        ForkResult::Child => {
            shmem_provider.post_fork(true).unwrap();
            do_stuff(&shmem);
            std::process::abort();
        }
    }
    let p = shmem.as_ptr_of::<Cell<usize>>().unwrap();
    assert_eq!(256, unsafe { (*p).get() });
}

Of course, you can also create data-races just by using as_mut_ptr... But fixing that would require a whole different design, whereas it would be pretty easy to add a Sync bound to ShMemProvider::new_on_shmem, ShMem::as_ptr_of, etc..

@langston-barrett langston-barrett added the bug Something isn't working label Jan 3, 2025
@domenukk
Copy link
Member

domenukk commented Jan 3, 2025

Feel free to try, but a lot other things may break(?)..

@langston-barrett
Copy link
Contributor Author

Really, the only way to actually prevent data-races would be to redesign ShMem to work more closely with fork, so that the child process gets a &mut T, the parent process waits for the child to exit, and then the parent gets the &mut T.

@domenukk
Copy link
Member

domenukk commented Jan 5, 2025

I don"t think that would help, rust doesn"t understand about fork at all...
Really, the moment you use shmem there is no way to be safe without some higher-level abstractions. That"s why we mark the funcitons unsafe in the first place..

@langston-barrett
Copy link
Contributor Author

Right, I was just trying to brainstorm such a higher-level abstraction.

That"s why we mark the funcitons unsafe in the first place..

Agreed, but I think it"s worth putting time and energy into exposing interfaces that are as easy to use correctly (and difficult to use incorrectly) as possible, even if they can"t ultimately be made safe. Adding Sync bounds would at least prevent some obvious mistakes.

@domenukk
Copy link
Member

domenukk commented Jan 7, 2025

Feel free to do so. I agree it feels more "more right" and def. cannot hurt

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

No branches or pull requests

2 participants