Issue 159: reviewer Waylonis

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

4 files changed, 154 insertions, 76 deletions

diff --git a/src/client/mac/handler/exception_handler.cc b/src/client/mac/handler/exception_handler.cc
index e540df23..8f87dd83 100644
--- a/src/client/mac/handler/exception_handler.cc
+++ b/src/client/mac/handler/exception_handler.cc
@@ -133,6 +133,7 @@ ExceptionHandler::ExceptionHandler(const string &dump_path,
       filter_(filter),
       callback_(callback),
       callback_context_(callback_context),
+      directCallback_(NULL),
       handler_thread_(NULL),
       handler_port_(0),
       previous_(NULL),
@@ -146,6 +147,27 @@ ExceptionHandler::ExceptionHandler(const string &dump_path,
   Setup(install_handler);
 }
 
+// special constructor if we want to bypass minidump writing and
+// simply get a callback with the exception information
+ExceptionHandler::ExceptionHandler(DirectCallback callback,
+                                   void *callback_context,
+                                   bool install_handler)
+    : dump_path_(),
+      filter_(NULL),
+      callback_(NULL),
+      callback_context_(callback_context),
+      directCallback_(callback),
+      handler_thread_(NULL),
+      handler_port_(0),
+      previous_(NULL),
+      installed_exception_handler_(false),
+      is_in_teardown_(false),
+      last_minidump_write_result_(false),
+      use_minidump_write_mutex_(false) {
+  MinidumpGenerator::GatherSystemInformation();
+  Setup(install_handler);
+}
+
 ExceptionHandler::~ExceptionHandler() {
   Teardown();
 }
@@ -186,36 +208,47 @@ bool ExceptionHandler::WriteMinidumpWithException(int exception_type,
                                                   int exception_code,
                                                   mach_port_t thread_name) {
   bool result = false;
-  string minidump_id;
-
-  // Putting the MinidumpGenerator in its own context will ensure that the
-  // destructor is executed, closing the newly created minidump file.
-  if (!dump_path_.empty()) {
-    MinidumpGenerator md;
-    if (exception_type && exception_code) {
-      // If this is a real exception, give the filter (if any) a chance to
-      // decided if this should be sent
-      if (filter_ && !filter_(callback_context_))
-        return false;
 
-      md.SetExceptionInformation(exception_type, exception_code, thread_name);
+  if (directCallback_) {
+    if (directCallback_(callback_context_,
+                        exception_type,
+                        exception_code,
+                        thread_name) ) {
+      if (exception_type && exception_code)
+        exit(exception_type);
     }
+  } else {
+    string minidump_id;
+
+    // Putting the MinidumpGenerator in its own context will ensure that the
+    // destructor is executed, closing the newly created minidump file.
+    if (!dump_path_.empty()) {
+      MinidumpGenerator md;
+      if (exception_type && exception_code) {
+        // If this is a real exception, give the filter (if any) a chance to
+        // decided if this should be sent
+        if (filter_ && !filter_(callback_context_))
+          return false;
+
+        md.SetExceptionInformation(exception_type, exception_code, thread_name);
+      }
 
-    result = md.Write(next_minidump_path_c_);
-  }
+      result = md.Write(next_minidump_path_c_);
+    }
 
-  // Call user specified callback (if any)
-  if (callback_) {
-    // If the user callback returned true and we're handling an exception
-    // (rather than just writing out the file), then we should exit without
-    // forwarding the exception to the next handler.
-    if (callback_(dump_path_c_, next_minidump_id_c_, callback_context_, 
-                  result)) {
-      if (exception_type && exception_code)
-        exit(exception_type);
+    // Call user specified callback (if any)
+    if (callback_) {
+      // If the user callback returned true and we're handling an exception
+      // (rather than just writing out the file), then we should exit without
+      // forwarding the exception to the next handler.
+      if (callback_(dump_path_c_, next_minidump_id_c_, callback_context_, 
+                    result)) {
+        if (exception_type && exception_code)
+          exit(exception_type);
+      }
     }
   }
-
+  
   return result;
 }
 
@@ -326,14 +359,11 @@ void *ExceptionHandler::WaitForMessage(void *exception_handler_class) {
                                     MACH_RCV_MSG | MACH_RCV_LARGE, 0,
                                     sizeof(receive), self->handler_port_,
                                     MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
-
     if (result == KERN_SUCCESS) {
       // Uninstall our handler so that we don't get in a loop if the process of
       // writing out a minidump causes an exception.  However, if the exception
       // was caused by a fork'd process, don't uninstall things
       if (receive.task.name == mach_task_self())
-        self->UninstallHandler();
-      
       // If the actual exception code is zero, then we're calling this handler
       // in a way that indicates that we want to either exit this thread or
       // generate a minidump
@@ -342,6 +372,8 @@ void *ExceptionHandler::WaitForMessage(void *exception_handler_class) {
       // to avoid misleading stacks.  If appropriate they will be resumed
       // afterwards.
       if (!receive.exception) {
+        self->UninstallHandler(false);
+      
         if (self->is_in_teardown_)
           return NULL;
 
@@ -356,6 +388,8 @@ void *ExceptionHandler::WaitForMessage(void *exception_handler_class) {
         if (self->use_minidump_write_mutex_)
           pthread_mutex_unlock(&self->minidump_write_mutex_);
       } else {
+        self->UninstallHandler(true);
+
         // When forking a child process with the exception handler installed,
         // if the child crashes, it will send the exception back to the parent
         // process.  The check for task == self_task() ensures that only 
@@ -419,7 +453,7 @@ bool ExceptionHandler::InstallHandler() {
   return installed_exception_handler_;
 }
 
-bool ExceptionHandler::UninstallHandler() {
+bool ExceptionHandler::UninstallHandler(bool in_exception) {
   kern_return_t result = KERN_SUCCESS;
   
   if (installed_exception_handler_) {
@@ -435,7 +469,11 @@ bool ExceptionHandler::UninstallHandler() {
         return false;
     }
     
-    delete previous_;
+    // this delete should NOT happen if an exception just occurred!
+    if (!in_exception) {
+      delete previous_; 
+    }
+    
     previous_ = NULL;
     installed_exception_handler_ = false;
   }
@@ -479,7 +517,7 @@ bool ExceptionHandler::Teardown() {
   kern_return_t result = KERN_SUCCESS;
   is_in_teardown_ = true;
 
-  if (!UninstallHandler())
+  if (!UninstallHandler(false))
     return false;
   
   // Send an empty message so that the handler_thread exits
diff --git a/src/client/mac/handler/exception_handler.h b/src/client/mac/handler/exception_handler.h
index 11babb73..8d7755a6 100644
--- a/src/client/mac/handler/exception_handler.h
+++ b/src/client/mac/handler/exception_handler.h
@@ -71,6 +71,14 @@ class ExceptionHandler {
                                    const char *minidump_id,
                                    void *context, bool succeeded);
 
+  // A callback function which will be called directly if an exception occurs.
+  // This bypasses the minidump file writing and simply gives the client
+  // the exception information.
+  typedef bool (*DirectCallback)( void *context,
+                                  int exception_type,
+                                  int exception_code,
+                                  mach_port_t thread_name);
+
   // Creates a new ExceptionHandler instance to handle writing minidumps.
   // Minidump files will be written to dump_path, and the optional callback
   // is called after writing the dump file, as described above.
@@ -80,6 +88,13 @@ class ExceptionHandler {
   ExceptionHandler(const string &dump_path, 
                    FilterCallback filter, MinidumpCallback callback,
                    void *callback_context, bool install_handler);
+
+  // A special constructor if we want to bypass minidump writing and
+  // simply get a callback with the exception information.
+  ExceptionHandler(DirectCallback callback,
+                   void *callback_context,
+                   bool install_handler);
+
   ~ExceptionHandler();
 
   // Get and set the minidump path.
@@ -104,7 +119,7 @@ class ExceptionHandler {
   bool InstallHandler();
 
   // Uninstall the mach exception handler (if any)
-  bool UninstallHandler();
+  bool UninstallHandler(bool in_exception);
       
   // Setup the handler thread, and if |install_handler| is true, install the
   // mach exception port handler
@@ -159,6 +174,10 @@ class ExceptionHandler {
   MinidumpCallback callback_;
   void *callback_context_;
 
+  // The callback function to be passed back when we don't want a minidump
+  // file to be written
+  DirectCallback directCallback_;
+
   // The thread that is created for the handler
   pthread_t handler_thread_;
 
diff --git a/src/client/mac/handler/minidump_generator.cc b/src/client/mac/handler/minidump_generator.cc
index 5cddf88d..bdabc1f4 100644
--- a/src/client/mac/handler/minidump_generator.cc
+++ b/src/client/mac/handler/minidump_generator.cc
@@ -50,7 +50,21 @@ namespace google_breakpad {
 MinidumpGenerator::MinidumpGenerator()
     : exception_type_(0),
       exception_code_(0),
-      exception_thread_(0) {
+      exception_thread_(0),
+      crashing_task_(mach_task_self()),
+      handler_thread_(mach_thread_self()) {
+  dynamic_images_ = new DynamicImages(mach_task_self());
+  GatherSystemInformation();
+}
+
+MinidumpGenerator::MinidumpGenerator(mach_port_t crashing_task, mach_port_t handler_thread)
+    : exception_type_(0),
+      exception_code_(0),
+      exception_thread_(0),
+      crashing_task_(crashing_task),
+      handler_thread_(handler_thread) {
+  dynamic_images_ = new DynamicImages(crashing_task_);
+  GatherSystemInformation();
 }
 
 MinidumpGenerator::~MinidumpGenerator() {
@@ -184,14 +198,14 @@ bool MinidumpGenerator::Write(const char *path) {
   return result;
 }
 
-static size_t CalculateStackSize(vm_address_t start_addr) {
+size_t MinidumpGenerator::CalculateStackSize(vm_address_t start_addr) {
   vm_address_t stack_region_base = start_addr;
   vm_size_t stack_region_size;
   natural_t nesting_level = 0;
   vm_region_submap_info submap_info;
   mach_msg_type_number_t info_count = VM_REGION_SUBMAP_INFO_COUNT;
   kern_return_t result = 
-    vm_region_recurse(mach_task_self(), &stack_region_base, &stack_region_size,
+    vm_region_recurse(crashing_task_, &stack_region_base, &stack_region_size,
                       &nesting_level, 
                       reinterpret_cast<vm_region_recurse_info_t>(&submap_info),
                       &info_count);
@@ -225,7 +239,13 @@ bool MinidumpGenerator::WriteStackFromStartAddress(
   if (!memory.Allocate(size))
     return false;
 
-  bool result = memory.Copy(reinterpret_cast<const void *>(start_addr), size);
+  void *stack_memory = ReadTaskMemory(crashing_task_, (void*)start_addr, size);
+
+  bool result = memory.Copy(stack_memory, size);
+  
+  free(stack_memory);
+  
+  
   stack_location->start_of_memory_range = start_addr;
   stack_location->memory = memory.location();
 
@@ -384,7 +404,7 @@ bool MinidumpGenerator::WriteThreadListStream(
   mach_msg_type_number_t thread_count;
   int non_generator_thread_count;
 
-  if (task_threads(mach_task_self(), &threads_for_task, &thread_count))
+  if (task_threads(crashing_task_, &threads_for_task, &thread_count))
     return false;
 
   // Don't include the generator thread
@@ -404,7 +424,7 @@ bool MinidumpGenerator::WriteThreadListStream(
   for (unsigned int i = 0; i < thread_count; ++i) {
     memset(&thread, 0, sizeof(MDRawThread));
 
-    if (threads_for_task[i] != mach_thread_self()) {
+    if (threads_for_task[i] != handler_thread_) {
       if (!WriteThreadStream(threads_for_task[i], &thread))
         return false;
 
@@ -496,57 +516,45 @@ bool MinidumpGenerator::WriteSystemInfoStream(
 
 bool MinidumpGenerator::WriteModuleStream(unsigned int index,
                                           MDRawModule *module) {
-  const struct mach_header *header = _dyld_get_image_header(index);
+  DynamicImage *image = dynamic_images_->GetImage(index);
+
+  if (!image)
+    return false;
+
+  const mach_header *header = image->GetMachHeader();
 
   if (!header)
     return false;
 
   int cpu_type = header->cputype;
-  unsigned long slide = _dyld_get_image_vmaddr_slide(index);
-  const char* name = _dyld_get_image_name(index);
-  const struct load_command *cmd =
-    reinterpret_cast<const struct load_command *>(header + 1);
 
   memset(module, 0, sizeof(MDRawModule));
 
-  for (unsigned int i = 0; cmd && (i < header->ncmds); i++) {
-    if (cmd->cmd == LC_SEGMENT) {
-      const struct segment_command *seg =
-        reinterpret_cast<const struct segment_command *>(cmd);
-      if (!strcmp(seg->segname, "__TEXT")) {
-        MDLocationDescriptor string_location;
-
-        if (!writer_.WriteString(name, 0, &string_location))
-          return false;
+  MDLocationDescriptor string_location;
 
-        module->base_of_image = seg->vmaddr + slide;
-        module->size_of_image = seg->vmsize;
-        module->module_name_rva = string_location.rva;
-
-        if (!WriteCVRecord(module, cpu_type, name))
-          return false;
+  const char* name = image->GetFilePath();
+  if (!writer_.WriteString(name, 0, &string_location))
+    return false;
 
-        return true;
-      }
-    }
+  module->base_of_image = image->GetVMAddr() + image->GetVMAddrSlide();
+  module->size_of_image = image->GetVMSize();
+  module->module_name_rva = string_location.rva;
 
-    cmd = reinterpret_cast<struct load_command *>((char *)cmd + cmd->cmdsize);
+  if (!WriteCVRecord(module, cpu_type, name)) {
+    return false;
   }
 
   return true;
 }
 
-static int FindExecutableModule() {
-  int image_count = _dyld_image_count();
-  const struct mach_header *header;
+int MinidumpGenerator::FindExecutableModule() {
+  int index = dynamic_images_->GetExecutableImageIndex();
 
-  for (int i = 0; i < image_count; ++i) {
-    header = _dyld_get_image_header(i);
-
-    if (header->filetype == MH_EXECUTE)
-      return i;
+  if (index >= 0) {
+    return index;
   }
-
+  
+  // failed - just use the first image
   return 0;
 }
 
@@ -606,7 +614,7 @@ bool MinidumpGenerator::WriteModuleListStream(
   if (!_dyld_present())
     return false;
 
-  int image_count = _dyld_image_count();
+  int image_count = dynamic_images_->GetImageCount();
 
   if (!list.AllocateObjectAndArray(image_count, MD_MODULE_SIZE))
     return false;
@@ -619,16 +627,18 @@ bool MinidumpGenerator::WriteModuleListStream(
   MDRawModule module;
   int executableIndex = FindExecutableModule();
 
-  if (!WriteModuleStream(executableIndex, &module))
+  if (!WriteModuleStream(executableIndex, &module)) {
     return false;
+  }
 
   list.CopyIndexAfterObject(0, &module, MD_MODULE_SIZE);
   int destinationIndex = 1;  // Write all other modules after this one
 
   for (int i = 0; i < image_count; ++i) {
     if (i != executableIndex) {
-      if (!WriteModuleStream(i, &module))
+      if (!WriteModuleStream(i, &module)) {
         return false;
+      }
 
       list.CopyIndexAfterObject(destinationIndex++, &module, MD_MODULE_SIZE);
     }
@@ -701,11 +711,11 @@ bool MinidumpGenerator::WriteBreakpadInfoStream(
   if (exception_thread_ && exception_type_) {
     info_ptr->validity = MD_BREAKPAD_INFO_VALID_DUMP_THREAD_ID |
                          MD_BREAKPAD_INFO_VALID_REQUESTING_THREAD_ID;
-    info_ptr->dump_thread_id = mach_thread_self();
+    info_ptr->dump_thread_id = handler_thread_;
     info_ptr->requesting_thread_id = exception_thread_;
   } else {
     info_ptr->validity = MD_BREAKPAD_INFO_VALID_DUMP_THREAD_ID;
-    info_ptr->dump_thread_id = mach_thread_self();
+    info_ptr->dump_thread_id = handler_thread_;
     info_ptr->requesting_thread_id = 0;
   }
 
diff --git a/src/client/mac/handler/minidump_generator.h b/src/client/mac/handler/minidump_generator.h
index 3061d46b..9fee9e5a 100644
--- a/src/client/mac/handler/minidump_generator.h
+++ b/src/client/mac/handler/minidump_generator.h
@@ -39,6 +39,8 @@
 #include "client/minidump_file_writer.h"
 #include "google_breakpad/common/minidump_format.h"
 
+#include "dynamic_images.h"
+
 namespace google_breakpad {
 
 using std::string;
@@ -53,6 +55,8 @@ using std::string;
 class MinidumpGenerator {
  public:
   MinidumpGenerator();
+  MinidumpGenerator(mach_port_t crashing_task, mach_port_t handler_thread);
+
   ~MinidumpGenerator();
 
   // Return <dir>/<unique_name>.dmp
@@ -98,6 +102,8 @@ class MinidumpGenerator {
   bool WriteCVRecord(MDRawModule *module, int cpu_type, 
                      const char *module_path);
   bool WriteModuleStream(unsigned int index, MDRawModule *module);
+  size_t CalculateStackSize(vm_address_t start_addr);
+  int  FindExecutableModule();
 
   // disallow copy ctor and operator=
   explicit MinidumpGenerator(const MinidumpGenerator &);
@@ -110,12 +116,17 @@ class MinidumpGenerator {
   int exception_type_;
   int exception_code_;
   mach_port_t exception_thread_;
-
+  mach_port_t crashing_task_;
+  mach_port_t handler_thread_;
+  
   // System information
   static char build_string_[16];
   static int os_major_version_;
   static int os_minor_version_;
   static int os_build_number_;
+  
+  // Information about dynamically loaded code
+  DynamicImages *dynamic_images_;
 };
 
 }  // namespace google_breakpad