-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
Tree-shaped multi-machine fuzzing #2302
Conversation
inputs::Input, | ||
}; | ||
|
||
/// The Receiving side of the multi-machine architecture |
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.
sending side?
SP: ShMemProvider, | ||
I: Input + Send + Sync + 'static, | ||
{ | ||
/// check for received messages, and forward them alongside the incoming message to inner. |
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.
maybe update comment?
if self.spawn_broker { | ||
log::info!("I am broker!!."); | ||
|
||
// TODO we don't want always a broker here, think about using different laucher process to spawn different configurations |
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.
this is much cleaner now 😀
let serialized_msg = bitcode::serialize(msg)?; | ||
let msg_len = u32::to_le_bytes(serialized_msg.len() as u32); | ||
|
||
// 0. Write the dummy byte |
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.
why this is needed? to write dummy bytes?
|
||
parent_lock.parent = loop { | ||
info!("Trying to connect to parent @ {}..", parent_addr); | ||
match TcpStream::connect(parent_addr).await { |
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.
shall we use set_keepalive() on this connection?
thread safety stuff
mv to sqlite less warnings
|
||
// Here, we suppose msg will never be written again and will always be available. | ||
// Thus, it is safe to handle this in a separate thread. | ||
let msg_lock = unsafe { NullLock::new((msg.as_ptr(), msg.len())) }; |
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.
I feel like this is wrong on a rust level, the mutable borrow only lives as long as on_new_message
, right? And here you spawn an async function that runs after the function returns, using the msg
.
at least the TcpMultiMachineLlmpReceiverHook
new function should be unsafe
, and this side effect ("on_new_message needs msg to have a quasi-static lifetime) should be documented there
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.
Please fix
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.
the msg_lock will leave beyond the call to on_new_message. it's valid with 2 hypothesis:
- messages, once sent, are never written again
- once a message is posted, it is never deallocated.
this patch is very experimental in general, we merged for practical reasons (it's part of the reason why it's hidden behind an undocumented feature for now)
I'll open the follow-up patch
|
||
if self.always_interesting { | ||
let item = fuzzer.add_input(state, executor, self, input)?; | ||
log::info!("Added received Testcase as item #{item}"); | ||
debug!("Added received Testcase as item #{item}"); |
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.
Why debug? This feels pretty confusing, I'd prefer to prepend log
(There is the non-log debug macro already)
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.
i tried to keep the existing as log and the new one as debug to avoid polluting the existing logs, otherwise it becomes very verbous. this one is my mistake, i forgot to revert it to info
@@ -13,6 +13,7 @@ use libafl_bolts::{ | |||
shmem::{NopShMemProvider, ShMemProvider}, | |||
ClientId, | |||
}; | |||
use log::debug; |
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.
This shadows the debug
macro, which is pretty handy
@@ -1,1369 +0,0 @@ | |||
//! TCP-backed event manager for scalable multi-processed fuzzing |
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.
Why remove this? It's good to have
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.
not sure, i'll ask to toka
@@ -32,6 +32,7 @@ Welcome to `LibAFL` | |||
clippy::similar_names, | |||
clippy::too_many_lines, | |||
clippy::into_iter_without_iter, // broken | |||
clippy::type_complexity, |
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.
No
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.
@tokatoka why allow this?
Why does this remove the tcp event manager? |
@@ -18,7 +18,7 @@ debug = true | |||
|
|||
[build-dependencies] | |||
cc = { version = "1.0", features = ["parallel"] } | |||
which = "4.4" | |||
which = "6.0" | |||
|
|||
[dependencies] | |||
libafl = { path = "../../libafl/", features = ["default", "tcp_manager"] } |
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.
How does that (still) work?
* tree-shaped multi-machine fuzzing * forgot main file * aaa * moving things around * fix * working? * remove debug panic * aaa * aaa * fmt * normal centralized adapted * removed old useless code * cleanup * llmp hooks * working multi machine apparently? * aaa * cleanup (AFLplusplus#2305) * added old message dispatch. thread safety stuff * testing things around * opti opti opti * :) * fuzz * limit the amound received at once to avoid congestion * remove useless corpus mv to sqlite less warnings * aaa * ; * big opti * adding cfgs * fix * fixer * fix * s * clippy and reduce generics * debugging * fix * more robust disconnection * aaa * aaa * aaa * nostd * more nostd * clippy * not in ci * unused * aaa * doc * clippy * clippy * clippy * no crash in libpng * aaa * aaa * aaa * aaa * graph generator * fix * fix * windows fix all --------- Co-authored-by: Dongjia "toka" Zhang <tokazerkje@outlook.com>
* Address comments from AFLplusplus#2302 * secure? * cleanup * early exit ftw * address clippy * Fix all the things
another attempt to get multi-machine fuzzing working, in addition to #66 and #1302.
this one aims to act at broker level to be resilient against restarts.
draft state for now, the code is messy.
it does some refactoring here and there to have things working nicely.