search

rust-fuzz / libfuzzer

Rust bindings and utilities for LLVM’s libFuzzer
Apache License 2.0
208 stars 44 forks source link

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

Closed Manishearth closed 4 years ago

Manishearth commented 4 years ago

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

alex commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Thoughts?

fitzgen commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.