From 0b8b3ba21ee7c1a16ac844965dbace820b263bc0 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sat, 16 Jun 2012 00:09:41 +0000 Subject: [PATCH] Harden the Unix signals code to be more async signal safe. This is likely only the tip of the ice berg, but this particular bug caused any double-free on a glibc system to turn into a deadlock! It is not generally safe to either allocate or release heap memory from within the signal handler. The 'pop_back()' in RemoveFilesToRemove was deleting memory and causing the deadlock. What's worse, eraseFromDisk in PathV1 has lots of allocation and deallocation paths. We even passed 'true' in a place that would have caused the *signal handler* to try to run the 'system' system call and shell out to 'rm -rf'. That was never going to work... This patch switches the file removal to use a vector of strings so that the exact text needed for the 'unlink' system call can be stored there. It switches the loop to be a boring indexed loop, and directly calls unlink without looking at the error. It also works quite hard to ensure that calling 'c_str()' is safe, by ensuring that the non-signal-handling code path that manipulates the vector always leaves it in a state where every element has already had 'c_str()' called at least once. I dunno exactly how overkill this is, but it fixes the deadlock-on-double free issue, and seems likely to prevent any other issues from sneaking up. Sorry for not having a test case, but I *really* don't know how to test signal handling code easily.... git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@158580 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Support/Unix/Signals.inc | 50 +++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/lib/Support/Unix/Signals.inc b/lib/Support/Unix/Signals.inc index c9ec9fce9aa..b2e5fd8b0a5 100644 --- a/lib/Support/Unix/Signals.inc +++ b/lib/Support/Unix/Signals.inc @@ -15,6 +15,7 @@ #include "Unix.h" #include "llvm/ADT/STLExtras.h" #include "llvm/Support/Mutex.h" +#include #include #include #if HAVE_EXECINFO_H @@ -43,7 +44,7 @@ static SmartMutex SignalsMutex; /// InterruptFunction - The function to call if ctrl-c is pressed. static void (*InterruptFunction)() = 0; -static std::vector FilesToRemove; +static std::vector FilesToRemove; static std::vector > CallBacksToRun; // IntSigs - Signals that may interrupt the program at any time. @@ -117,10 +118,20 @@ static void UnregisterHandlers() { /// RemoveFilesToRemove - 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. static void RemoveFilesToRemove() { - while (!FilesToRemove.empty()) { - FilesToRemove.back().eraseFromDisk(true); - FilesToRemove.pop_back(); + // Note: avoid iterators in case of debug iterators that allocate or release + // memory. + for (unsigned i = 0, e = FilesToRemove.size(); i != e; ++i) { + // Note that we don't want to use any external code here, and we don't care + // about errors. We're going to try as hard as we can as often as we need + // to to make these files go away. If these aren't files, too bad. + // + // We do however rely on a std::string implementation for which repeated + // calls to 'c_str()' don't allocate memory. We pre-call 'c_str()' on all + // of these strings to try to ensure this is safe. + unlink(FilesToRemove[i].c_str()); } } @@ -178,7 +189,19 @@ void llvm::sys::SetInterruptFunction(void (*IF)()) { bool llvm::sys::RemoveFileOnSignal(const sys::Path &Filename, std::string* ErrMsg) { SignalsMutex.acquire(); - FilesToRemove.push_back(Filename); + std::string *OldPtr = &FilesToRemove[0]; + FilesToRemove.push_back(Filename.str()); + + // We want to call 'c_str()' on every std::string in this vector so that if + // the underlying implementation requires a re-allocation, it happens here + // rather than inside of the signal handler. If we see the vector grow, we + // have to call it on every entry. If it remains in place, we only need to + // call it on the latest one. + if (OldPtr == &FilesToRemove[0]) + FilesToRemove.back().c_str(); + else + for (unsigned i = 0, e = FilesToRemove.size(); i != e; ++i) + FilesToRemove[i].c_str(); SignalsMutex.release(); @@ -189,10 +212,19 @@ bool llvm::sys::RemoveFileOnSignal(const sys::Path &Filename, // DontRemoveFileOnSignal - The public API void llvm::sys::DontRemoveFileOnSignal(const sys::Path &Filename) { SignalsMutex.acquire(); - std::vector::reverse_iterator I = - std::find(FilesToRemove.rbegin(), FilesToRemove.rend(), Filename); - if (I != FilesToRemove.rend()) - FilesToRemove.erase(I.base()-1); + std::vector::reverse_iterator RI = + std::find(FilesToRemove.rbegin(), FilesToRemove.rend(), Filename.str()); + std::vector::iterator I = FilesToRemove.end(); + if (RI != FilesToRemove.rend()) + I = FilesToRemove.erase(RI.base()-1); + + // We need to call c_str() on every element which would have been moved by + // the erase. These elements, in a C++98 implementation where c_str() + // requires a reallocation on the first call may have had the call to c_str() + // made on insertion become invalid by being copied down an element. + for (std::vector::iterator E = FilesToRemove.end(); I != E; ++I) + I->c_str(); + SignalsMutex.release(); }