From aa96ccfbf319f6e1d60532c8d65e5d7131aea602 Mon Sep 17 00:00:00 2001 From: JF Bastien Date: Wed, 16 May 2018 04:30:00 +0000 Subject: [PATCH] Signal handling should be signal-safe Summary: Before this patch, signal handling wasn't signal safe. This leads to real-world crashes. It used ManagedStatic inside of signals, this can allocate and can lead to unexpected state when a signal occurs during llvm_shutdown (because llvm_shutdown destroys the ManagedStatic). It also used cl::opt without custom backing storage. Some de-allocation was performed as well. Acquiring a lock in a signal handler is also a great way to deadlock. We can't just disable signals on llvm_shutdown because the signals might do useful work during that shutdown. We also can't just disable llvm_shutdown for programs (instead of library uses of clang) because we'd have to then mark the pointers as not leaked and make sure all the ManagedStatic uses are OK to leak and remain so. Move all of the code to lock-free datastructures instead, and avoid having any of them in an inconsistent state. I'm not trying to be fancy, I'm not using any explicit memory order because this code isn't hot. The only purpose of the atomics is to guarantee that a signal firing on the same or a different thread doesn't see an inconsistent state and crash. In some cases we might miss some state (for example, we might fail to delete a temporary file), but that's fine. Note that I haven't touched any of the backtrace support despite it not technically being totally signal-safe. When that code is called we know something bad is up and we don't expect to continue execution, so calling something that e.g. sets errno is the least of our problems. A similar patch should be applied to lib/Support/Windows/Signals.inc, but that can be done separately. Reviewers: dexonsmith Subscribers: aheejin, llvm-commits Differential Revision: https://reviews.llvm.org/D46858 llvm-svn: 332428 --- include/llvm/Support/Signals.h | 4 +- lib/Support/Signals.cpp | 45 ++++-- lib/Support/Unix/Signals.inc | 234 ++++++++++++++++++++++---------- lib/Support/Windows/Signals.inc | 5 +- 4 files changed, 201 insertions(+), 87 deletions(-) diff --git a/include/llvm/Support/Signals.h b/include/llvm/Support/Signals.h index 2b5c3e4012c..f25a0496990 100644 --- a/include/llvm/Support/Signals.h +++ b/include/llvm/Support/Signals.h @@ -56,10 +56,12 @@ namespace sys { // Run all registered signal handlers. void RunSignalHandlers(); + using SignalHandlerCallback = void (*)(void *); + /// Add a function to be called when an abort/kill signal is delivered to the /// process. The handler can have a cookie passed to it to identify what /// instance of the handler it is. - void AddSignalHandler(void (*FnPtr)(void *), void *Cookie); + void AddSignalHandler(SignalHandlerCallback FnPtr, void *Cookie); /// This function registers a function to be called when the user "interrupts" /// the program (typically by pressing ctrl-c). When the user interrupts the diff --git a/lib/Support/Signals.cpp b/lib/Support/Signals.cpp index 414619bc3c9..6270a9bbef5 100644 --- a/lib/Support/Signals.cpp +++ b/lib/Support/Signals.cpp @@ -36,19 +36,42 @@ using namespace llvm; -static cl::opt +// Use explicit storage to avoid accessing cl::opt in a signal handler. +static bool DisableSymbolicationFlag = false; +static cl::opt DisableSymbolication("disable-symbolication", cl::desc("Disable symbolizing crash backtraces."), - cl::init(false), cl::Hidden); + cl::location(DisableSymbolicationFlag), cl::Hidden); -static ManagedStatic>> - CallBacksToRun; -void sys::RunSignalHandlers() { - if (!CallBacksToRun.isConstructed()) - return; - for (auto &I : *CallBacksToRun) - I.first(I.second); - CallBacksToRun->clear(); +// Callbacks to run in signal handler must be lock-free because a signal handler +// could be running as we add new callbacks. We don't add unbounded numbers of +// callbacks, an array is therefore sufficient. +// Assume two pointers are always lock-free. +struct LLVM_ALIGNAS(sizeof(void *) * 2) CallbackAndCookie { + sys::SignalHandlerCallback Callback; + void *Cookie; +}; +static constexpr size_t MaxSignalHandlerCallbacks = 8; +static std::atomic CallBacksToRun[MaxSignalHandlerCallbacks]; + +void sys::RunSignalHandlers() { // Signal-safe. + for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) { + CallbackAndCookie DoNothing{nullptr, nullptr}; + auto Entry = CallBacksToRun[I].exchange(DoNothing); + if (Entry.Callback) + (*Entry.Callback)(Entry.Cookie); + } +} + +static void insertSignalHandler(sys::SignalHandlerCallback FnPtr, + void *Cookie) { // Signal-safe. + CallbackAndCookie Entry = CallbackAndCookie{FnPtr, Cookie}; + for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) { + CallbackAndCookie Expected{nullptr, nullptr}; + if (CallBacksToRun[I].compare_exchange_strong(Expected, Entry)) + return; + } + llvm_unreachable("too many signal callbacks already registered"); } static bool findModulesAndOffsets(void **StackTrace, int Depth, @@ -68,7 +91,7 @@ static FormattedNumber format_ptr(void *PC) { LLVM_ATTRIBUTE_USED static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace, int Depth, llvm::raw_ostream &OS) { - if (DisableSymbolication) + if (DisableSymbolicationFlag) return false; // Don't recursively invoke the llvm-symbolizer binary. diff --git a/lib/Support/Unix/Signals.inc b/lib/Support/Unix/Signals.inc index d828674de49..1f3d7c9012a 100644 --- a/lib/Support/Unix/Signals.inc +++ b/lib/Support/Unix/Signals.inc @@ -11,6 +11,27 @@ // Unix signals occurring while your program is running. // //===----------------------------------------------------------------------===// +// +// This file is extremely careful to only do signal-safe things while in a +// signal handler. In particular, memory allocation and acquiring a mutex +// while in a signal handler should never occur. ManagedStatic isn't usable from +// a signal handler for 2 reasons: +// +// 1. Creating a new one allocates. +// 2. The signal handler could fire while llvm_shutdown is being processed, in +// which case the ManagedStatic is in an unknown state because it could +// already have been destroyed, or be in the process of being destroyed. +// +// Modifying the behavior of the signal handlers (such as registering new ones) +// can acquire a mutex, but all this guarantees is that the signal handler +// behavior is only modified by one thread at a time. A signal handler can still +// fire while this occurs! +// +// Adding work to a signal handler requires lock-freedom (and assume atomics are +// always lock-free) because the signal handler could fire while new work is +// being added. +// +//===----------------------------------------------------------------------===// #include "Unix.h" #include "llvm/ADT/STLExtras.h" @@ -60,12 +81,119 @@ using namespace llvm; static RETSIGTYPE SignalHandler(int Sig); // defined below. -static ManagedStatic > SignalsMutex; - /// The function to call if ctrl-c is pressed. -static void (*InterruptFunction)() = nullptr; +using InterruptFunctionType = void (*)(); +static std::atomic InterruptFunction = + ATOMIC_VAR_INIT(nullptr); -static ManagedStatic> FilesToRemove; +/// Signal-safe removal of files. +/// Inserting and erasing from the list isn't signal-safe, but removal of files +/// themselves is signal-safe. Memory is freed when the head is freed, deletion +/// is therefore not signal-safe either. +class FileToRemoveList { + std::atomic Filename = ATOMIC_VAR_INIT(nullptr); + std::atomic Next = ATOMIC_VAR_INIT(nullptr); + + FileToRemoveList() = default; + // Not signal-safe. + FileToRemoveList(const std::string &str) : Filename(strdup(str.c_str())) {} + +public: + // Not signal-safe. + ~FileToRemoveList() { + if (FileToRemoveList *N = Next.exchange(nullptr)) + delete N; + if (char *F = Filename.exchange(nullptr)) + free(F); + } + + // Not signal-safe. + static void insert(std::atomic &Head, + const std::string &Filename) { + // Insert the new file at the end of the list. + FileToRemoveList *NewHead = new FileToRemoveList(Filename); + std::atomic *InsertionPoint = &Head; + FileToRemoveList *OldHead = nullptr; + while (!InsertionPoint->compare_exchange_strong(OldHead, NewHead)) { + InsertionPoint = &OldHead->Next; + OldHead = nullptr; + } + } + + // Not signal-safe. + static void erase(std::atomic &Head, + const std::string &Filename) { + // Use a lock to avoid concurrent erase: the comparison would access + // free'd memory. + static ManagedStatic> Lock; + sys::SmartScopedLock Writer(*Lock); + + for (FileToRemoveList *Current = Head.load(); Current; + Current = Current->Next.load()) { + if (char *OldFilename = Current->Filename.load()) { + if (OldFilename == Filename) { + // Leave an empty filename. + OldFilename = Current->Filename.exchange(nullptr); + // The filename might have become null between the time we + // compared it and we exchanged it. + if (OldFilename) + free(OldFilename); + } + } + } + } + + // Signal-safe. + static void removeAllFiles(std::atomic &Head) { + // If cleanup were to occur while we're removing files we'd have a bad time. + // Make sure we're OK by preventing cleanup from doing anything while we're + // removing files. If cleanup races with us and we win we'll have a leak, + // but we won't crash. + FileToRemoveList *OldHead = Head.exchange(nullptr); + + for (FileToRemoveList *currentFile = OldHead; currentFile; + currentFile = currentFile->Next.load()) { + // If erasing was occuring while we're trying to remove files we'd look + // at free'd data. Take away the path and put it back when done. + if (char *path = currentFile->Filename.exchange(nullptr)) { + // Get the status so we can determine if it's a file or directory. If we + // can't stat the file, ignore it. + struct stat buf; + if (stat(path, &buf) != 0) + continue; + + // If this is not a regular file, ignore it. We want to prevent removal + // of special files like /dev/null, even if the compiler is being run + // with the super-user permissions. + if (!S_ISREG(buf.st_mode)) + continue; + + // Otherwise, remove the file. We ignore any errors here as there is + // nothing else we can do. + unlink(path); + + // We're done removing the file, erasing can safely proceed. + currentFile->Filename.exchange(path); + } + } + + // We're done removing files, cleanup can safely proceed. + Head.exchange(OldHead); + } +}; +static std::atomic FilesToRemove = ATOMIC_VAR_INIT(nullptr); + +/// Clean up the list in a signal-friendly manner. +/// Recall that signals can fire during llvm_shutdown. If this occurs we should +/// either clean something up or nothing at all, but we shouldn't crash! +struct FilesToRemoveCleanup { + // Not signal-safe. + ~FilesToRemoveCleanup() { + FileToRemoveList *Head = FilesToRemove.exchange(nullptr); + if (Head) + delete Head; + } +}; static StringRef Argv0; @@ -94,13 +222,12 @@ static const int KillSigs[] = { #endif }; -static unsigned NumRegisteredSignals = 0; +static std::atomic NumRegisteredSignals = ATOMIC_VAR_INIT(0); static struct { struct sigaction SA; int SigNo; } RegisteredSignalInfo[array_lengthof(IntSigs) + array_lengthof(KillSigs)]; - #if defined(HAVE_SIGALTSTACK) // Hold onto both the old and new alternate signal stack so that it's not // reported as a leak. We don't make any attempt to remove our alt signal @@ -132,18 +259,24 @@ static void CreateSigAltStack() { static void CreateSigAltStack() {} #endif -static void RegisterHandlers() { - sys::SmartScopedLock Guard(*SignalsMutex); +static void RegisterHandlers() { // Not signal-safe. + // The mutex prevents other threads from registering handlers while we're + // doing it. We also have to protect the handlers and their count because + // a signal handler could fire while we're registeting handlers. + static ManagedStatic> SignalHandlerRegistrationMutex; + sys::SmartScopedLock Guard(*SignalHandlerRegistrationMutex); // If the handlers are already registered, we're done. - if (NumRegisteredSignals != 0) return; + if (NumRegisteredSignals.load() != 0) + return; // Create an alternate stack for signal handling. This is necessary for us to // be able to reliably handle signals due to stack overflow. CreateSigAltStack(); auto registerHandler = [&](int Signal) { - assert(NumRegisteredSignals < array_lengthof(RegisteredSignalInfo) && + unsigned Index = NumRegisteredSignals.load(); + assert(Index < array_lengthof(RegisteredSignalInfo) && "Out of space for signal handlers!"); struct sigaction NewHandler; @@ -153,9 +286,8 @@ static void RegisterHandlers() { sigemptyset(&NewHandler.sa_mask); // Install the new handler, save the old one in RegisteredSignalInfo. - sigaction(Signal, &NewHandler, - &RegisteredSignalInfo[NumRegisteredSignals].SA); - RegisteredSignalInfo[NumRegisteredSignals].SigNo = Signal; + sigaction(Signal, &NewHandler, &RegisteredSignalInfo[Index].SA); + RegisteredSignalInfo[Index].SigNo = Signal; ++NumRegisteredSignals; }; @@ -167,44 +299,16 @@ static void RegisterHandlers() { static void UnregisterHandlers() { // Restore all of the signal handlers to how they were before we showed up. - for (unsigned i = 0, e = NumRegisteredSignals; i != e; ++i) + for (unsigned i = 0, e = NumRegisteredSignals.load(); i != e; ++i) { sigaction(RegisteredSignalInfo[i].SigNo, &RegisteredSignalInfo[i].SA, nullptr); - NumRegisteredSignals = 0; + --NumRegisteredSignals; + } } - -/// Process the FilesToRemove list. This function should be called with the -/// SignalsMutex lock held. NB: This must be an async signal safe function. It -/// cannot allocate or free memory, even in debug builds. +/// Process the FilesToRemove list. static void RemoveFilesToRemove() { - // Avoid constructing ManagedStatic in the signal handler. - // If FilesToRemove is not constructed, there are no files to remove. - if (!FilesToRemove.isConstructed()) - return; - - // We avoid iterators in case of debug iterators that allocate or release - // memory. - std::vector& FilesToRemoveRef = *FilesToRemove; - for (unsigned i = 0, e = FilesToRemoveRef.size(); i != e; ++i) { - const char *path = FilesToRemoveRef[i].c_str(); - - // Get the status so we can determine if it's a file or directory. If we - // can't stat the file, ignore it. - struct stat buf; - if (stat(path, &buf) != 0) - continue; - - // If this is not a regular file, ignore it. We want to prevent removal of - // special files like /dev/null, even if the compiler is being run with the - // super-user permissions. - if (!S_ISREG(buf.st_mode)) - continue; - - // Otherwise, remove the file. We ignore any errors here as there is nothing - // else we can do. - unlink(path); - } + FileToRemoveList::removeAllFiles(FilesToRemove); } // The signal handler that runs. @@ -221,20 +325,13 @@ static RETSIGTYPE SignalHandler(int Sig) { sigprocmask(SIG_UNBLOCK, &SigMask, nullptr); { - unique_lock> Guard(*SignalsMutex); RemoveFilesToRemove(); if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig) != std::end(IntSigs)) { - if (InterruptFunction) { - void (*IF)() = InterruptFunction; - Guard.unlock(); - InterruptFunction = nullptr; - IF(); // run the interrupt function. - return; - } + if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr)) + return OldInterruptFunction(); - Guard.unlock(); raise(Sig); // Execute the default handler. return; } @@ -254,45 +351,36 @@ static RETSIGTYPE SignalHandler(int Sig) { } void llvm::sys::RunInterruptHandlers() { - sys::SmartScopedLock Guard(*SignalsMutex); RemoveFilesToRemove(); } void llvm::sys::SetInterruptFunction(void (*IF)()) { - { - sys::SmartScopedLock Guard(*SignalsMutex); - InterruptFunction = IF; - } + InterruptFunction.exchange(IF); RegisterHandlers(); } // The public API bool llvm::sys::RemoveFileOnSignal(StringRef Filename, std::string* ErrMsg) { - { - sys::SmartScopedLock Guard(*SignalsMutex); - FilesToRemove->push_back(Filename); - } - + // Ensure that cleanup will occur as soon as one file is added. + static ManagedStatic FilesToRemoveCleanup; + *FilesToRemoveCleanup; + FileToRemoveList::insert(FilesToRemove, Filename.str()); RegisterHandlers(); return false; } // The public API void llvm::sys::DontRemoveFileOnSignal(StringRef Filename) { - sys::SmartScopedLock Guard(*SignalsMutex); - std::vector::reverse_iterator RI = - find(reverse(*FilesToRemove), Filename); - std::vector::iterator I = FilesToRemove->end(); - if (RI != FilesToRemove->rend()) - I = FilesToRemove->erase(RI.base()-1); + FileToRemoveList::erase(FilesToRemove, Filename.str()); } /// Add a function to be called when a signal is delivered to the process. The /// handler can have a cookie passed to it to identify what instance of the /// handler it is. -void llvm::sys::AddSignalHandler(void (*FnPtr)(void *), void *Cookie) { - CallBacksToRun->push_back(std::make_pair(FnPtr, Cookie)); +void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr, + void *Cookie) { // Signal-safe. + insertSignalHandler(FnPtr, Cookie); RegisterHandlers(); } diff --git a/lib/Support/Windows/Signals.inc b/lib/Support/Windows/Signals.inc index 959376b52f7..cc225d9824d 100644 --- a/lib/Support/Windows/Signals.inc +++ b/lib/Support/Windows/Signals.inc @@ -560,8 +560,9 @@ void llvm::sys::SetInterruptFunction(void (*IF)()) { /// Add a function to be called when a signal is delivered to the process. The /// handler can have a cookie passed to it to identify what instance of the /// handler it is. -void llvm::sys::AddSignalHandler(void (*FnPtr)(void *), void *Cookie) { - CallBacksToRun->push_back(std::make_pair(FnPtr, Cookie)); +void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr, + void *Cookie) { + insertSignalHandler(FnPtr, Cookie); RegisterHandler(); LeaveCriticalSection(&CriticalSection); }