Issue 182: linux handler doesn't have sigcontext if called from a previous signal handler. r=mento / Liu Li

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@188 4c0a9323-5329-0410-9bdc-e9ce6186880e

7 files changed, 87 insertions, 25 deletions

diff --git a/src/client/linux/handler/exception_handler.cc b/src/client/linux/handler/exception_handler.cc
index eb7cbce2..2036dd05 100644
--- a/src/client/linux/handler/exception_handler.cc
+++ b/src/client/linux/handler/exception_handler.cc
@@ -118,7 +118,7 @@ ExceptionHandler::~ExceptionHandler() {
 }
 
 bool ExceptionHandler::WriteMinidump() {
-  return InternalWriteMinidump(0, NULL);
+  return InternalWriteMinidump(0, 0, NULL);
 }
 
 // static
@@ -127,7 +127,7 @@ bool ExceptionHandler::WriteMinidump(const string &dump_path,
                    void *callback_context) {
   ExceptionHandler handler(dump_path, NULL, callback,
                            callback_context, false);
-  return handler.InternalWriteMinidump(0, NULL);
+  return handler.InternalWriteMinidump(0, 0, NULL);
 }
 
 void ExceptionHandler::SetupHandler() {
@@ -176,8 +176,15 @@ void ExceptionHandler::HandleException(int signo) {
   // the signal handler frame as value parameter. For some reasons, the
   // prototype of the handler doesn't declare this information as parameter, we
   // will do it by hand. It is the second parameter above the signal number.
-  const struct sigcontext *sig_ctx =
-    reinterpret_cast<const struct sigcontext *>(&signo + 1);
+  // However, if we are being called by another signal handler passing the
+  // signal up the chain, then we may not have this random extra parameter,
+  // so we may have to walk the stack to find it.  We do the actual work
+  // on another thread, where it's a little safer, but we want the ebp
+  // from this frame to find it.
+  uintptr_t current_ebp = 0;
+  asm volatile ("movl %%ebp, %0"
+                :"=m"(current_ebp));
+
   pthread_mutex_lock(&handler_stack_mutex_);
   ExceptionHandler *current_handler =
     handler_stack_->at(handler_stack_->size() - ++handler_stack_index_);
@@ -185,7 +192,9 @@ void ExceptionHandler::HandleException(int signo) {
 
   // Restore original handler.
   current_handler->TeardownHandler(signo);
-  if (current_handler->InternalWriteMinidump(signo, sig_ctx)) {
+
+  struct sigcontext *sig_ctx = NULL;
+  if (current_handler->InternalWriteMinidump(signo, current_ebp, &sig_ctx)) {
     // Fully handled this exception, safe to exit.
     exit(EXIT_FAILURE);
   } else {
@@ -194,7 +203,7 @@ void ExceptionHandler::HandleException(int signo) {
     typedef void (*SignalHandler)(int signo, struct sigcontext);
     SignalHandler old_handler =
       reinterpret_cast<SignalHandler>(current_handler->old_handlers_[signo]);
-    if (old_handler != NULL)
+    if (old_handler != NULL && sig_ctx != NULL)
       old_handler(signo, *sig_ctx);
   }
 
@@ -212,7 +221,8 @@ void ExceptionHandler::HandleException(int signo) {
 }
 
 bool ExceptionHandler::InternalWriteMinidump(int signo,
-                                    const struct sigcontext *sig_ctx) {
+                                             uintptr_t sighandler_ebp,
+                                             struct sigcontext **sig_ctx) {
   if (filter_ && !filter_(callback_context_))
     return false;
 
@@ -239,7 +249,7 @@ bool ExceptionHandler::InternalWriteMinidump(int signo,
     }
 
     success = minidump_generator_.WriteMinidumpToFile(
-        minidump_path, signo, sig_ctx);
+                       minidump_path, signo, sighandler_ebp, sig_ctx);
 
     // Unblock the signals.
     if (blocked) {
diff --git a/src/client/linux/handler/exception_handler.h b/src/client/linux/handler/exception_handler.h
index 0c0a2e83..2a212315 100644
--- a/src/client/linux/handler/exception_handler.h
+++ b/src/client/linux/handler/exception_handler.h
@@ -132,7 +132,7 @@ class ExceptionHandler {
   // execution state independently of a crash.  Returns true on success.
   bool WriteMinidump();
 
-    // Convenience form of WriteMinidump which does not require an
+  // Convenience form of WriteMinidump which does not require an
   // ExceptionHandler instance.
   static bool WriteMinidump(const string &dump_path,
                             MinidumpCallback callback,
@@ -151,7 +151,14 @@ class ExceptionHandler {
   // Signal handler.
   static void HandleException(int signo);
 
-  bool InternalWriteMinidump(int signo, const struct sigcontext *sig_ctx);
+  // If called from a signal handler, sighandler_ebp is the ebp of
+  // that signal handler's frame, and sig_ctx is an out parameter
+  // that will be set to point at the sigcontext that was placed
+  // on the stack by the kernel.  You can pass zero and NULL
+  // for the second and third parameters if you are not calling
+  // this from a signal handler.
+  bool InternalWriteMinidump(int signo, uintptr_t sighandler_ebp,
+                             struct sigcontext **sig_ctx);
 
  private:
   FilterCallback filter_;
diff --git a/src/client/linux/handler/linux_thread.cc b/src/client/linux/handler/linux_thread.cc
index 1bd45b58..2fe103e4 100644
--- a/src/client/linux/handler/linux_thread.cc
+++ b/src/client/linux/handler/linux_thread.cc
@@ -381,4 +381,30 @@ bool LinuxThread::IsAddressMapped(uintptr_t address) const {
   return addr.is_mapped;
 }
 
+bool LinuxThread::FindSigContext(uintptr_t sighandler_ebp,
+                                 struct sigcontext **sig_ctx) {
+  uintptr_t previous_ebp;
+  const int MAX_STACK_DEPTH = 10;
+  int depth_counter = 0;
+
+  do {
+    // We're looking for a |struct sigcontext| as the second parameter
+    // to a signal handler function call.  Luckily, the sigcontext
+    // has an ebp member which should match the ebp pointed to
+    // by the ebp of the signal handler frame.
+    previous_ebp = reinterpret_cast<uintptr_t>(GetNextFrame(
+                                  reinterpret_cast<void**>(sighandler_ebp)));
+    // The stack looks like this:
+    // | previous ebp | previous eip | first param | second param |,
+    // so we need to offset by 3 to get to the second parameter.
+    *sig_ctx = reinterpret_cast<struct sigcontext*>(sighandler_ebp +
+                                                    3 * sizeof(uintptr_t));
+    sighandler_ebp = previous_ebp;
+    depth_counter++;
+  } while(previous_ebp != (*sig_ctx)->ebp && sighandler_ebp != 0 &&
+          IsAddressMapped(sighandler_ebp) && depth_counter < MAX_STACK_DEPTH);
+
+  return previous_ebp == (*sig_ctx)->ebp && previous_ebp != 0;
+}
+
 }  // namespace google_breakpad
diff --git a/src/client/linux/handler/linux_thread.h b/src/client/linux/handler/linux_thread.h
index 9f5d479f..f738c2e0 100644
--- a/src/client/linux/handler/linux_thread.h
+++ b/src/client/linux/handler/linux_thread.h
@@ -171,7 +171,10 @@ class LinuxThread {
   int ListModules(CallbackParam<ModuleCallback> *callback_param) const;
 
   // Get the bottom of the stack from ebp.
-  uintptr_t GetThreadStackBottom(uintptr_t current_esp) const;
+  uintptr_t GetThreadStackBottom(uintptr_t current_ebp) const;
+
+  // Finds a sigcontext on the stack given the ebp of our signal handler.
+  bool FindSigContext(uintptr_t sighandler_ebp, struct sigcontext **sig_ctx);
 
  private:
   // This callback will run when a new thread has been found.
diff --git a/src/client/linux/handler/minidump_generator.cc b/src/client/linux/handler/minidump_generator.cc
index b212d44f..ea1b54a1 100644
--- a/src/client/linux/handler/minidump_generator.cc
+++ b/src/client/linux/handler/minidump_generator.cc
@@ -78,8 +78,14 @@ struct WriterArgument {
   // Signal number when crash happed. Can be 0 if this is a requested dump.
   int signo;
 
-  // Signal contex when crash happed. Can be NULL if this is a requested dump.
-  const struct sigcontext *sig_ctx;
+  // The ebp of the signal handler frame.  Can be zero if this
+  // is a requested dump.
+  uintptr_t sighandler_ebp;
+
+  // Signal context when crash happed. Can be NULL if this is a requested dump.
+  // This is actually an out parameter, but it will be filled in at the start
+  // of the writer thread.
+  struct sigcontext *sig_ctx;
 
   // Used to get information about the threads.
   LinuxThread *thread_lister;
@@ -272,10 +278,10 @@ bool WriteCrashedThreadStream(MinidumpFileWriter *minidump_writer,
 
   UntypedMDRVA memory(minidump_writer);
   if (!WriteThreadStack(writer_args->sig_ctx->ebp,
-                   writer_args->sig_ctx->esp,
-                   writer_args->thread_lister,
-                   &memory,
-                   &thread->stack))
+                        writer_args->sig_ctx->esp,
+                        writer_args->thread_lister,
+                        &memory,
+                        &thread->stack))
     return false;
 
   TypedMDRVA<MDRawContextX86> context(minidump_writer);
@@ -714,12 +720,15 @@ int Write(void *argument) {
   if (!writer_args->thread_lister->SuspendAllThreads())
     return -1;
 
-  if (writer_args->sig_ctx != NULL) {
+  if (writer_args->sighandler_ebp != 0 &&
+      writer_args->thread_lister->FindSigContext(writer_args->sighandler_ebp,
+                                                 &writer_args->sig_ctx)) {
     writer_args->crashed_stack_bottom =
-    writer_args->thread_lister->GetThreadStackBottom(writer_args->sig_ctx->ebp);
+      writer_args->thread_lister->GetThreadStackBottom(
+                                             writer_args->sig_ctx->ebp);
     int crashed_pid =  FindCrashingThread(writer_args->crashed_stack_bottom,
-                                         writer_args->requester_pid,
-                                         writer_args->thread_lister);
+                                          writer_args->requester_pid,
+                                          writer_args->thread_lister);
     if (crashed_pid > 0)
       writer_args->crashed_pid = crashed_pid;
   }
@@ -769,7 +778,8 @@ void MinidumpGenerator::AllocateStack() {
 
 bool MinidumpGenerator::WriteMinidumpToFile(const char *file_pathname,
                                    int signo,
-                                   const struct sigcontext *sig_ctx) const {
+                                   uintptr_t sighandler_ebp,
+                                   struct sigcontext **sig_ctx) const {
   assert(file_pathname != NULL);
   assert(stack_ != NULL);
 
@@ -786,12 +796,15 @@ bool MinidumpGenerator::WriteMinidumpToFile(const char *file_pathname,
     argument.requester_pid = getpid();
     argument.crashed_pid = getpid();
     argument.signo = signo;
-    argument.sig_ctx = sig_ctx;
+    argument.sighandler_ebp = sighandler_ebp;
+    argument.sig_ctx = NULL;
 
     int cloned_pid = clone(Write, stack_.get() + kStackSize,
                            CLONE_VM | CLONE_FILES | CLONE_FS | CLONE_UNTRACED,
                            (void*)&argument);
     waitpid(cloned_pid, NULL, __WALL);
+    if (sig_ctx != NULL)
+        *sig_ctx = argument.sig_ctx;
     return true;
   }
 
diff --git a/src/client/linux/handler/minidump_generator.h b/src/client/linux/handler/minidump_generator.h
index db74f914..7c0511f5 100644
--- a/src/client/linux/handler/minidump_generator.h
+++ b/src/client/linux/handler/minidump_generator.h
@@ -32,6 +32,8 @@
 #ifndef CLIENT_LINUX_HANDLER_MINIDUMP_GENERATOR_H__
 #define CLIENT_LINUX_HANDLER_MINIDUMP_GENERATOR_H__
 
+#include <stdint.h>
+
 #include "google_breakpad/common/breakpad_types.h"
 #include "processor/scoped_ptr.h"
 
@@ -54,7 +56,8 @@ class MinidumpGenerator {
    // Write minidump.
    bool WriteMinidumpToFile(const char *file_pathname,
                             int signo,
-                            const struct sigcontext *sig_ctx) const;
+                            uintptr_t sighandler_ebp,
+                            struct sigcontext **sig_ctx) const;
   private:
    // Allocate memory for stack.
    void AllocateStack();
diff --git a/src/client/linux/handler/minidump_test.cc b/src/client/linux/handler/minidump_test.cc
index a6ebcc20..f8c4e784 100644
--- a/src/client/linux/handler/minidump_test.cc
+++ b/src/client/linux/handler/minidump_test.cc
@@ -77,7 +77,7 @@ static void CreateThread(int num) {
 int main(int argc, char *argv[]) {
   CreateThread(10);
   google_breakpad::MinidumpGenerator mg;
-  if (mg.WriteMinidumpToFile("minidump_test.out", -1, NULL))
+  if (mg.WriteMinidumpToFile("minidump_test.out", -1, 0, NULL))
     printf("Succeeded written minidump\n");
   else
     printf("Failed to write minidump\n");