[libFuzzer] Diff 8 - Improve synchronization.

[mpividori]

Summary

+ Move the SIGTERM, SIGINT and SIGALRM handlers into a separate thread for Posix implemention (instead of asynchronous signal handler). This is more appropriate for 2 reasons:

• InterruptCallback() and AlarmCallback() use functions like printf, and access to global data, which is not appropriate for a signal handler. By using a separate thread, we are able to synchronize with the main thread.
• Is the same approach than Windows, which makes code simpler, since we are sure that InterruptCallback() and AlarmCallback() will be excecuted in a separate thread in both Windows and Posix systems. Under this assumption, we can safely use locks to synchronize that thread with the main thread. (We couldn't do that if the same code could be executed asynchronously by a signal handler in Posix systems and by a separated thread in Windows).

+ I added a new flag RunningCB to know if the Fuzzer's main thread is running the CB function, instead of using ! CurrentUnitSize. ! CurrentUnitSize doesn't work properly. For example, in FuzzerLoop.cpp, line 452, we execute the callback with size 0. Previous implementation failed to detect timeouts in that execution.

+ Add a mutex RunningCBMtx to synchronize the access to the Fuzzer's data between different threads:
All the information related to the state of the fuzzer is only modified by the main thread, when it is not running the callback function.
So, in order to consistently access to the Fuzzer's data, we should lock the RunningCBMtx and make sure RunningCB is true (the main thread is running the CB).
This is used to synchronize the thread which manages the timers, and the one which supervises rss limits, with the main thread.
In the callbacks: Fuzzer::InterruptCallback(), Fuzzer::AlarmCallback() and Fuzzer::RssLimitCallback(), I lock the mutex RunningCBMtx, before accessing the Fuzzer's data.
I can do that because I am sure that callbacks are executed by a separate thread, both in Posix and Windows implementations.
For the special case of: Fuzzer::CrashCallback(), I can't use the mutex because the callback could be executed asynchronously by the main thread in Posix, where we use signal handlers for crash signals such as: SIGSEGV, SIGILL, etc. So, I do my best and only check for the RunningCB flag.

[kcc]

OMG.
Some of this error handling used to be in a separate thread and I pushed it into the same thread to avoid synchronization problems.
This deserves more discussion, I am not convinced yet.

lib/Fuzzer/FuzzerLoop.cpp

544

please use std::lock_guard

[zturner]

It seems to me like the motivation for this change is that timers on Windows have their callback executed in an arbitrary thread context. If we could fix that -- force the timer to execute its callback in the main thread -- then this problem would go away. Is that right? All other signals (posix) and exceptions (windows) happen on the main thread already, so those are not the problem, only the timer, and only on Windows.

So would it be possible to just implement the timer logic in the main fuzzer loop? Use std::chrono::high_resolution_clock for example. We wouldn't need to treat the callback specially either, when it fired you could use the Win32 API RaiseException which would still trigger the handler you've installed via AddVectoredExceptionHandler.

Thoughts?

[mpividori]

+ Use lock_guard instead of lock() and unlock().

[mpividori]

Hi @kcc @zturner , I will split my explanation in 2 parts:

+) Previous implementation for Posix was not appropriate: (this reason is not related to Windows at all)

It registers signals handlers:

• SIGSEGV SIGFPE SIGBUS SIGILL SIGABRT, call the function CrashCallback().
• SIGINT SIGTERM, call the function InterruptCallback().
• SIGALRM, call the function AlarmCallback().

