GTTF: Make Linux stack dump signal handler async-signal safe.

base/debug/stack_trace_posix.cc

Index: base/debug/stack_trace_posix.cc
diff --git a/base/debug/stack_trace_posix.cc b/base/debug/stack_trace_posix.cc
index ff756fc0eece78622d703162432726d517b198fa..3d7fb6fd5f95418638a4342ae626543082c44016 100644
--- a/base/debug/stack_trace_posix.cc
+++ b/base/debug/stack_trace_posix.cc
@@ -15,24 +15,13 @@
 #include <sys/types.h>
 #include <unistd.h>
 
-#include <string>
-#include <vector>
-
-#if defined(__GLIBCXX__)
-#include <cxxabi.h>
-#endif
-
-#if defined(OS_MACOSX)
-#include <AvailabilityMacros.h>
-#endif
+#include <ostream>
 
 #include "base/basictypes.h"
+#include "base/debug/debugger.h"
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/safe_strerror_posix.h"
-#include "base/string_piece.h"
-#include "base/stringprintf.h"
+#include "base/string_number_conversions.h"
 
 #if defined(USE_SYMBOLIZE)
 #include "base/third_party/symbolize/symbolize.h"
@@ -43,124 +32,63 @@ namespace debug {
 
 namespace {
 
-// The prefix used for mangled symbols, per the Itanium C++ ABI:
-// http://www.codesourcery.com/cxx-abi/abi.html#mangling
-const char kMangledSymbolPrefix[] = "_Z";
-
-// Characters that can be used for symbols, generated by Ruby:
-// (('a'..'z').to_a+('A'..'Z').to_a+('0'..'9').to_a + ['_']).join
-const char kSymbolCharacters[] =
-    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_";
-
-#if !defined(USE_SYMBOLIZE)
-// Demangles C++ symbols in the given text. Example:
-//
-// "out/Debug/base_unittests(_ZN10StackTraceC1Ev+0x20) [0x817778c]"
-// =>
-// "out/Debug/base_unittests(StackTrace::StackTrace()+0x20) [0x817778c]"
-void DemangleSymbols(std::string* text) {
-#if defined(__GLIBCXX__)
-
-  std::string::size_type search_from = 0;
-  while (search_from < text->size()) {
-    // Look for the start of a mangled symbol, from search_from.
-    std::string::size_type mangled_start =
-        text->find(kMangledSymbolPrefix, search_from);
-    if (mangled_start == std::string::npos) {
-      break;  // Mangled symbol not found.
-    }
+class BacktraceOutputHandler {
+ public:
+  virtual void HandleOutput(const char* output) = 0;
 
-    // Look for the end of the mangled symbol.
-    std::string::size_type mangled_end =
-        text->find_first_not_of(kSymbolCharacters, mangled_start);
-    if (mangled_end == std::string::npos) {
-      mangled_end = text->size();
-    }
-    std::string mangled_symbol =
-        text->substr(mangled_start, mangled_end - mangled_start);
-
-    // Try to demangle the mangled symbol candidate.
-    int status = 0;
-    scoped_ptr_malloc<char> demangled_symbol(
-        abi::__cxa_demangle(mangled_symbol.c_str(), NULL, 0, &status));
-    if (status == 0) {  // Demangling is successful.
-      // Remove the mangled symbol.
-      text->erase(mangled_start, mangled_end - mangled_start);
-      // Insert the demangled symbol.
-      text->insert(mangled_start, demangled_symbol.get());
-      // Next time, we'll start right after the demangled symbol we inserted.
-      search_from = mangled_start + strlen(demangled_symbol.get());
-    } else {
-      // Failed to demangle.  Retry after the "_Z" we just found.
-      search_from = mangled_start + 2;
-    }
-  }
+ protected:
+  virtual ~BacktraceOutputHandler() {}
+};
 
-#endif  // defined(__GLIBCXX__)
-}
-#endif  // !defined(USE_SYMBOLIZE)
+void ProcessBacktrace(void *const *trace,
+                      int size,
+                      BacktraceOutputHandler* handler) {
+  // NOTE: This code MUST be async-signal safe (it's used by in-process
+  // stack dumping signal handler). NO malloc or stdio is allowed here.
+
+  for (int i = 0; i < size; ++i) {
+    handler->HandleOutput("\t");
 
-// Gets the backtrace as a vector of strings. If possible, resolve symbol
-// names and attach these. Otherwise just use raw addresses. Returns true
-// if any symbol name is resolved.  Returns false on error and *may* fill
-// in |error_message| if an error message is available.
-bool GetBacktraceStrings(void *const *trace, int size,
-                         std::vector<std::string>* trace_strings,
-                         std::string* error_message) {
-  bool symbolized = false;
+    char buf[1024] = { '\0' };
 
 #if defined(USE_SYMBOLIZE)
-  for (int i = 0; i < size; ++i) {
-    char symbol[1024];
     // Subtract by one as return address of function may be in the next
     // function when a function is annotated as noreturn.
-    if (google::Symbolize(static_cast<char *>(trace[i]) - 1,
-                          symbol, sizeof(symbol))) {
-      // Don't call DemangleSymbols() here as the symbol is demangled by
-      // google::Symbolize().
-      trace_strings->push_back(
-          base::StringPrintf("%s [%p]", symbol, trace[i]));
-      symbolized = true;
-    } else {
-      trace_strings->push_back(base::StringPrintf("%p", trace[i]));
-    }
-  }
-#else
-  scoped_ptr_malloc<char*> trace_symbols(backtrace_symbols(trace, size));
-  if (trace_symbols.get()) {
-    for (int i = 0; i < size; ++i) {
-      std::string trace_symbol = trace_symbols.get()[i];
-      DemangleSymbols(&trace_symbol);
-      trace_strings->push_back(trace_symbol);
-    }
-    symbolized = true;
-  } else {
-    if (error_message)
-      *error_message = safe_strerror(errno);
-    for (int i = 0; i < size; ++i) {
-      trace_strings->push_back(base::StringPrintf("%p", trace[i]));
-    }
-  }
+    void* address = static_cast<char*>(trace[i]) - 1;
+    if (google::Symbolize(address, buf, sizeof(buf)))
+      handler->HandleOutput(buf);
+    else
+      handler->HandleOutput("<unknown>");
+
+    handler->HandleOutput(" ");
 #endif  // defined(USE_SYMBOLIZE)
 
-  return symbolized;
+    handler->HandleOutput("[0x");
+    internal::itoa_r(reinterpret_cast<intptr_t>(trace[i]),
+                     buf, sizeof(buf), 16);
+    handler->HandleOutput(buf);
+    handler->HandleOutput("]\n");
+  }
 }
 
 void StackDumpSignalHandler(int signal, siginfo_t* info, ucontext_t* context) {
+  // NOTE: This code MUST be async-signal safe.
+  // NO malloc or stdio is allowed here.
+
   if (BeingDebugged())
     BreakDebugger();
 
-#if defined(OS_MACOSX)
-  // TODO(phajdan.jr): Fix async-signal non-safety (http://crbug.com/101155).
-  DLOG(ERROR) << "Received signal " << signal;
-  StackTrace().PrintBacktrace();
-#endif
+  char buf[1024] = { '\0' };
+  strncat(buf, "Received signal ", sizeof(buf) - 1);
+  internal::itoa_r(signal, buf + strlen(buf), sizeof(buf) - strlen(buf), 10);
+  RAW_LOG(ERROR, buf);
+
+  debug::StackTrace().PrintBacktrace();
 
   // TODO(shess): Port to Linux.
 #if defined(OS_MACOSX)
   // TODO(shess): Port to 64-bit.
 #if ARCH_CPU_X86_FAMILY && ARCH_CPU_32_BITS
-  char buf[1024];
   size_t len;
 
   // NOTE: Even |snprintf()| is not on the approved list for signal
@@ -201,6 +129,68 @@ void StackDumpSignalHandler(int signal, siginfo_t* info, ucontext_t* context) {
   _exit(1);
 }
 
+class PrintBacktraceOutputHandler : public BacktraceOutputHandler {
+ public:
+  PrintBacktraceOutputHandler() {}
+
+  virtual void HandleOutput(const char* output) {
+    // NOTE: This code MUST be async-signal safe (it's used by in-process
+    // stack dumping signal handler). NO malloc or stdio is allowed here.
+    HANDLE_EINTR(write(STDERR_FILENO, output, strlen(output)));
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PrintBacktraceOutputHandler);
+};
+
+class StreamBacktraceOutputHandler : public BacktraceOutputHandler {
+ public:
+  StreamBacktraceOutputHandler(std::ostream* os) : os_(os) {
+  }
+
+  virtual void HandleOutput(const char* output) {
+    (*os_) << output;
+  }
+
+ private:
+  std::ostream* os_;
+
+  DISALLOW_COPY_AND_ASSIGN(StreamBacktraceOutputHandler);
+};
+
+void WarmUpBacktrace() {
+  // Warm up stack trace infrastructure. It turns out that on the first
+  // call glibc initializes some internal data structures using pthread_once,
+  // and even backtrace() can call malloc(), leading to hangs.
+  //
+  // Example stack trace snippet (with tcmalloc):
+  //
+  // #8  0x0000000000a173b5 in tc_malloc
+  //             at ./third_party/tcmalloc/chromium/src/debugallocation.cc:1161
+  // #9  0x00007ffff7de7900 in _dl_map_object_deps at dl-deps.c:517
+  // #10 0x00007ffff7ded8a9 in dl_open_worker at dl-open.c:262
+  // #11 0x00007ffff7de9176 in _dl_catch_error at dl-error.c:178
+  // #12 0x00007ffff7ded31a in _dl_open (file=0x7ffff625e298 "libgcc_s.so.1")
+  //             at dl-open.c:639
+  // #13 0x00007ffff6215602 in do_dlopen at dl-libc.c:89
+  // #14 0x00007ffff7de9176 in _dl_catch_error at dl-error.c:178
+  // #15 0x00007ffff62156c4 in dlerror_run at dl-libc.c:48
+  // #16 __GI___libc_dlopen_mode at dl-libc.c:165
+  // #17 0x00007ffff61ef8f5 in init
+  //             at ../sysdeps/x86_64/../ia64/backtrace.c:53
+  // #18 0x00007ffff6aad400 in pthread_once
+  //             at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_once.S:104
+  // #19 0x00007ffff61efa14 in __GI___backtrace
+  //             at ../sysdeps/x86_64/../ia64/backtrace.c:104
+  // #20 0x0000000000752a54 in base::debug::StackTrace::StackTrace
+  //             at base/debug/stack_trace_posix.cc:175
+  // #21 0x00000000007a4ae5 in
+  //             base::(anonymous namespace)::StackDumpSignalHandler
+  //             at base/process_util_posix.cc:172
+  // #22 <signal handler called>
+  StackTrace stack_trace;
+}
+
 }  // namespace
 
 #if !defined(OS_IOS)
@@ -214,6 +204,9 @@ bool EnableInProcessStackDumping() {
   sigemptyset(&action.sa_mask);
   bool success = (sigaction(SIGPIPE, &action, NULL) == 0);
 
+  // Avoid hangs during backtrace initialization, see above.
+  WarmUpBacktrace();
+
   sig_t handler = reinterpret_cast<sig_t>(&StackDumpSignalHandler);
   success &= (signal(SIGILL, handler) != SIG_ERR);
   success &= (signal(SIGABRT, handler) != SIG_ERR);
@@ -227,36 +220,81 @@ bool EnableInProcessStackDumping() {
 #endif  // !defined(OS_IOS)
 
 StackTrace::StackTrace() {
+  // NOTE: This code MUST be async-signal safe (it's used by in-process
+  // stack dumping signal handler). NO malloc or stdio is allowed here.
+
   // Though the backtrace API man page does not list any possible negative
   // return values, we take no chance.
   count_ = std::max(backtrace(trace_, arraysize(trace_)), 0);
 }
 
 void StackTrace::PrintBacktrace() const {
-  fflush(stderr);
-  std::vector<std::string> trace_strings;
-  GetBacktraceStrings(trace_, count_, &trace_strings, NULL);
-  for (size_t i = 0; i < trace_strings.size(); ++i) {
-    fprintf(stderr, "\t%s\n", trace_strings[i].c_str());
-  }
+  // NOTE: This code MUST be async-signal safe (it's used by in-process
+  // stack dumping signal handler). NO malloc or stdio is allowed here.
+
+  PrintBacktraceOutputHandler handler;
+  ProcessBacktrace(trace_, count_, &handler);
 }
 
 void StackTrace::OutputToStream(std::ostream* os) const {
-  std::vector<std::string> trace_strings;
-  std::string error_message;
-  if (GetBacktraceStrings(trace_, count_, &trace_strings, &error_message)) {
-    (*os) << "Backtrace:\n";
-  } else {
-    if (!error_message.empty())
-      error_message = " (" + error_message + ")";
-    (*os) << "Unable to get symbols for backtrace" << error_message << ". "
-          << "Dumping raw addresses in trace:\n";
+  StreamBacktraceOutputHandler handler(os);
+  ProcessBacktrace(trace_, count_, &handler);
+}
+
+namespace internal {

[Scott Hess - ex-Googler, 2012/11/07 01:02:26]

itoa() is defined as handling bases between 2 and 36.  This only handles bases
between 2 and 16.  At minimum you need to fail if base is outside the range
[2,16].

I think it would be reasonable to only allow bases 10 and 16, honestly.

+
+// NOTE: code from sandbox/linux/seccomp-bpf/demo.cc.
+char *itoa_r(intptr_t i, char *buf, size_t sz, int base) {

[Scott Hess - ex-Googler, 2012/11/07 01:02:26]

You might consider just requiring sz to be able to contain enough space to
accomodate sizeof(i)*8+1 (largest 2-bit output plus nul terminator).  Adjust
accordingly if you decide to restrict to bases 10 and 16.

+  // Make sure we can write at least one NUL byte.
+  size_t n = 1;
+  if (n > sz)
+    return NULL;
+
+  char *start = buf;
+
+  // Handle negative numbers.
+  if (i < 0) {

[jar (doing other things), 2012/11/07 00:32:07]

This should only be handled if base is 10.

[Scott Hess - ex-Googler, 2012/11/07 01:02:26]

As currently used, you could just push the negative case into the caller.

Actually, I'm not entirely clear whether the negative case actually happens in
practice for us.

+    // Make sure we can write the '-' character.
+    if (++n > sz) {
+      *start = '\000';
+      return NULL;
+    }
+    *start++ = '-';
   }
 
-  for (size_t i = 0; i < trace_strings.size(); ++i) {
-    (*os) << "\t" << trace_strings[i] << "\n";
+  uintptr_t j = (i > 0) ? i : -i;

[jar (doing other things), 2012/11/07 00:32:07]

The fixup for signed conversion should only be done if this is base 10.

+
+  // Loop until we have converted the entire number. Output at least one
+  // character (i.e. '0').
+  char *ptr = start;
+  do {
+    // Make sure there is still enough space left in our output buffer.
+    if (++n > sz) {
+      buf = NULL;

[Scott Hess - ex-Googler, 2012/11/07 01:02:26]

I think this would make more sense as return NULL, or the same thing as the case
above where it also sets *buf='\000'.  As-is, it's stuffing a truncated
representation of the low-order bits into the output buffer.

+      break;

[Scott Hess - ex-Googler, 2012/11/07 01:02:26]

I think this should just return NULL, or *buf='\000' and return NULL.  If
someone depends on the nul-terminated low-order digits to be in the buffer,
that's weird.

+    }
+
+    // Output the next digit.
+    *ptr++ = "0123456789abcdef"[j % base];
+    j /= base;
+  } while (j);
+
+  // Terminate the output with a NUL character.
+  *ptr = '\000';
+
+  // Conversion to ASCII actually resulted in the digits being in reverse
+  // order. We can't easily generate them in forward order, as we can't tell
+  // the number of characters needed until we are done converting.
+  // So, now, we reverse the string (except for the possible "-" sign).
+  while (--ptr > start) {
+    char ch = *ptr;
+    *ptr = *start;
+    *start++ = ch;
   }
+  return buf;
 }
 
+}  // namespace internal
+
 }  // namespace debug
 }  // namespace base