Skip to content
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 #51

Closed
wants to merge 1 commit into from

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 30, 2019

Redo of https://github.com/rust-fuzz/libfuzzer-sys/pull/33

I'm not merging this into master just yet, we should work on the next branch and land everything simultaneously so we can do a lockstep upgrade (https://github.com/rust-fuzz/libfuzzer-sys/issues/52).

r? @fitzgen

cc @PaulGrandperrin

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me modulo the comments below and the CI errors!

Looks like the example directory needs to be updated as well:

error: cannot find macro `fuzz_target` in this scope
 --> src/main.rs:4:5
  |
4 |     fuzz_target!(|data: &[u8]| {
  |     ^^^^^^^^^^^
warning: unused import: `libfuzzer_sys::fuzz`
 --> src/main.rs:1:5
  |
1 | use libfuzzer_sys::fuzz;
  |     ^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

#[allow(improper_ctypes)]
fn rust_fuzzer_test_input(input: &[u8]);
// 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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE(argc: *mut c_int, argv: *mut *mut *mut c_char, callback: extern fn(*const u8, usize) -> c_int );
#[linkage_name = "_ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE"]
fn fuzzer_driver(argc: *mut c_int, argv: *mut *mut *mut c_char, callback: extern fn(*const u8, usize) -> c_int );

and we miiiight also need to add an extra underscore for macos to the mangled name here with a cfg_attr or something.


unsafe {
// save closure capture at static location
STATIC_CLOSURE = Box::into_raw(Box::new(closure)) as *const ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check that STATIC_CLOSURE is null, or else this is unsafe if someone called the function multiple times (which could actually happen if called from from multiple threads). And it should probably be either an AtomicUsize or a thread local.

Or we could wrap this whole fuzz function's body in a std::sync::Once and call_once, which is probably also fine.

@alex
Copy link
Member

alex commented Dec 30, 2019

I feel like I missed the motivation for this, is it just consistency with hongfuzz? IMO this API seems worse: It's more complex for users (they need to define a main function in addition to the fuzz callback), and the internals seem to be more complex as well (the need to track the closure in the static var)

@fitzgen
Copy link
Member

fitzgen commented Dec 30, 2019

I believe the motivation is portability of fuzz targets across different fuzzers.

I'm not familiar with the constraints that led to hongfuzz's API so I don't really have an informed opinion on the topic. Maybe Manish does?

@Manishearth
Copy link
Member Author

No, I have no idea. Yeah, I'm not super happy about this either, which was why I held off on reviewing it the first time (and then kinda forgot). Other folks reviewed the PR so I decided to take the time and make it work.

I don't think it's that much more complex: in the old version you had to write no_main and a macro, here you have a main function and a macro inside it. The fact that it's easier to do setup/teardown is a big plus.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 30, 2019

One way to get around the static closure problem is to pass down a data pointer, we'll have to tweak libfuzzer to make it work

(If we're tweaking libfuzzer anyway it may be worth also adding the custom debug hook)

@Manishearth
Copy link
Member Author

Yeah, I really don't like the static closure thing. We have a couple options here:

  • Not do this change at all
  • Tweak the libfuzzer-sys API to allow for extra data to be passed down

Thoughts?

@fitzgen
Copy link
Member

fitzgen commented Dec 31, 2019

If it were up to me, I would probably punt on this API change, at least for now. Unless someone can come up with a stronger/clearer motivation.

+cc @frewsxcv

@frewsxcv
Copy link
Member

No strong feelings from about either design. Only motivation I'm aware of was to be consistent across the various fuzzers in the rust-fuzz ecosystem.

@fitzgen
Copy link
Member

fitzgen commented Jan 3, 2020

OK, I think we should close this PR since we can't seem to find super convincing motivation for it.

If we do find that motivation in the future, we can always reopen / reimplement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants