Change API to be the same as Honggfuzz-rs and eventually AFL.rs

[Manishearth]

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

[fitzgen]

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

[fitzgen]

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.

[fitzgen]

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]

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]

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]

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]

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]

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]

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]

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]

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.