-
Notifications
You must be signed in to change notification settings - Fork 47
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
Change API to be the same as Honggfuzz-rs and eventually AFL.rs #33
base: main
Are you sure you want to change the base?
Conversation
The main drawback I see is that this is using a non-public API of libFuzzer and even worse, a mangled C++ non-public API... From a pragmatic point of view, it is not a huge risk because any time the libfuzzer code will change, we will have to manually import it and so it'll be easy to do adjustments. However, I think we should contact the libfuzzer team to ask them to stabilize and export this API in a C format. |
Very cool! I don't think this closes rust-fuzz/cargo-fuzz#119 though, see my comment there. |
Using internals is fine, given that we bundle libfuzzer within this project anyway. Though, what should be done is adding a separate .cpp file to the vendored libfuzzer copy that exposes a proper I’m not yet sold on this sort of API sharing, but it is definitely better than nothing. And we can definitely work on making fuzzers swappable through |
@nagisa yeah reexporting the function via a cpp wrapper is the way to go, I was just too lazy to do it for this proof of concept. I think the main takeaway from this PR and the other PR on AFL.rs is that we can pretty much abstract whatever API we want to the user, without technical limitations. Now is probably a good time to debate about which API we want to go forward with and implement it in all 3 fuzzers. |
This looks pretty nice. One of my plans for libfuzzer was to patch the cpp code so that instead of the weird linkage dance, you just call |
What's the status with this? Is there any chance of this getting merged? |
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.
lgtm! i don't see why not!
sorry for the overdue review, better late than never lol
cc @fitzgen we should make sure this fits in with our plans as well |
Seems fine to me. |
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.
Are we okay with the breakage this will cause? cargo fuzz does not pin libfuzzer-sys versions, so anyone who cargo fuzz init
s will get a newer libfuzzer and older targets.
Maybe we should try to publish this crate? And pair that with a change so that cargo fuzz pins to a major version.
fn rust_fuzzer_test_input(input: &[u8]); | ||
extern "C" { | ||
// This is the mangled name of the C++ function starting the fuzzer | ||
fn _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE(argc: *mut c_int, argv: *mut *mut *mut c_char, callback: extern fn(*const u8, usize) -> c_int ); |
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.
Is this the same on all platforms? Should we be exposing an extern c function instead?
|
||
unsafe { | ||
// save closure at static location and forget its lifetime | ||
STATIC_CLOSURE = Some(std::mem::transmute(closure as &mut FnMut(&[u8]))); |
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 unnecessary, it can be done statically by wrapping it with a function
+1 I was also thinking about this in the context of using the soon-to-be-released breaking update for |
@@ -7,7 +7,7 @@ extern "C" { | |||
fn _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE(argc: *mut c_int, argv: *mut *mut *mut c_char, callback: extern fn(*const u8, usize) -> c_int ); | |||
} | |||
|
|||
static mut STATIC_CLOSURE: Option<Box<FnMut(&[u8])>> = None; | |||
static mut STATIC_CLOSURE: Option<&mut FnMut(&[u8])> = None; |
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 isn't safe at all. We should not be using FnMut.
There are a couple things that need fixing, and it needs a rebase as well, I'm going to open a new PR. |
There is a lot of black magic here and this PR is not really intented to be merged directly, principally because it break compatibility.
I understand this code feels very wacky but it leads to a very nice user API:
#![no_main]
But really, the very big main advantage is that after this, all fuzzers, libfuzzer-sys, honggfuzz-rs and AFL.rs (after rust-fuzz/afl.rs#137) will be able to share the exact same API!
This will allow to simplify a lot https://github.com/rust-fuzz/targets and easily make
cargo-fuzz
compatible with all fuzzers.closes rust-fuzz/cargo-fuzz#119
closes rust-fuzz/cargo-fuzz#101