Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lgtm
lib/Fuzzer/FuzzerUtilWindows.cpp | ||
---|---|---|
132 ↗ | (On Diff #79671) | Huh, I guess that works on Windows, but it just interacts with abort(). |
lib/Fuzzer/FuzzerUtil.h | ||
---|---|---|
44 ↗ | (On Diff #79671) | do you need this level of indirection? |
lib/Fuzzer/FuzzerUtil.h | ||
---|---|---|
44 ↗ | (On Diff #79671) | @kcc |
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?
Hi, thanks for your comments. I think the 3 reasons I mentioned before justify this change. At least from my point of view:
- 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.
+ 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
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. |
non-windows code LGTM
lib/Fuzzer/FuzzerUtilWindows.cpp | ||
---|---|---|
30 ↗ | (On Diff #80625) | Why do you need a copy of the flags? |
lib/Fuzzer/FuzzerUtilWindows.cpp | ||
---|---|---|
30 ↗ | (On Diff #80625) | you can rely on this FuzzingOptions object to be live throughout the process. |
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. |
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 .... |