ShMem should probably not give out references

[langston-barrett]

ShMem::as_object{,_mut} returns a (mutable) reference. This lets the compiler make strong assumptions about ownership that do not apply. For example, consider the following program:

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

#[no_mangle]
pub fn setup() {
    let mut prov = libafl_bolts::shmem::StdShMemProvider::default();
    let mut shmem1 = unsafe { prov.new_shmem_object::<u32>().unwrap_unchecked() };
    let mut shmem2 = unsafe { prov.clone_ref(&shmem1).unwrap_unchecked() };
    let r1 = unsafe { shmem1.as_object_mut::<u32>() };
    let r2 = unsafe { shmem2.as_object_mut::<u32>() };
    go(r1, r2);
}

#[no_mangle]
pub fn go(r1: &mut u32, r2: &mut u32) {
    mut_r1(r1);
    mut_r2(r2);
    if *r1 == 1 {
        println!("r1 = {r1}");
    }
}

#[no_mangle]
pub fn mut_r1(r1: &mut u32) {
    *r1 = 0;
    *r1 = 1;
}

#[no_mangle]
pub fn mut_r2(r2: &mut u32) {
    *r2 = 32;
}

First, let's look at mut_r1:

cargo asm --lib --release mut_r1

.section .text.mut_r1,"ax",@progbits
	.globl	mut_r1
	.p2align	4, 0x90
	.type	mut_r1,@function
mut_r1:
	.cfi_startproc
	mov dword ptr [rdi], 1
	ret

The write of 0 has disappeared, because Rust assumes that no other thread/process can read the reference &mut r1.

That example is a bit artificial, but the behavior of this program is:

cargo run -qr

r1 = 32

This is very counter-intuitive, and probably happens because Rust/LLVM decide to elide the load before the if and/or reorder it with respect to the mutation in mut_r2, because it can tell that the condition "is always true"..

Now, you might say "but you used unsafe, of course everything went wrong"! The problem is that I believe that this usage of the API corresponds with how it's intended to be used: the fuzzer reads from it, while the fuzzee writes to it. And the documentation for as_object{,_mut} don't mention anything about aliasing, it only discusses initialization.

The solution is probably to deprecate as_object{,_mut} in favor of an API that uses raw pointers, and encourages reading/writing them with ptr::{read,write}_volatile.

[domenukk]

Yes, this should probably return pointers instead, and potentially be renamed to something like as_ptr_of.
In any case I don't even think this needs to be part of ShMem but it might be extra trait or some helper function...

[domenukk]

Do you want to fix this? The Bt observer will have to keep a reference as well, and only create a borrow when it's used

[langston-barrett]

Do you want to fix this?

I think there are a two "levels" at which this could be fixed:

• Remove the methods from ShMem, but adapt all the callsites in the example fuzzers to cast the raw pointer to a mutable reference. Requires minimal code change, and fixes the problem for other library consumers, but still has UB in the example fuzzers.
• Come up with a comprehensive fix to all consumers of these references, e.g., AFLppCmpLogObserver. This would be quite difficult, as a data-race-proof fix would require some kind of IPC locking. Even a racy optimization-proof fix would require dealing with raw pointers, probably through ptr::{write,read}_volatile. Using these APIs properly would be quite difficult (for example, read_volatile creates a bitwise copy even for non-Copy types, and AFLppCmpLogMap is indeed non-Copy).

I can definitely do the former, but don't feel equipped to do the latter.

[domenukk]

We don't need data-race-proof (since we use it single threaded), we need rust UB proof. For this, keeping a reference to the raw pointer in all users is enough, and casting it to a reference wherever we use it (for a short time)

My understanding is: we need to return a pointer here, then fix the consumers to store pointers instead of refs.

[langston-barrett]

We don't need data-race-proof (since we use it single threaded)

I thought the point of ShMem was to use it for IPC (i.e., forking fuzzers)? So the fuzzer could conceivably be reading while the fuzzee is writing, leading to a data-race?

For this, keeping a reference to the raw pointer in all users is enough, and casting it to a reference wherever we use it (for a short time)

Yeah, I think this would be fine so long as we know that all of the accesses happen after the fuzzee completely terminates, so that the memory is not getting mutated while it's being read (and we have to assume that it never corrupts the shared memory, but that seems relatively safe, and it would be infeasible to do better).

[domenukk]

If it's used for IPC people have to build protocols on top, yes.
I'll throw a PR together give me sec

[domenukk]

@langston-barrett can you take a look at my solution in #1751 - does that look better?

[domenukk]

Fixed in #1751