This is an archive of the discontinued LLVM Phabricator instance.

LibFuzzer - Implement timers for Windows and improve synchronization.
AbandonedPublic

Authored by mpividori on Nov 28 2016, 4:44 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 79472.Nov 28 2016, 4:44 PM
mpividori retitled this revision from to LibFuzzer - Implement timers for Windows and improve synchronization..
mpividori updated this object.
mpividori added a reviewer: zturner.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
mpividori updated this revision to Diff 79592.Nov 29 2016, 9:31 AM

+ 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 updated this revision to Diff 79594.Nov 29 2016, 9:41 AM

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

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

zturner added inline comments.Nov 29 2016, 9:54 AM
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 added inline comments.Nov 29 2016, 10:18 AM
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 added inline comments.Nov 29 2016, 10:30 AM
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 added inline comments.Nov 29 2016, 11:05 AM
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 added inline comments.Nov 29 2016, 11:15 AM
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 added inline comments.Nov 29 2016, 11:29 AM
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 added inline comments.Nov 29 2016, 11:37 AM
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 updated this revision to Diff 79621.Nov 29 2016, 12:27 PM
mpividori edited edge metadata.

+ Initialize inline the members of SignalHandlerOptions.

amccarth edited edge metadata.Nov 29 2016, 1:47 PM

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 added inline comments.Nov 29 2016, 2:19 PM
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 abandoned this revision.Nov 29 2016, 5:28 PM