LibFuzzer - Implement timers for Windows and improve synchronization.

[mpividori]

Summary

+ Implemented timeouts for Windows using TimerQueueTimers.
+ Modified the implementation of timer for Posix systems. Instead of using ALRM signals, I create a new thread.

This simplifies the code, since both Posix and Windows implementations use a special thread to call AlarmCallback().
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).
Also, I realized that previous implementation assumed that the ALRM signals would be handled by the main thread, which is not necessarily true. In POSIX it is unspecified with thread handle signals.

+ 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.

[mpividori]

+ Move the interruption handler into a separate thread for Posix implemention (instead of asynchronous signal handler). This is more appropriate for 2 reasons:

• InterruptCallback() uses functions like printf, and accesses 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() will be excecuted in a separate thread in both Windows and Posix systems.

+ Improve Signal Handler interface (FuzzerUtil*.cpp files).

[mpividori]

+ Remove: #include "FuzzerInternal.h" in FuzzerUtil*.cpp files.

[zturner]

+amccarth for the windows stuff. +kcc for the posix refactoring.

[zturner]

lib/Fuzzer/FuzzerUtil.h

47–54 ↗

(On Diff #79594)

How about getting rid of the constructor and initializing all these inline?

int timeout = 0;
HandlerT sigalrm_cb = nullptr;
// etc

lib/Fuzzer/FuzzerUtilWindows.cpp

30

@kcc: Are there any issues with using static global constructors in libfuzzer?

195–196

Shouldn't use all delete the timer as well? You store it locally in SetTimer but then let the handle leak. So every time we call SetTimer it will leak a handle.

[mpividori]

lib/Fuzzer/FuzzerUtil.h

47–54 ↗

(On Diff #79594)

@zturner , I think using a constructor is clearer. Do you mean to avoid the static constructor? In that case, I can remove it and initialize that members inline inside SetSignalHandler().

lib/Fuzzer/FuzzerUtilWindows.cpp

30

@zturner , the Code Standards mentions that we should avoid static constructors: [[1]](http://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors). In this case, I included it because it is a very simple constructor. I can remove it and initialize that struct inside SetSignalHandler().

195–196

According to the documentation, the timers which were added to the TimerQueue are automatically deleted when deleting the TimerQueue.

[zturner]

lib/Fuzzer/FuzzerUtil.h

47–54 ↗

(On Diff #79594)

I don't know if it's stated anywhere in the coding standards, but there has been an push in the LLVM community recently to use inline initialization instead of constructor initializer list in cases such as this where there are long lists of member variables initialized with constant values. So your entire class definition would look like:

struct SignalHandlerOptions {
  int timeout = 0;
  HandlerT sigalrm_cb = nullptr;
  HandlerT sigsegv_cb = nullptr;
  HandlerT sigbus_cb = nullptr;
  HandlerT sigabrt_cb = nullptr;
  HandlerT sigill_cb = nullptr;
  HandlerT sigfpe_cb = nullptr;
  HandlerT sigint_cb = nullptr;
  HandlerT sigterm_cb = nullptr;
};

And there would be no constructor definition. In this case it's not critical, but the motivation is that when you have multiple constructor overloads that all repeat the same exact initializer list, this syntax reduces boilerplate and reduces the chance of 2 constructors accidentally initializing things differently.

lib/Fuzzer/FuzzerUtilWindows.cpp

195–196

Ahh, I didn't notice that. But it seems we can still create multiple timers here. For example if you call the SetTimer twice it will add a new timer instead of overwriting the old timer. Is this desired? If a user is not supposed to call SetTimer twice, then maybe we should add assert(!TimerQueue). But if it is ok to call SetTimer more than once and have subsequent calls overwrite the existing timer value, then we should probably store the HANDLE in the class and delete it and recreate on subsequent calls.

Also, one thing I just thought of that might be a problem. The timer callback is not going to be invoked on the fuzzer thread, but rather a thread in the windows system thread pool. But I looked at the Alarm callback function and it begins like this:

if (!InFuzzingThread()) return;

So, I think this check is going to always fail. If we need to guarantee that alarms happen on the fuzzing thread, we need a different strategy.

What do you think?

[mpividori]

lib/Fuzzer/FuzzerUtil.h

47–54 ↗

(On Diff #79594)

@zturner , Thanks. Yes, I agree. It is simpler and reduces boilerplate code. I will add that changes.

lib/Fuzzer/FuzzerUtilWindows.cpp

195–196

@zturner thanks for your comments.
+ Yes, we can create multiple timers here, but we don't expose the function SetTimer any more (I removed that), so the user doesn't have access to it. The Timer instance is only accessed by SetSignalHandler() function. Anyway, I can add the changes you proposed.
Also, the code could be simplified by using a separate thread for the timer instead of using TimerQueues. Something like:

void TimerThread(int Seconds) {
  while (1) {
    sleep(Seconds);
    AlrmCallback();
  }
}

I wasn't sure which option is prefered. What do you think? If we consider a separate thread, we could use the same implementation for Posix too, and get rid of SIGALRM.

+ The if (!InFuzzingThread()) return; line was removed in these changes. Yes, I adapted the code to handle that situation.

Previous implementation assumed that the ALRM signals would be handled by the main thread, which is not necessarily true. In POSIX it is unspecified with thread handle signals.
With the changes included in this diff, we can be sure that Alarm callback is executed by a thread different to the main thread in both Posix and Windows implementations:

• In Windows: by a thread from the windows system thread pool.
• In Posix: by a separate thread that waits for SIGALRM signals.

[zturner]

lib/Fuzzer/FuzzerInternal.h

146–147

It seems like we could use std::atomic_bool here. Does that not work for some reason? The only thing I see that is questionable is that in FuzzerCallback::CrashCallback we check the value, and then run multiple lines of code before returning.

But on the other hand, in Fuzzer::ExecuteCallback, we always set it to true before executing any callback, and false after. So maybe there is no race condition?

atomic_bool is more lightweight than grabbing a full blown mutex, so if we can be sure it's safe, then it seems better.

lib/Fuzzer/FuzzerUtilWindows.cpp

195–196

I don't think we need to create a thread for it, using the windows threadpool is more efficient.

[mpividori]

lib/Fuzzer/FuzzerInternal.h

146–147

@zturner, thanks for your comments.
I can't use std::atomic_bool because not only do I need to be sure it is modified atomically, buy also I need to synchronize the main thread with the rest of the threads accessing the Fuzzer's data. Because of that I added the mutex RunningCBMtx.
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).
By locking the RunningCBMtx, we can be sure that the main thread won't change its state in RunningCB.
This is used to synchronize the thread which manages the timers, and the one which supervises rss limits, with the main thread.

[mpividori]

lib/Fuzzer/FuzzerInternal.h

146–147

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 in Posix and Windows implementations.
For the special case of: Fuzzer::CrashCallback(), I can't use the mutex because the callback can 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.

[mpividori]

+ Initialize inline the members of SignalHandlerOptions.

[amccarth]

I focused on the Windows-specific code, and it looks good to me as long as the signal callbacks are doing anything tricky.

lib/Fuzzer/FuzzerUtilWindows.cpp

32

Tiny little irrelevant nit: s/WINAPI/CALLBACK/

In modern times, there is no practical distinction, as they both evaluate to __stdcall. Technically, the exception handler is a callback rather than a Windows API.

36

I'm not sure what these callbacks may be doing in lib fuzzer. Be aware that exception handlers cannot acquire any synchronization objects and cannot allocate memory, presumably because that could cause a deadlock.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms681419(v=vs.85).aspx

[mpividori]

lib/Fuzzer/FuzzerUtilWindows.cpp

36

@amccarth, thanks for your comments.
Yes, in fact all that callbacks call Fuzzer::CrashCallback() which doesn't consider the synchronization lock, as I mentioned before.

[mpividori]

I splitted this diff in :
+ https://reviews.llvm.org/D27237
+ https://reviews.llvm.org/D27238
+ https://reviews.llvm.org/D27240

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:
   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

if (int TraceLevel = AllocTracer.TraceLevel) {
     Printf("FREE[%zd]   %p\n", N, ptr);
     if (TraceLevel >= 2 && EF)
       EF->__sanitizer_print_stack_trace();
   }
 }
 
 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());
+  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) {
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();
+  RunningCBMtx.lock();
+  RunningCB = true;
+  RunningCBMtx.unlock();
   int Res = CB(DataCopy, Size);
+  RunningCBMtx.lock();
+  RunningCB = false;
+  RunningCBMtx.unlock();
   UnitStopTime = system_clock::now();
   (void)Res;
   assert(Res == 0);
   HasMoreMallocsThanFrees = AllocTracer.Stop();
   CurrentUnitSize = 0;
   delete[] DataCopy;
 }
 

lib/Fuzzer/FuzzerUtilPosix.cpp

 #include <sys/syscall.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <thread>
 #include <unistd.h>
 
 namespace fuzzer {
 
-static void AlarmHandler(int, siginfo_t *, void *) {
-  Fuzzer::StaticAlarmCallback();
-}
-
 static void CrashHandler(int, siginfo_t *, void *) {
   Fuzzer::StaticCrashSignalCallback();
 }
 
 static void InterruptHandler(int, siginfo_t *, void *) {
   Fuzzer::StaticInterruptCallback();
 }
 
 static void SetSigaction(int signum,
                          void (*callback)(int, siginfo_t *, void *)) {
   struct sigaction sigact;
   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) {
-  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 SleepSeconds(int Seconds) {
   sleep(Seconds);  // Use C API to avoid coverage from instrumented libc++.
 }
 
+static void TimerThread(int Seconds) {
+  while (true) {
+    SleepSeconds(Seconds);
+    Fuzzer::StaticAlarmCallback();
+  }
+}
+
+void SetTimer(int Seconds) {
+  std::thread T(TimerThread, Seconds);
+  T.detach();
+}
+
 int GetPid() { return getpid(); }
 
 size_t GetPeakRSSMb() {
   struct rusage usage;
   if (getrusage(RUSAGE_SELF, &usage))
     return 0;
   if (LIBFUZZER_LINUX) {
     // ru_maxrss is in KiB

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) {
   switch (ExceptionInfo->ExceptionRecord->ExceptionCode) {
     case EXCEPTION_ACCESS_VIOLATION:
     case EXCEPTION_ARRAY_BOUNDS_EXCEEDED:
     case EXCEPTION_STACK_OVERFLOW:
       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:
switch (dwCtrlType) {
     case CTRL_BREAK_EVENT:
       Fuzzer::StaticInterruptCallback();
       return TRUE;
     default:
       return FALSE;
   }
 }
 
+void CALLBACK ALRMHandler(PVOID /*lpParameter*/, BOOLEAN /*TimerOrWaitFired*/) {
+  Fuzzer::StaticAlarmCallback();
+}
+
+class TimerQ {
+  HANDLE TimerQueue;
+ public:
+  TimerQ() : TimerQueue(NULL) {};
+  ~TimerQ() {
+    if (TimerQueue)
+      DeleteTimerQueueEx(TimerQueue, NULL);
+  };
+  void SetTimer(int Seconds) {
+    if (!TimerQueue) {
+      TimerQueue = CreateTimerQueue();
+      if (!TimerQueue) {
+        Printf("libFuzzer: CreateTimerQueue failed.\n");
+        exit(1);
+      }
+    }
+    HANDLE Timer;
+    if (!CreateTimerQueueTimer(&Timer, TimerQueue, ALRMHandler, NULL,
+        Seconds*1000, Seconds*1000, 0)) {
+      Printf("libFuzzer: CreateTimerQueueTimer failed.\n");
+      exit(1);
+    }
+  };
+};
+
+static TimerQ Timer;
+
 void SetTimer(int Seconds) {
-  // TODO: Complete this implementation.
-  return;
+  Timer.SetTimer(Seconds);
 }
 
 void SetSigSegvHandler() {
   if (!AddVectoredExceptionHandler(1, SEGVHandler)) {
     Printf("libFuzzer: AddVectoredExceptionHandler failed.\n");
     exit(1);
   }
 }

 }
 
 int GetPid() { return GetCurrentProcessId(); }
 
 size_t GetPeakRSSMb() {
   PROCESS_MEMORY_COUNTERS info;
   if (!GetProcessMemoryInfo(GetCurrentProcess(), &info, sizeof(info)))
     return 0;
   return info.PeakWorkingSetSize >> 20;
 }
 
 bool ExecuteCommandAndReadOutput(const std::string &Command, std::string *Out) {
   FILE *Pipe = _popen(Command.c_str(), "r");
   if (!Pipe) return false;
   char Buff[1024];
   size_t N;
   while ((N = fread(Buff, 1, sizeof(Buff), Pipe)) > 0)
     Out->append(Buff, N);