Fix for issue 242, plus a redo of how old exception handlers are tracked, and called.

R=Craig.Schlenter, luly81



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

3 files changed, 59 insertions, 22 deletions

diff --git a/src/client/linux/handler/Makefile b/src/client/linux/handler/Makefile
index ef30df1d..73be56d8 100644
--- a/src/client/linux/handler/Makefile
+++ b/src/client/linux/handler/Makefile
@@ -1,7 +1,7 @@
 CXX=g++
 CC=gcc
 
-CXXFLAGS=-gstabs -I../../.. -Wall -D_REENTRANT
+CXXFLAGS=-gstabs+ -I../../.. -Wall -D_REENTRANT
 LDFLAGS=-lpthread
 
 OBJ_DIR=.
diff --git a/src/client/linux/handler/exception_handler.cc b/src/client/linux/handler/exception_handler.cc
index b253cb39..26e09b7d 100644
--- a/src/client/linux/handler/exception_handler.cc
+++ b/src/client/linux/handler/exception_handler.cc
@@ -81,6 +81,15 @@ ExceptionHandler::ExceptionHandler(const string &dump_path,
       installed_handler_(install_handler) {
   set_dump_path(dump_path);
 
+  act_.sa_handler = HandleException;
+  act_.sa_flags = SA_ONSTACK;
+  sigemptyset(&act_.sa_mask);
+  // now, make sure we're blocking all the signals we are handling
+  // when we're handling any of them
+  for ( size_t i = 0; i < sizeof(SigTable) / sizeof(SigTable[0]); ++i) {
+    sigaddset(&act_.sa_mask, SigTable[i]);
+  }
+
   if (install_handler) {
     SetupHandler();
     pthread_mutex_lock(&handler_stack_mutex_);
@@ -149,20 +158,26 @@ void ExceptionHandler::SetupHandler() {
 }
 
 void ExceptionHandler::SetupHandler(int signo) {
-  struct sigaction act, old_act;
-  act.sa_handler = HandleException;
-  act.sa_flags = SA_ONSTACK;
-  if (sigaction(signo, &act, &old_act) < 0)
-    return;
-  old_handlers_[signo] = old_act.sa_handler;
+
+  // We're storing pointers to the old signal action
+  // structure, rather than copying the structure
+  // because we can't count on the sa_mask field to
+  // be scalar.
+  struct sigaction *old_act = &old_actions_[signo];
+
+  if (sigaction(signo, &act_, old_act) < 0)
+   return;
 }
 
 void ExceptionHandler::TeardownHandler(int signo) {
-  if (old_handlers_.find(signo) != old_handlers_.end()) {
-    struct sigaction act;
-    act.sa_handler = old_handlers_[signo];
-    act.sa_flags = 0;
-    sigaction(signo, &act, 0);
+  TeardownHandler(signo, NULL);
+}
+
+void ExceptionHandler::TeardownHandler(int signo, struct sigaction *final_handler) {
+  if (old_actions_[signo].sa_handler) {
+    struct sigaction *act = &old_actions_[signo];
+    sigaction(signo, act, final_handler);
+    memset(&old_actions_[signo], 0x0, sizeof(struct sigaction));
   }
 }
 
@@ -193,7 +208,8 @@ void ExceptionHandler::HandleException(int signo) {
   pthread_mutex_unlock(&handler_stack_mutex_);
 
   // Restore original handler.
-  current_handler->TeardownHandler(signo);
+  struct sigaction old_action;
+  current_handler->TeardownHandler(signo, &old_action);
 
   struct sigcontext *sig_ctx = NULL;
   if (current_handler->InternalWriteMinidump(signo, current_ebp, &sig_ctx)) {
@@ -202,11 +218,23 @@ void ExceptionHandler::HandleException(int signo) {
   } else {
     // Exception not fully handled, will call the next handler in stack to
     // process it.
-    typedef void (*SignalHandler)(int signo, struct sigcontext);
-    SignalHandler old_handler =
-      reinterpret_cast<SignalHandler>(current_handler->old_handlers_[signo]);
-    if (old_handler != NULL && sig_ctx != NULL)
+    if (old_action.sa_handler != NULL && sig_ctx != NULL) {
+
+      // Have our own typedef, because of the comment above w.r.t signal
+      // context on the stack
+      typedef void (*SignalHandler)(int signo, struct sigcontext);
+
+      SignalHandler old_handler =
+          reinterpret_cast<SignalHandler>(old_action.sa_handler);
+
+      sigset_t old_set;
+      // Use SIG_BLOCK here because we don't want to unblock a signal
+      // that the signal handler we're currently in needs to block
+      sigprocmask(SIG_BLOCK, &old_action.sa_mask, &old_set);
       old_handler(signo, *sig_ctx);
+      sigprocmask(SIG_SETMASK, &old_set, NULL);
+    }
+
   }
 
   pthread_mutex_lock(&handler_stack_mutex_);
@@ -247,7 +275,7 @@ bool ExceptionHandler::InternalWriteMinidump(int signo,
 
   // Unblock the signals.
   if (blocked) {
-    sigprocmask(SIG_SETMASK, &sig_old, &sig_old);
+    sigprocmask(SIG_SETMASK, &sig_old, NULL);
   }
 
   if (callback_)
diff --git a/src/client/linux/handler/exception_handler.h b/src/client/linux/handler/exception_handler.h
index 6e6156c5..6ea09a11 100644
--- a/src/client/linux/handler/exception_handler.h
+++ b/src/client/linux/handler/exception_handler.h
@@ -36,6 +36,7 @@
 
 #include <map>
 #include <string>
+#include <signal.h>
 #include <vector>
 
 #include "client/linux/handler/minidump_generator.h"
@@ -146,6 +147,8 @@ class ExceptionHandler {
   void SetupHandler(int signo);
   // Teardown the handler for a signal.
   void TeardownHandler(int signo);
+  // Teardown the handler for a signal.
+  void TeardownHandler(int signo, struct sigaction *old);
   // Teardown all handlers.
   void TeardownAllHandler();
 
@@ -192,10 +195,6 @@ class ExceptionHandler {
   // when created (with an install_handler parameter set to true).
   bool installed_handler_;
 
-  // Keep the previous handlers for the signal.
-  typedef void (*sighandler_t)(int);
-  std::map<int, sighandler_t> old_handlers_;
-
   // The global exception handler stack. This is need becuase there may exist
   // multiple ExceptionHandler instances in a process. Each will have itself
   // registered in this stack.
@@ -210,6 +209,16 @@ class ExceptionHandler {
   // disallow copy ctor and operator=
   explicit ExceptionHandler(const ExceptionHandler &);
   void operator=(const ExceptionHandler &);
+
+  // The sigactions structure we use for each signal
+  struct sigaction act_;
+
+
+  // Keep the previous handlers for the signal.
+  // We're wasting a bit of memory here since we only change
+  // the handler for some signals but i want to avoid allocating
+  // memory in the signal handler
+  struct sigaction old_actions_[NSIG];
 };
 
 }  // namespace google_breakpad