Use shmem for forkserver several pointers #3249
Conversation
|
Consider merging your branch with #3233 as well? So that we can confirm the issue is fixed. |
|
@wtdcode Sure. Merged. |
|
Hmmmm, I don't think the failed CI is related to my change. Anyway, the android CI has successfully passed |
|
It is #3248 |
|
Done, you could update your branch =). |
| use libafl_targets::{ | ||
| map_input_shared_memory, map_shared_memory, start_forkserver, MaybePersistentForkserverParent, | ||
| }; | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn libafl_start_forkserver() { | ||
| let Ok(mut shm_provider) = StdShMemProvider::new() else { |
There was a problem hiding this comment.
On Android StdShMemProvider spawns a server, we need to double check if this works for this use case (otherwise we should set a different provider here)
There was a problem hiding this comment.
I totally agree. But I know little about Android, and I also don't know why the standard shmem provider needs a service wrapper. Maybe this should be checked by someone knowing this detail.
There was a problem hiding this comment.
We need a service on android to hand out ashmem handles via unix sockets, as that is the only way to get a 'reference' to an existing shared memory allocation.
There was a problem hiding this comment.
This is for multi process IPC. For a forkserver we don't need that since the child can just inherit the allocations on fork
There was a problem hiding this comment.
@Evian-Zhang probably we want to hard-code AshMemProvider here
There was a problem hiding this comment.
Hmmm, is it possible for someone to setup a android fuzzer test CI so that we can test the correctness? I personally don't know any of android things, so although I'm willing to hard code it, I'm afraid we cannot ensure its correctness.
There was a problem hiding this comment.
I will do that shortly these days. Will ping you if there is some failures.
Considering the slow performance of QEMU emulation, maybe we can just test simple fuzzers.
There was a problem hiding this comment.
Thank you! I will improve this shmem after your work :)
libafl_bolts/src/shmem.rs
Outdated
| /// this shared memory. | ||
| /// | ||
| /// Note that calling this method will result in a memory leak. | ||
| fn into_raw<T: Sized>(self) -> *mut T { |
There was a problem hiding this comment.
I don't 100% get the need for this function, why can't we keep a reference to the shmem around in general?
There was a problem hiding this comment.
The shms are globally owned and we will need lots of global OnceCell to keep them. Note we can’t change EDGES_MAP or so to OnceCell because of symbol names. Therefore, I guess the intention is to avoid such duplicate cells? I’m okay with either way.
There was a problem hiding this comment.
Yes, just as @wtdcode said, the whole forksever logic relies on those global raw pointers, if we maintain those raw pointers and the shmem at the same time, we are creating potentially two mutable references to the same memory, which I think the memory leak is a better choice. And the original version of forksever also leaks the memory.
There was a problem hiding this comment.
Let's keep this as a (non-public) function inside the forkserver then, it shouldn't be something most people/not part of the ShMem trait, IMHO.
|
Nice! Left some comments |
| env: | ||
| ANDROID_NDK_HOME: ${{ steps.setup-ndk.outputs.ndk-path }} | ||
| ANDROID_NDK_ROOT: ${{ steps.setup-ndk.outputs.ndk-path }} | ||
| run: cargo ndk -t x86_64 build |
There was a problem hiding this comment.
Can you add this to a justfile? We want to have every CI task in a justfile in the future
There was a problem hiding this comment.
Sorry I don't really know about the detail. @wtdcode Can you do this in your branch? I could merge such changes.
There was a problem hiding this comment.
@domenukk It is not a new fuzzer or test but just doing cross-building. I don't see a directory to place such justfiles. ./just seems common library scripts.
There was a problem hiding this comment.
Create a repo-wide justfile, we'll need it at some point anyway #3099
Description
This will supersede #3229
Checklist
./scripts/precommit.shand addressed all comments