Windows exception handler does not survive stack overflows (#34). r=brian,

thanks also to darin
 - All minidump writing is now done on a dedicated thread.  When a stack
   overflow exception occurs, the only work that needs to be done on the
   exception thread will easily fit within the guard page.

http://groups.google.com/group/airbag-dev/browse_thread/thread/3935e339d8354a75


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

2 files changed, 145 insertions, 11 deletions

diff --git a/src/client/windows/handler/exception_handler.cc b/src/client/windows/handler/exception_handler.cc
index e58dea65..1d64cf9e 100644
--- a/src/client/windows/handler/exception_handler.cc
+++ b/src/client/windows/handler/exception_handler.cc
@@ -37,6 +37,14 @@
 namespace google_airbag {
 
 ExceptionHandler *ExceptionHandler::current_handler_ = NULL;
+HANDLE ExceptionHandler::handler_thread_ = NULL;
+CRITICAL_SECTION ExceptionHandler::handler_critical_section_;
+HANDLE ExceptionHandler::handler_start_semaphore_ = NULL;
+HANDLE ExceptionHandler::handler_finish_semaphore_ = NULL;
+ExceptionHandler *ExceptionHandler::requesting_handler_ = NULL;
+DWORD ExceptionHandler::requesting_thread_id_ = 0;
+EXCEPTION_POINTERS *ExceptionHandler::exception_info_ = NULL;
+bool ExceptionHandler::handler_return_value_ = false;
 
 ExceptionHandler::ExceptionHandler(const wstring &dump_path,
                                    MinidumpCallback callback,
@@ -46,6 +54,22 @@ ExceptionHandler::ExceptionHandler(const wstring &dump_path,
       dump_path_(dump_path), dbghelp_module_(NULL),
       minidump_write_dump_(NULL), previous_handler_(current_handler_),
       previous_filter_(NULL) {
+  if (!handler_thread_) {
+    // The first time an ExceptionHandler is created, set up the handler
+    // thread and the synchronization primitives.
+    InitializeCriticalSection(&handler_critical_section_);
+    handler_start_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
+    handler_finish_semaphore_ = CreateSemaphore(NULL, 0, 1, NULL);
+
+    DWORD thread_id;
+    handler_thread_ = CreateThread(NULL,         // lpThreadAttributes
+                                   64 * 1024,    // dwStackSize
+                                   ExceptionHandlerThreadMain,
+                                   NULL,         // lpParameter
+                                   0,            // dwCreationFlags
+                                   &thread_id);
+  }
+
   UpdateNextID();
   dbghelp_module_ = LoadLibrary(L"dbghelp.dll");
   if (dbghelp_module_) {
@@ -66,18 +90,72 @@ ExceptionHandler::~ExceptionHandler() {
     SetUnhandledExceptionFilter(previous_filter_);
     current_handler_ = previous_handler_;
   }
+
+  if (previous_handler_ == NULL) {
+    // When destroying the last ExceptionHandler, clean up the handler thread
+    // and synchronization primitives.
+    TerminateThread(handler_thread_, 1);
+    handler_thread_ = NULL;
+    DeleteCriticalSection(&handler_critical_section_);
+    CloseHandle(handler_start_semaphore_);
+    handler_start_semaphore_ = NULL;
+    CloseHandle(handler_finish_semaphore_);
+    handler_finish_semaphore_ = NULL;
+  }
 }
 
 // static
-LONG ExceptionHandler::HandleException(EXCEPTION_POINTERS *exinfo) {
-  if (!current_handler_->WriteMinidumpWithException(exinfo)) {
-    return EXCEPTION_CONTINUE_SEARCH;
+DWORD ExceptionHandler::ExceptionHandlerThreadMain(void *lpParameter) {
+  while (true) {
+    if (WaitForSingleObject(handler_start_semaphore_, INFINITE) ==
+        WAIT_OBJECT_0) {
+      // Perform the requested action.
+      handler_return_value_ = requesting_handler_->WriteMinidumpWithException(
+          requesting_thread_id_, exception_info_);
+
+      // Allow the requesting thread to proceed.
+      ReleaseSemaphore(handler_finish_semaphore_, 1, NULL);
+    }
   }
-  return EXCEPTION_EXECUTE_HANDLER;
+
+  // Not reached.  This thread will be terminated by ExceptionHandler's
+  // destructor.
+  return 0;
+}
+
+// static
+LONG ExceptionHandler::HandleException(EXCEPTION_POINTERS *exinfo) {
+  return current_handler_->WriteMinidumpOnHandlerThread(exinfo) ?
+      EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH;
+}
+
+bool ExceptionHandler::WriteMinidumpOnHandlerThread(EXCEPTION_POINTERS *exinfo) {
+  EnterCriticalSection(&handler_critical_section_);
+
+  // Set up data to be passed in to the handler thread.
+  requesting_handler_ = this;
+  requesting_thread_id_ = GetCurrentThreadId();
+  exception_info_ = exinfo;
+
+  // This causes the handler thread to call WriteMinidumpWithException.
+  ReleaseSemaphore(handler_start_semaphore_, 1, NULL);
+
+  // Wait until WriteMinidumpWithException is done and collect its return value.
+  WaitForSingleObject(handler_finish_semaphore_, INFINITE);
+  bool status = handler_return_value_;
+
+  // Clean up.
+  requesting_handler_ = NULL;
+  requesting_thread_id_ = 0;
+  exception_info_ = NULL;
+
+  LeaveCriticalSection(&handler_critical_section_);
+
+  return status;
 }
 
 bool ExceptionHandler::WriteMinidump() {
-  bool success = WriteMinidumpWithException(NULL);
+  bool success = WriteMinidumpOnHandlerThread(NULL);
   UpdateNextID();
   return success;
 }
@@ -90,7 +168,8 @@ bool ExceptionHandler::WriteMinidump(const wstring &dump_path,
   return handler.WriteMinidump();
 }
 
-bool ExceptionHandler::WriteMinidumpWithException(EXCEPTION_POINTERS *exinfo) {
+bool ExceptionHandler::WriteMinidumpWithException(DWORD requesting_thread_id,
+                                                  EXCEPTION_POINTERS *exinfo) {
   wchar_t dump_file_name[MAX_PATH];
   swprintf_s(dump_file_name, MAX_PATH, L"%s\\%s.dmp",
              dump_path_.c_str(), next_minidump_id_.c_str());
@@ -106,16 +185,18 @@ bool ExceptionHandler::WriteMinidumpWithException(EXCEPTION_POINTERS *exinfo) {
                                   NULL);
     if (dump_file != INVALID_HANDLE_VALUE) {
       MINIDUMP_EXCEPTION_INFORMATION except_info;
-      except_info.ThreadId = GetCurrentThreadId();
+      except_info.ThreadId = requesting_thread_id;
       except_info.ExceptionPointers = exinfo;
       except_info.ClientPointers = FALSE;
 
+      // TODO(mmentovai): include IDs of handler and requesting threads.
+
       // The explicit comparison to TRUE avoids a warning (C4800).
       success = (minidump_write_dump_(GetCurrentProcess(),
                                       GetCurrentProcessId(),
                                       dump_file,
                                       MiniDumpNormal,
-                                      &except_info,
+                                      exinfo ? &except_info : NULL,
                                       NULL,
                                       NULL) == TRUE);
 
diff --git a/src/client/windows/handler/exception_handler.h b/src/client/windows/handler/exception_handler.h
index 9f7f14d0..b49093aa 100644
--- a/src/client/windows/handler/exception_handler.h
+++ b/src/client/windows/handler/exception_handler.h
@@ -116,12 +116,30 @@ class ExceptionHandler {
       CONST PMINIDUMP_USER_STREAM_INFORMATION UserStreamParam,
       CONST PMINIDUMP_CALLBACK_INFORMATION CallbackParam);
 
-  // This function does the actual writing of a minidump.
-  bool WriteMinidumpWithException(EXCEPTION_POINTERS *exinfo);
+  // Runs the main loop for the exception handler thread.
+  static DWORD WINAPI ExceptionHandlerThreadMain(void *lpParameter);
 
-  // Called when an unhandled exception occurs.
+  // Called on the exception thread when an unhandled exception occurs.
+  // Signals the exception handler thread to handle the exception.
   static LONG WINAPI HandleException(EXCEPTION_POINTERS *exinfo);
 
+  // This is called on the exception thread or on another thread that
+  // the user wishes to produce a dump from.  It calls
+  // WriteMinidumpWithException on the handler thread, avoiding stack
+  // overflows and inconsistent dumps due to writing the dump from
+  // the exception thread.  If the dump is requested as a result of an
+  // exception, exinfo contains exception information, otherwise, it
+  // is NULL.
+  bool WriteMinidumpOnHandlerThread(EXCEPTION_POINTERS *exinfo);
+
+  // This function does the actual writing of a minidump.  It is called
+  // on the handler thread.  requesting_thread_id is the ID of the thread
+  // that requested the dump.  If the dump is requested as a result of
+  // an exception, exinfo contains exception information, otherwise,
+  // it is NULL.
+  bool WriteMinidumpWithException(DWORD requesting_thread_id,
+                                  EXCEPTION_POINTERS *exinfo);
+
   // Generates a new ID and stores it in next_minidump_id_.
   void UpdateNextID();
 
@@ -140,6 +158,41 @@ class ExceptionHandler {
   // the currently-installed ExceptionHandler, of which there can be only 1
   static ExceptionHandler *current_handler_;
 
+  // The exception handler thread, if one has been created.
+  static HANDLE handler_thread_;
+
+  // The critical section enforcing the requirement that only one exception be
+  // handled at a time.
+  static CRITICAL_SECTION handler_critical_section_;
+
+  // Semaphores used to move exception handling between the exception thread
+  // and the handler thread.  handler_start_semaphore_ is signalled by the
+  // exception thread to wake up the handler thread when an exception occurs.
+  // handler_finish_semaphore_ is signalled by the handler thread to wake up
+  // the exception thread when handling is complete.
+  static HANDLE handler_start_semaphore_;
+  static HANDLE handler_finish_semaphore_;
+
+  // The next 3 fields are static data passed from the requesting thread to
+  // the handler thread.
+
+  // The ExceptionHandler through which a request to write a dump was routed.
+  // This will be the same as current_handler_ for exceptions, but
+  // user-requested dumps may be routed through any live ExceptionHandler.
+  static ExceptionHandler *requesting_handler_;
+
+  // The thread ID of the thread requesting the dump (either the exception
+  // thread or any other thread that called WriteMinidump directly).
+  static DWORD requesting_thread_id_;
+
+  // The exception info passed to the exception handler on the exception
+  // thread, if an exception occurred.  NULL for user-requested dumps.
+  static EXCEPTION_POINTERS *exception_info_;
+
+  // The return value of the handler, passed from the handler thread back to
+  // the requesting thread.
+  static bool handler_return_value_;
+
   // disallow copy ctor and operator=
   explicit ExceptionHandler(const ExceptionHandler &);
   void operator=(const ExceptionHandler &);