-
-
Notifications
You must be signed in to change notification settings - Fork 394
Use shmem for forkserver several pointers #3249
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
Changes from 8 commits
5759044
9bf7b06
a3753aa
f50a366
88c550f
f04e845
f99113b
e4be1fc
1a3fe98
cf1ceb5
259730f
6923664
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,20 @@ | ||
use libafl_bolts::shmem::{ShMemProvider, StdShMemProvider}; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Evian-Zhang probably we want to hard-code AshMemProvider here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I will improve this shmem after your work :) |
||
std::process::exit(1); | ||
}; | ||
|
||
// Map shared memory region for the edge coverage map | ||
if map_shared_memory().is_err() { | ||
if map_shared_memory(&mut shm_provider).is_err() { | ||
std::process::exit(1); | ||
} | ||
// Map shared memory region for input and its len | ||
if map_input_shared_memory().is_err() { | ||
if map_input_shared_memory(&mut shm_provider).is_err() { | ||
std::process::exit(1); | ||
}; | ||
// Start the forkserver | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,10 @@ | |
#[cfg(feature = "alloc")] | ||
use alloc::{rc::Rc, string::ToString, vec::Vec}; | ||
#[cfg(feature = "alloc")] | ||
use core::{cell::RefCell, fmt, fmt::Display, mem::ManuallyDrop}; | ||
use core::{cell::RefCell, fmt, fmt::Display}; | ||
use core::{ | ||
fmt::Debug, | ||
mem::size_of, | ||
mem::{ManuallyDrop, size_of}, | ||
ops::{Deref, DerefMut}, | ||
}; | ||
#[cfg(feature = "std")] | ||
|
@@ -247,6 +247,15 @@ pub trait ShMem: Sized + Debug + Clone + DerefMut<Target = [u8]> { | |
} | ||
} | ||
|
||
/// Consume current shared memory structure, and get the raw pointer to | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. |
||
let mut manually_dropped = ManuallyDrop::new(self); | ||
manually_dropped.as_mut_ptr().cast() | ||
} | ||
|
||
/// Get the description of the shared memory mapping | ||
fn description(&self) -> ShMemDescription { | ||
ShMemDescription { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a repo-wide justfile, we'll need it at some point anyway #3099
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes can be done later