[PATCH] D27240: [libFuzzer] Diff 8 - Improve synchronization.

[Marcos Pividori via Phabricator via llvm-commits]

mpividori added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D27240