[llvm-commits] [llvm] r158580 - /llvm/trunk/lib/Support/Unix/Signals.inc

[Matt Beaumont-Gay]

On Fri, Jun 15, 2012 at 5:09 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> Author: chandlerc
> Date: Fri Jun 15 19:09:41 2012
> New Revision: 158580
>
> URL: http://llvm.org/viewvc/llvm-project?rev=158580&view=rev
> Log:
> 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....
>
> Modified:
>    llvm/trunk/lib/Support/Unix/Signals.inc
>
> Modified: llvm/trunk/lib/Support/Unix/Signals.inc
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Signals.inc?rev=158580&r1=158579&r2=158580&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/Unix/Signals.inc (original)
> +++ llvm/trunk/lib/Support/Unix/Signals.inc Fri Jun 15 19:09:41 2012
> @@ -15,6 +15,7 @@
>  #include "Unix.h"
>  #include "llvm/ADT/STLExtras.h"
>  #include "llvm/Support/Mutex.h"
> +#include <string>
>  #include <vector>
>  #include <algorithm>
>  #if HAVE_EXECINFO_H
> @@ -43,7 +44,7 @@
>  /// InterruptFunction - The function to call if ctrl-c is pressed.
>  static void (*InterruptFunction)() = 0;
>
> -static std::vector<sys::Path> FilesToRemove;
> +static std::vector<std::string> FilesToRemove;
>  static std::vector<std::pair<void(*)(void*), void*> > CallBacksToRun;
>
>  // IntSigs - Signals that may interrupt the program at any time.
> @@ -117,10 +118,20 @@
>
>  /// 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 @@
>  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();

Cast to void to avoid -Wunused-expression?

> +  else
> +    for (unsigned i = 0, e = FilesToRemove.size(); i != e; ++i)
> +      FilesToRemove[i].c_str();
>
>   SignalsMutex.release();
>
> @@ -189,10 +212,19 @@
>  // DontRemoveFileOnSignal - The public API
>  void llvm::sys::DontRemoveFileOnSignal(const sys::Path &Filename) {
>   SignalsMutex.acquire();
> -  std::vector<sys::Path>::reverse_iterator I =
> -    std::find(FilesToRemove.rbegin(), FilesToRemove.rend(), Filename);
> -  if (I != FilesToRemove.rend())
> -    FilesToRemove.erase(I.base()-1);
> +  std::vector<std::string>::reverse_iterator RI =
> +    std::find(FilesToRemove.rbegin(), FilesToRemove.rend(), Filename.str());
> +  std::vector<std::string>::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<std::string>::iterator E = FilesToRemove.end(); I != E; ++I)
> +    I->c_str();
> +
>   SignalsMutex.release();
>  }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits