This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Diff 7 - Improve Signal Handler interface.
ClosedPublic

Authored by mpividori on Nov 29 2016, 4:24 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 79671.Nov 29 2016, 4:24 PM
mpividori retitled this revision from to [libFuzzer] Diff 7 - Improve Signal Handler interface..
mpividori updated this object.
mpividori added reviewers: kcc, zturner.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.

looks good to me, but +rnk since he's the Level 99 exception handling warrior.

rnk accepted this revision.Nov 30 2016, 12:46 PM
rnk edited edge metadata.

lgtm

lib/Fuzzer/FuzzerUtilWindows.cpp
132 ↗(On Diff #79671)

Huh, I guess that works on Windows, but it just interacts with abort().

This revision is now accepted and ready to land.Nov 30 2016, 12:46 PM
kcc added inline comments.Nov 30 2016, 6:32 PM
lib/Fuzzer/FuzzerUtil.h
44 ↗(On Diff #79671)

do you need this level of indirection?

mpividori added inline comments.Nov 30 2016, 7:30 PM
lib/Fuzzer/FuzzerUtil.h
44 ↗(On Diff #79671)

@kcc
+ This way we make explicit which callback function will be registered for a specific signal. In previous implementation, when calling functions like SetSigSegvHandler(), it's not clear which callback will be called for that given signal.
+ With these changes we decouple the implementation of FuzzerUtils from the rest of the Fuzzer's implementation. In fact, you can see I remove the includes: #include "FuzzerInternal.h" from the FuzzerUtil*.cpp implementations.
+ This changes simplify the implementation of Windows's exception handling.

mpividori added inline comments.Dec 5 2016, 12:04 PM
lib/Fuzzer/FuzzerUtil.h
44 ↗(On Diff #79671)

Hi @kcc ,
Would you agree with these changes?

kcc edited edge metadata.Dec 5 2016, 6:18 PM

Dunno. As is the change increases the code size w/o solving any problem.

rnk added a comment.Dec 6 2016, 2:04 PM
In D27238#614137, @kcc wrote:

Dunno. As is the change increases the code size w/o solving any problem.

The problem, as I see it, is that SetSigFooHandler pattern exposes the wrong cross-platform API. On Windows, we will install exactly one exception handler, switch on the exception code, and then decide whether to catch it based on the user's options.

Would you be OK with this direction if we had a struct of bools instead of a struct of callbacks?

kcc added a comment.Dec 6 2016, 2:30 PM

One way here is to add these booleans to FuzzingOptions

Hi, thanks for your comments. I think the 3 reasons I mentioned before justify this change. At least from my point of view:

  1. This way we make explicit which callback function will be registered for a specific signal. In previous implementation, when calling functions like SetSigSegvHandler(), it's not clear which callback will be called for that given signal.
  2. With these changes we decouple the implementation of FuzzerUtils from the rest of the Fuzzer's implementation. In fact, you can see I remove the includes: #include "FuzzerInternal.h" from the FuzzerUtil*.cpp implementations.
  3. This changes simplify the implementation of Windows's exception handling.

+ I created a new struct SignalHandlerOptions to expose an independent interface for the signal handling, independent from the fuzzer implementation. Another option is to pass the Flags struct from Fuzzer's implementation. Then we can analyse the flags: flags.handle_segv, falgs.handle_ill, etc. (With the disadvantage that we are not decoupling the implementation of FuzzerUtils from the rest of the Fuzzer's implementation (1 and 2), and we are passing far more information that needed...).

+ Another option is to expose this interface:

SetSigSegvHandler(void (*cb)(void));
SetSigIll(void (*cb)(void));
.....

And then we register the callbacks like:

SetSigSegvHandler(Fuzzer::StaticCrashCallback);

This way we achieve the mentioned points 1) and 2), but not 3)

If you think the reasons I mentioned above are not very important, I can remove this diff. :)

Thanks

kcc added a comment.Dec 6 2016, 3:30 PM

Your patch does make sense, but OTOH, indirection and encapsulation are a very powerful tool -- don't overuse them.
I still suggest that you add bool flags to FuzzerOptions and use them in FuzzerUtilWindows.cpp

lib/Fuzzer/FuzzerUtilPosix.cpp
58 ↗(On Diff #79671)

Don't use lambda in such cases because you will see the lambda in the error messages and that will make the messages longer.

mpividori updated this revision to Diff 80625.Dec 7 2016, 10:23 AM
mpividori edited edge metadata.

Done. (Note that we are achieving 3), but not 1) and 2) ).
Thanks.

kcc added a comment.Dec 7 2016, 10:54 AM

non-windows code LGTM

lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

Why do you need a copy of the flags?

mpividori added inline comments.Dec 7 2016, 11:04 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

@kcc,
We can't save a reference to Options, because we don't know when it will be destroyed. The ExceptionHandler() function must access to Handler option the entire life of the program.

kcc added inline comments.Dec 7 2016, 11:09 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

you can rely on this FuzzingOptions object to be live throughout the process.

mpividori added inline comments.Dec 7 2016, 11:17 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

@kcc I wanted to avoid that kind of assumptions. But if you prefer I can do that.....

kcc added inline comments.Dec 7 2016, 11:29 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

the options is a global scope object. It's not an assumption, it's a contract, all of the rest of the code rely on it.

mpividori added inline comments.Dec 7 2016, 11:53 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

@kcc , it is not strictly global, it is defined inside FuzzerDriver(). It will work fine now because we don't do anything after returning from FuzzerDriver(). In the future, if we change the code and do something else after executing FuzzerDriver() which generates an exception, we are in troubles. We must remember this assumption ....
Thanks,

mpividori added inline comments.Dec 12 2016, 8:24 AM
lib/Fuzzer/FuzzerUtilWindows.cpp
30 ↗(On Diff #80625)

@zturner What is your opinion on this?

mpividori updated this revision to Diff 81094.Dec 12 2016, 9:13 AM

@zturner Thanks. I include that changes in this new diff.

zturner accepted this revision.Dec 12 2016, 9:18 AM
zturner edited edge metadata.

@kcc Would you agree with this final diff? Can I push this commit? Thanks.

kcc accepted this revision.Dec 12 2016, 3:50 PM
kcc edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.