The implementation of that callbacks include functions like printf() (it isn't async-safe), and access to global data, which is not appropriate for a signal handler that will be executed asynchronously.

Also, that functions are not necessarily executed by the main thread. For example, they could be executed by the thread that supervises rss limit.
In POSIX it is unspecified which thread handle signals. So, while executing that callbacks, we don't know if the main thread continues its execution modifying the fuzzer's state and making it inconsistent.

Because of that, I think it is better to have a separate thread to handle the signals: SIGINT, SIGTERM and SIGALRM (this signals are sent to the process as a whole, we don't know which thread). We will block that signals to all the threads in the actual process, using pthread_sigmask(), and have only one thread waiting for incoming signals with sigwait(). We can be sure that signals will be handled only by that thread and we can synchronize that thread with the main thread to access to the Fuzzer's data.

For signals SIGSEGV SIGFPE SIGBUS SIGILL SIGABRT, (which are sent to a specific thread that generated the given problem, not to the process as a whole), we can't use a separate thread. They will be probably executed by the main thread, when that problems are caused inside the execution of the CB function being fuzzed. Because of that, I don't use the lock inside CrashCallback(), because it will be called by the main thread. I can't lock, because locks functions are not async-safe, but at the same time, I am sure the main thread will not continue executing and modifying the fuzzer data (because it is executing this callback), so all I have to do is check the flag RunningCB to know if the fuzzer's data is consistent.

So, this is the main reason of that changes for Posix implementation. Move all of the signal handling into a separate thread when possible.

+) In Windows, we implement similar functionalities using:

• Vectored Exception Handling for exceptions similar to the signals: SIGSEGV SIGFPE SIGBUS SIGILL.
• Signal for: SIGABRT.
• SetConsoleCtrlHandler(), for behaviour similar to: SIGINT, SIGTERM.
• Timer Queues for timer implementation, similar to: SIGALRM

The callbacks InterruptCallback() registered by SetConsoleCtrlHandler(), and AlarmCallback() registered by CreateTimerQueueTimer(), will be executed in a separate thread.
So, we need to synchronize the access to the fuzzer's data by that threads. The main thread could continue its execution and make the data inconsistent.

So, with these changes, we make code simpler, since we are sure that InterruptCallback() and AlarmCallback() will be excecuted in a separate thread in both Windows and Posix systems. Under this assumption, we can safely use locks to synchronize that thread with the main thread.

[mpividori]

After personally discussing this with @kcc and @zturner , we decided that we will continue with the previous implementation, even if it is not completely correct.
Main reasons:

• It was mentioned that this implementation makes code more complex.
• It was mentioned that relying in signal masking is not a good idea because that implementation is generally full of bugs in posix*s systems.

Thanks.

lib/Fuzzer/FuzzerInternal.h

 #ifndef LLVM_FUZZER_INTERNAL_H
 #define LLVM_FUZZER_INTERNAL_H
 
 #include <algorithm>
 #include <atomic>
 #include <chrono>
 #include <climits>
 #include <cstdlib>
+#include <mutex>
 #include <string.h>
 
 #include "FuzzerDefs.h"
 #include "FuzzerExtFunctions.h"
 #include "FuzzerInterface.h"
 #include "FuzzerOptions.h"
 #include "FuzzerSHA1.h"
 #include "FuzzerValueBitMap.h"
private:
   size_t RunOne(const Unit &U) { return RunOne(U.data(), U.size()); }
   void WriteToOutputCorpus(const Unit &U);
   void WriteUnitToFileWithPrefix(const Unit &U, const char *Prefix);
   void PrintStats(const char *Where, const char *End = "\n", size_t Units = 0);
   void PrintStatusForNewUnit(const Unit &U);
   void ShuffleCorpus(UnitVector *V);
   void AddToCorpus(const Unit &U);
   void CheckExitOnSrcPosOrItem();
+  void SetRunningCB(bool Value);
 
   // Trace-based fuzzing: we run a unit with some kind of tracing
   // enabled and record potentially useful mutations. Then
   // We apply these mutations one by one to the unit and run it again.
 
   // Start tracing; forget all previously proposed mutations.
   void StartTraceRecording();
   // Stop tracing.
   void StopTraceRecording();
 
   void SetDeathCallback();
   static void StaticDeathCallback();
   void DumpCurrentUnit(const char *Prefix);
   void DeathCallback();
 
   void ResetEdgeCoverage();
   void ResetCounters();
   void PrepareCounters(Fuzzer::Coverage *C);
   bool RecordMaxCoverage(Fuzzer::Coverage *C);
 
   void AllocateCurrentUnitData();
   uint8_t *CurrentUnitData = nullptr;
   std::atomic<size_t> CurrentUnitSize;
+  bool RunningCB;
+  std::mutex RunningCBMtx;
   uint8_t BaseSha1[kSHA1NumBytes];  // Checksum of the base unit.
 
   size_t TotalNumberOfRuns = 0;
   size_t NumberOfNewUnitsAdded = 0;
 
   bool HasMoreMallocsThanFrees = false;
   size_t NumberOfLeakDetectionAttempts = 0;
 

lib/Fuzzer/FuzzerLoop.cpp

void Fuzzer::HandleMalloc(size_t Size) {
   DumpCurrentUnit("oom-");
   Printf("SUMMARY: libFuzzer: out-of-memory\n");
   PrintFinalStats();
   _Exit(Options.ErrorExitCode); // Stop right now.
 }
 
 Fuzzer::Fuzzer(UserCallback CB, InputCorpus &Corpus, MutationDispatcher &MD,
                FuzzingOptions Options)
-    : CB(CB), Corpus(Corpus), MD(MD), Options(Options) {
+    : RunningCB(false), CB(CB), Corpus(Corpus), MD(MD), Options(Options) {
   SetDeathCallback();
   InitializeTraceState();
   assert(!F);
   F = this;
   TPC.ResetMaps();
   ResetCoverage();
   IsMyThread = true;
   if (Options.DetectLeaks && EF->__sanitizer_install_malloc_and_free_hooks)

 void Fuzzer::CrashCallback() {
   Printf("==%d== ERROR: libFuzzer: deadly signal\n", GetPid());
   if (EF->__sanitizer_print_stack_trace)
     EF->__sanitizer_print_stack_trace();
   Printf("NOTE: libFuzzer has rudimentary signal handlers.\n"
          "      Combine libFuzzer with AddressSanitizer or similar for better "
          "crash reports.\n");
   Printf("SUMMARY: libFuzzer: deadly signal\n");
+  if (RunningCB) { // The fuzzer is in a consistent state.
     DumpCurrentUnit("crash-");
     PrintFinalStats();
+  }
   exit(Options.ErrorExitCode);
 }
 
 void Fuzzer::InterruptCallback() {
   Printf("==%d== libFuzzer: run interrupted; exiting\n", GetPid());
+  std::lock_guard<std::mutex> Lock(RunningCBMtx);
+  if (RunningCB) // The fuzzer is in a consistent state.
     PrintFinalStats();
   _Exit(0);  // Stop right now, don't perform any at-exit actions.
 }
 
 NO_SANITIZE_MEMORY
 void Fuzzer::AlarmCallback() {
   assert(Options.UnitTimeoutSec > 0);
-  if (!InFuzzingThread()) return;
-  if (!CurrentUnitSize)
+  std::lock_guard<std::mutex> Lock(RunningCBMtx);
+  if (!RunningCB)
     return; // We have not started running units yet.
   size_t Seconds =
       duration_cast<seconds>(system_clock::now() - UnitStartTime).count();
   if (Seconds == 0)
     return;
   if (Options.Verbosity >= 2)
     Printf("AlarmCallback %zd\n", Seconds);
   if (Seconds >= (size_t)Options.UnitTimeoutSec) {

 
 void Fuzzer::RssLimitCallback() {
   Printf(
       "==%d== ERROR: libFuzzer: out-of-memory (used: %zdMb; limit: %zdMb)\n",
       GetPid(), GetPeakRSSMb(), Options.RssLimitMb);
   Printf("   To change the out-of-memory limit use -rss_limit_mb=<N>\n\n");
   if (EF->__sanitizer_print_memory_profile)
     EF->__sanitizer_print_memory_profile(95);
-  DumpCurrentUnit("oom-");
   Printf("SUMMARY: libFuzzer: out-of-memory\n");
+  std::lock_guard<std::mutex> Lock(RunningCBMtx);
+  if (RunningCB) { // The fuzzer is in a consistent state.
+    DumpCurrentUnit("oom-");
     PrintFinalStats();
+  }
   _Exit(Options.ErrorExitCode); // Stop right now.
 }
 
 void Fuzzer::PrintStats(const char *Where, const char *End, size_t Units) {
   size_t ExecPerSec = execPerSec();
   if (Options.OutputCSV) {
     static bool csvHeaderPrinted = false;
     if (!csvHeaderPrinted) {
if (!Options.ExitOnItem.empty()) {
     if (Corpus.HasUnit(Options.ExitOnItem)) {
       Printf("INFO: found item with checksum '%s', exiting.\n",
              Options.ExitOnItem.c_str());
       _Exit(0);
     }
   }
 }
 
+void Fuzzer::SetRunningCB(bool Value) {
+  std::lock_guard<std::mutex> Lock(RunningCBMtx);
+  RunningCB = Value;
+}
+
 void Fuzzer::RereadOutputCorpus(size_t MaxSize) {
   if (Options.OutputCorpus.empty() || !Options.ReloadIntervalSec) return;
   std::vector<Unit> AdditionalCorpus;
   ReadDirToVectorOfUnits(Options.OutputCorpus.c_str(), &AdditionalCorpus,
                          &EpochOfLastReadOfOutputCorpus, MaxSize,
                          /*ExitOnError*/ false);
   if (Options.Verbosity >= 2)
     Printf("Reload: read %zd new units.\n", AdditionalCorpus.size());
void Fuzzer::ExecuteCallback(const uint8_t *Data, size_t Size) {
   memcpy(DataCopy, Data, Size);
   if (CurrentUnitData && CurrentUnitData != Data)
     memcpy(CurrentUnitData, Data, Size);
   CurrentUnitSize = Size;
   AllocTracer.Start(Options.TraceMalloc);
   UnitStartTime = system_clock::now();
   ResetCounters();  // Reset coverage right before the callback.
   TPC.ResetMaps();
+  SetRunningCB(true);
   int Res = CB(DataCopy, Size);
+  SetRunningCB(false);
   UnitStopTime = system_clock::now();
   (void)Res;
   assert(Res == 0);
   HasMoreMallocsThanFrees = AllocTracer.Stop();
   CurrentUnitSize = 0;
   delete[] DataCopy;
 }
 

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);
   }
 }
 
+static void SignalHandlerThread(sigset_t Set) {
+  int Sig;
+  while (true) {
+    sigwait(&Set, &Sig);
+    switch (Sig) {
+      case SIGALRM:
+        HandlerOpt.sigalrm_cb();
+        break;
+      case SIGINT:
+        HandlerOpt.sigint_cb();
+        break;
+      case SIGTERM:
+        HandlerOpt.sigterm_cb();
+        break;
+    }
+  }
+}
+
 void SetSignalHandler(const SignalHandlerOptions& Opt) {
   HandlerOpt = Opt;
 
+  sigset_t Set;
+  sigemptyset(&Set);
+
+  // Set SIGALRM, SIGING and SIGTERM to be handled in a separate thread.
   if (Opt.timeout > 0 && Opt.sigalrm_cb) {
+    sigaddset(&Set, SIGALRM);
     struct itimerval T {{Opt.timeout, 0}, {Opt.timeout, 0}};
     if (setitimer(ITIMER_REAL, &T, nullptr)) {
       Printf("libFuzzer: setitimer failed with %d\n", errno);
       exit(1);
     }
-    SetSigaction(SIGALRM, [](int,siginfo_t*,void*) {HandlerOpt.sigalrm_cb();});
   }
   if (Opt.sigint_cb)
-    SetSigaction(SIGINT, [](int,siginfo_t*,void*) {HandlerOpt.sigint_cb();});
+    sigaddset(&Set, SIGINT);
   if (Opt.sigterm_cb)
-    SetSigaction(SIGTERM, [](int,siginfo_t*,void*) {HandlerOpt.sigterm_cb();});
+    sigaddset(&Set, SIGTERM);
+
+  if ((Opt.timeout > 0 && Opt.sigalrm_cb) || Opt.sigint_cb || Opt.sigterm_cb) {
+    //Block the listed signals in all future threads.
+    int Err;
+    if ((Err = pthread_sigmask(SIG_BLOCK, &Set, NULL))) {
+      Printf("libFuzzer: pthread_sigmask failed with %d.\n", Err);
+      exit(1);
+    }
+    // Start thread to wait for incoming signals.
+    std::thread T(SignalHandlerThread, Set);
+    T.detach();
+  }
+
   if (Opt.sigsegv_cb)
     SetSigaction(SIGSEGV, [](int,siginfo_t*,void*) {HandlerOpt.sigsegv_cb();});
   if (Opt.sigbus_cb)
     SetSigaction(SIGBUS, [](int,siginfo_t*,void*) {HandlerOpt.sigbus_cb();});
   if (Opt.sigabrt_cb)
     SetSigaction(SIGABRT, [](int,siginfo_t*,void*) {HandlerOpt.sigabrt_cb();});
   if (Opt.sigill_cb)
     SetSigaction(SIGILL, [](int,siginfo_t*,void*) {HandlerOpt.sigill_cb();});