[libFuzzer] Diff 7 - Improve Signal Handler interface.

[zturner]

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

[rnk]

lgtm

lib/Fuzzer/FuzzerUtilWindows.cpp

151

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

[kcc]

lib/Fuzzer/FuzzerUtil.h

46

do you need this level of indirection?

[mpividori]

lib/Fuzzer/FuzzerUtil.h

46

@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]

lib/Fuzzer/FuzzerUtil.h

46

Hi @kcc ,
Would you agree with these changes?

[kcc]

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

[rnk]

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]

One way here is to add these booleans to FuzzingOptions

[mpividori]

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]

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

63

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]

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

[kcc]

non-windows code LGTM

lib/Fuzzer/FuzzerUtilWindows.cpp

30

Why do you need a copy of the flags?

[mpividori]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

@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]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

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

[mpividori]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

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

[kcc]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

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]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

@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]

lib/Fuzzer/FuzzerUtilWindows.cpp

30

@zturner What is your opinion on this?

[mpividori]

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

[mpividori]

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

[kcc]

LGTM

lib/Fuzzer/FuzzerDriver.cpp

int FuzzerDriver(int *argc, char ***argv, UserCallback Callback) {
   auto *F = new Fuzzer(Callback, *Corpus, *MD, Options);
 
   for (auto &U: Dictionary)
     if (U.size() <= Word::GetMaxSize())
       MD->AddWordToManualDictionary(Word(U.data(), U.size()));
 
   StartRssThread(F, Flags.rss_limit_mb);
 
-  // Timer
-  if (Flags.timeout > 0)
-    SetTimer(Flags.timeout / 2 + 1);
-  if (Flags.handle_segv) SetSigSegvHandler();
-  if (Flags.handle_bus) SetSigBusHandler();
-  if (Flags.handle_abrt) SetSigAbrtHandler();
-  if (Flags.handle_ill) SetSigIllHandler();
-  if (Flags.handle_fpe) SetSigFpeHandler();
-  if (Flags.handle_int) SetSigIntHandler();
-  if (Flags.handle_term) SetSigTermHandler();
+  Options.HandleAbrt = Flags.handle_abrt;
+  Options.HandleBus = Flags.handle_bus;
+  Options.HandleFpe = Flags.handle_fpe;
+  Options.HandleIll = Flags.handle_ill;
+  Options.HandleInt = Flags.handle_int;
+  Options.HandleSegv = Flags.handle_segv;
+  Options.HandleTerm = Flags.handle_term;
+  SetSignalHandler(Options);
 
   if (Flags.minimize_crash_internal_step)
     return MinimizeCrashInputInternalStep(F, Corpus);
 
   if (DoPlainRun) {
     Options.SaveArtifacts = false;
     int Runs = std::max(1, Flags.runs);
     Printf("%s: Running %zd inputs %d time(s) each.\n", ProgName->c_str(),

lib/Fuzzer/FuzzerOptions.h

struct FuzzingOptions {
   bool PrintNEW = true; // Print a status line when new units are found;
   bool OutputCSV = false;
   bool PrintNewCovPcs = false;
   bool PrintFinalStats = false;
   bool PrintCorpusStats = false;
   bool PrintCoverage = false;
   bool DetectLeaks = true;
   int  TraceMalloc = 0;
+  bool HandleAbrt = false;
+  bool HandleBus = false;
+  bool HandleFpe = false;
+  bool HandleIll = false;
+  bool HandleInt = false;
+  bool HandleSegv = false;
+  bool HandleTerm = false;
 };
 
 }  // namespace fuzzer
 
 #endif  // LLVM_FUZZER_OPTIONS_H

lib/Fuzzer/FuzzerUtil.h

 
 std::string DescribePC(const char *SymbolizedFMT, uintptr_t PC);
 
 int NumberOfCpuCores();
 
 bool ExecuteCommandAndReadOutput(const std::string &Command, std::string *Out);
 
 // Platform specific functions.
-void SetTimer(int Seconds);
-
-void SetSigSegvHandler();
-void SetSigBusHandler();
-void SetSigAbrtHandler();
-void SetSigIllHandler();
-void SetSigFpeHandler();
-void SetSigIntHandler();
-void SetSigTermHandler();
+void SetSignalHandler(const FuzzingOptions& Options);
 
 void SleepSeconds(int Seconds);
 
 int GetPid();
 
 size_t GetPeakRSSMb();
 
 int ExecuteCommand(const std::string &Command);
 
 FILE* OpenProcessPipe(const char *Command, const char *Mode);
 
 const void *SearchMemory(const void *haystack, size_t haystacklen,
                          const void *needle, size_t needlelen);
 
 }  // namespace fuzzer
 #endif  // LLVM_FUZZER_UTIL_H

lib/Fuzzer/FuzzerUtilPosix.cpp

static void SetSigaction(int signum,
   memset(&sigact, 0, sizeof(sigact));
   sigact.sa_sigaction = callback;
   if (sigaction(signum, &sigact, 0)) {
     Printf("libFuzzer: sigaction failed with %d\n", errno);
     exit(1);
   }
 }
 
-void SetTimer(int Seconds) {
+static void SetTimer(int Seconds) {
   struct itimerval T {{Seconds, 0}, {Seconds, 0}};
   if (setitimer(ITIMER_REAL, &T, nullptr)) {
     Printf("libFuzzer: setitimer failed with %d\n", errno);
     exit(1);
   }
   SetSigaction(SIGALRM, AlarmHandler);
 }
 
-void SetSigSegvHandler() { SetSigaction(SIGSEGV, CrashHandler); }
-void SetSigBusHandler() { SetSigaction(SIGBUS, CrashHandler); }
-void SetSigAbrtHandler() { SetSigaction(SIGABRT, CrashHandler); }
-void SetSigIllHandler() { SetSigaction(SIGILL, CrashHandler); }
-void SetSigFpeHandler() { SetSigaction(SIGFPE, CrashHandler); }
-void SetSigIntHandler() { SetSigaction(SIGINT, InterruptHandler); }
-void SetSigTermHandler() { SetSigaction(SIGTERM, InterruptHandler); }
+void SetSignalHandler(const FuzzingOptions& Options) {
+  if (Options.UnitTimeoutSec > 0)
+    SetTimer(Options.UnitTimeoutSec / 2 + 1);
+  if (Options.HandleInt)
+    SetSigaction(SIGINT, InterruptHandler);
+  if (Options.HandleTerm)
+    SetSigaction(SIGTERM, InterruptHandler);
+  if (Options.HandleSegv)
+    SetSigaction(SIGSEGV, CrashHandler);
+  if (Options.HandleBus)
+    SetSigaction(SIGBUS, CrashHandler);
+  if (Options.HandleAbrt)
+    SetSigaction(SIGABRT, CrashHandler);
+  if (Options.HandleIll)
+    SetSigaction(SIGILL, CrashHandler);
+  if (Options.HandleFpe)
+    SetSigaction(SIGFPE, CrashHandler);
+}
 
 void SleepSeconds(int Seconds) {
   sleep(Seconds);  // Use C API to avoid coverage from instrumented libc++.
 }
 
 int GetPid() { return getpid(); }
 
 size_t GetPeakRSSMb() {

lib/Fuzzer/FuzzerUtilWindows.cpp

 #include <sstream>
 #include <stdio.h>
 #include <sys/types.h>
 #include <windows.h>
 #include <Psapi.h>
 
 namespace fuzzer {
 
-LONG WINAPI SEGVHandler(PEXCEPTION_POINTERS ExceptionInfo) {
+static struct {
+   bool HandleAbrt = false;
+   bool HandleBus = false;
+   bool HandleFpe = false;
+   bool HandleIll = false;
+   bool HandleInt = false;
+   bool HandleSegv = false;
+   bool HandleTerm = false;
+} HandlerOpt;
+
+LONG CALLBACK ExceptionHandler(PEXCEPTION_POINTERS ExceptionInfo) {
   switch (ExceptionInfo->ExceptionRecord->ExceptionCode) {
     case EXCEPTION_ACCESS_VIOLATION:
     case EXCEPTION_ARRAY_BOUNDS_EXCEEDED:
     case EXCEPTION_STACK_OVERFLOW:
+      if (HandlerOpt.HandleSegv)
         Fuzzer::StaticCrashSignalCallback();
       break;
-  }
-  return EXCEPTION_CONTINUE_SEARCH;
-}
-
-LONG WINAPI BUSHandler(PEXCEPTION_POINTERS ExceptionInfo) {
-  switch (ExceptionInfo->ExceptionRecord->ExceptionCode) {
     case EXCEPTION_DATATYPE_MISALIGNMENT:
     case EXCEPTION_IN_PAGE_ERROR:
+      if (HandlerOpt.HandleBus)
         Fuzzer::StaticCrashSignalCallback();
       break;
-  }
-  return EXCEPTION_CONTINUE_SEARCH;
-}
-
-LONG WINAPI ILLHandler(PEXCEPTION_POINTERS ExceptionInfo) {
-  switch (ExceptionInfo->ExceptionRecord->ExceptionCode) {
     case EXCEPTION_ILLEGAL_INSTRUCTION:
     case EXCEPTION_PRIV_INSTRUCTION:
+      if (HandlerOpt.HandleIll)
         Fuzzer::StaticCrashSignalCallback();
       break;
-  }
-  return EXCEPTION_CONTINUE_SEARCH;
-}
-
-LONG WINAPI FPEHandler(PEXCEPTION_POINTERS ExceptionInfo) {
-  switch (ExceptionInfo->ExceptionRecord->ExceptionCode) {
     case EXCEPTION_FLT_DENORMAL_OPERAND:
     case EXCEPTION_FLT_DIVIDE_BY_ZERO:
     case EXCEPTION_FLT_INEXACT_RESULT:
     case EXCEPTION_FLT_INVALID_OPERATION:
     case EXCEPTION_FLT_OVERFLOW:
     case EXCEPTION_FLT_STACK_CHECK:
     case EXCEPTION_FLT_UNDERFLOW:
     case EXCEPTION_INT_DIVIDE_BY_ZERO:
     case EXCEPTION_INT_OVERFLOW:
+      if (HandlerOpt.HandleFpe)
         Fuzzer::StaticCrashSignalCallback();
       break;
   }
   return EXCEPTION_CONTINUE_SEARCH;
 }
 
-BOOL WINAPI INTHandler(DWORD dwCtrlType) {
+BOOL WINAPI CtrlHandler(DWORD dwCtrlType) {
   switch (dwCtrlType) {
     case CTRL_C_EVENT:
+      if (HandlerOpt.HandleInt)
         Fuzzer::StaticInterruptCallback();
       return TRUE;
-    default:
-      return FALSE;
-  }
-}
-
-BOOL WINAPI TERMHandler(DWORD dwCtrlType) {
-  switch (dwCtrlType) {
     case CTRL_BREAK_EVENT:
+      if (HandlerOpt.HandleTerm)
         Fuzzer::StaticInterruptCallback();
       return TRUE;
-    default:
-      return FALSE;
   }
+  return FALSE;
 }
 
 void CALLBACK AlarmHandler(PVOID, BOOLEAN) {
   Fuzzer::StaticAlarmCallback();
 }
 
 class TimerQ {
   HANDLE TimerQueue;
if (!CreateTimerQueueTimer(&Timer, TimerQueue, AlarmHandler, NULL,
       Printf("libFuzzer: CreateTimerQueueTimer failed.\n");
       exit(1);
     }
   };
 };
 
 static TimerQ Timer;
 
-void SetTimer(int Seconds) {
-  Timer.SetTimer(Seconds);
-  return;
-}
-
-void SetSigSegvHandler() {
-  if (!AddVectoredExceptionHandler(1, SEGVHandler)) {
-    Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
-    exit(1);
-  }
-}
-
-void SetSigBusHandler() {
-  if (!AddVectoredExceptionHandler(1, BUSHandler)) {
-    Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
-    exit(1);
-  }
-}
-
 static void CrashHandler(int) {
   Fuzzer::StaticCrashSignalCallback();
 }
 
-void SetSigAbrtHandler() { signal(SIGABRT, CrashHandler); }
+void SetSignalHandler(const FuzzingOptions& Options) {
+  HandlerOpt.HandleAbrt = Options.HandleAbrt;
+  HandlerOpt.HandleBus = Options.HandleBus;
+  HandlerOpt.HandleFpe = Options.HandleFpe;
+  HandlerOpt.HandleIll = Options.HandleIll;
+  HandlerOpt.HandleInt = Options.HandleInt;
+  HandlerOpt.HandleSegv = Options.HandleSegv;
+  HandlerOpt.HandleTerm = Options.HandleTerm;
 
-void SetSigIllHandler() {
-  if (!AddVectoredExceptionHandler(1, ILLHandler)) {
-    Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
-    exit(1);
-  }
-}
-
-void SetSigFpeHandler() {
-  if (!AddVectoredExceptionHandler(1, FPEHandler)) {
-    Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
-    exit(1);
-  }
-}
+  if (Options.UnitTimeoutSec > 0)
+    Timer.SetTimer(Options.UnitTimeoutSec / 2 + 1);
 
-void SetSigIntHandler() {
-  if (!SetConsoleCtrlHandler(INTHandler, TRUE)) {
+  if (Options.HandleInt || Options.HandleTerm)
+    if (!SetConsoleCtrlHandler(CtrlHandler, TRUE)) {
       DWORD LastError = GetLastError();
       Printf("libFuzzer: SetConsoleCtrlHandler failed (Error code: %lu).\n",
         LastError);
       exit(1);
     }
+
+  if (Options.HandleSegv || Options.HandleBus || Options.HandleIll ||
+      Options.HandleFpe)
+    if (!AddVectoredExceptionHandler(1, ExceptionHandler)) {
+      Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
+      exit(1);
     }
 
-void SetSigTermHandler() {
-  if (!SetConsoleCtrlHandler(TERMHandler, TRUE)) {
-    DWORD LastError = GetLastError();
-    Printf("libFuzzer: SetConsoleCtrlHandler failed (Error code: %lu).\n",
-      LastError);
+  if (Options.HandleAbrt)
+    if (SIG_ERR == signal(SIGABRT, CrashHandler)) {
+      Printf("libFuzzer: signal failed with %d\n", errno);
       exit(1);
     }
 }
 
 void SleepSeconds(int Seconds) {
   Sleep(Seconds * 1000);
 }
 
 int GetPid() { return GetCurrentProcessId(); }