Breakpad Linux: Fix crash handler writing to disk on the wrong thread....

chrome/browser/crash_handler_host_linux.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/crash_handler_host_linux.h"
 
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/socket.h>
 #include <sys/syscall.h>
-#include <sys/types.h>
 #include <unistd.h>
 
 #include "base/eintr_wrapper.h"
 #include "base/file_path.h"
 #include "base/format_macros.h"
 #include "base/linux_util.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/singleton.h"
 #include "base/string_util.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
 #include "breakpad/src/client/linux/handler/exception_handler.h"
 #include "breakpad/src/client/linux/minidump_writer/linux_dumper.h"
 #include "breakpad/src/client/linux/minidump_writer/minidump_writer.h"
 #include "chrome/app/breakpad_linux.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/env_vars.h"
 
 using google_breakpad::ExceptionHandler;
 
 namespace {
 
+// The length of the control message:
+const unsigned kControlMsgSize =
+    CMSG_SPACE(2*sizeof(int)) + CMSG_SPACE(sizeof(struct ucred));
+// The length of the regular payload:
+const unsigned kCrashContextSize = sizeof(ExceptionHandler::CrashContext);
+
 // Handles the crash dump and frees the allocated BreakpadInfo struct.
 void CrashDumpTask(CrashHandlerHostLinux* handler, BreakpadInfo* info) {
   if (handler->IsShuttingDown())
     return;
 
   HandleCrashDump(*info);
   delete[] info->filename;
   delete[] info->process_type;
   delete[] info->crash_url;
   delete[] info->guid;
@@ skipping 57 matching lines @@
 }
 
 void CrashHandlerHostLinux::OnFileCanReadWithoutBlocking(int fd) {
   DCHECK_EQ(fd, browser_socket_);
 
   // A process has crashed and has signaled us by writing a datagram
   // to the death signal socket. The datagram contains the crash context needed
   // for writing the minidump as well as a file descriptor and a credentials
   // block so that they can't lie about their pid.
 
-  // The length of the control message:
-  static const unsigned kControlMsgSize =
-      CMSG_SPACE(2*sizeof(int)) + CMSG_SPACE(sizeof(struct ucred));
-  // The length of the regular payload:
-  static const unsigned kCrashContextSize =
-      sizeof(ExceptionHandler::CrashContext);
-
   const size_t kIovSize = 7;
   struct msghdr msg = {0};
   struct iovec iov[kIovSize];
   char crash_context[kCrashContextSize];
   char* guid = new char[kGuidSize + 1];
   char* crash_url = new char[kMaxActiveURLSize + 1];
   char* distro = new char[kDistroSize + 1];
   char* tid_buf_addr = NULL;
   int tid_fd = -1;
   uint64_t uptime;
@@ skipping 140 matching lines @@
       // went away.  We'll just take a guess here and assume the crashing
       // thread is the thread group leader.
       crashing_tid = crashing_pid;
     }
 
     ExceptionHandler::CrashContext* bad_context =
         reinterpret_cast<ExceptionHandler::CrashContext*>(crash_context);
     bad_context->tid = crashing_tid;
   }
 
-  bool upload = true;
+  // Sanitize the string data a bit more
+  guid[kGuidSize] = crash_url[kMaxActiveURLSize] = distro[kDistroSize] = 0;
+
+  BreakpadInfo* info = new BreakpadInfo;
+
+  info->process_type_length = process_type_.length();
+  char* process_type_str = new char[info->process_type_length + 1];
+  process_type_.copy(process_type_str, info->process_type_length);
+  process_type_str[info->process_type_length] = '\0';
+  info->process_type = process_type_str;
+
+  info->crash_url_length = strlen(crash_url);
+  info->crash_url = crash_url;
+
+  info->guid_length = strlen(guid);
+  info->guid = guid;
+
+  info->distro_length = strlen(distro);
+  info->distro = distro;
+
+  info->upload = (getenv(env_vars::kHeadless) == NULL);
+  info->process_start_time = uptime;
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(this,
+                        &CrashHandlerHostLinux::WriteDumpFile,
+                        info,
+                        crashing_pid,
+                        reinterpret_cast<char*>(&crash_context),

[piman, 2011/04/15 05:17:17]

Woah, are we passing a reference to a buffer on the stack here ? It's invalid
when WriteDumpFile executes

Also, is it really necessary to hop threads ? We are already on a worker thread.

[Lei Zhang, 2011/04/15 05:25:07]

Doh. I'll fix it in a bit.

+                        signal_fd));
+}
+
+void CrashHandlerHostLinux::WriteDumpFile(BreakpadInfo* info,
+                                          pid_t crashing_pid,
+                                          char* crash_context,
+                                          int signal_fd) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
   FilePath dumps_path("/tmp");
   PathService::Get(base::DIR_TEMP, &dumps_path);
-  if (getenv(env_vars::kHeadless)) {
-    upload = false;
+  if (!info->upload)
     PathService::Get(chrome::DIR_CRASH_DUMPS, &dumps_path);
-  }
   const uint64 rand = base::RandUint64();
   const std::string minidump_filename =
       StringPrintf("%s/chromium-%s-minidump-%016" PRIx64 ".dmp",
                    dumps_path.value().c_str(), process_type_.c_str(), rand);
   if (!google_breakpad::WriteMinidump(minidump_filename.c_str(),
                                       crashing_pid, crash_context,
                                       kCrashContextSize)) {
     LOG(ERROR) << "Failed to write crash dump for pid " << crashing_pid;
-    HANDLE_EINTR(close(signal_fd));

[Lei Zhang, 2011/02/17 22:49:08]

On failure, we closed |signal_fd| twice.

   }
 
+  char* minidump_filename_str = new char[minidump_filename.length() + 1];
+  minidump_filename.copy(minidump_filename_str, minidump_filename.length());
+  minidump_filename_str[minidump_filename.length()] = '\0';
+  info->filename = minidump_filename_str;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      NewRunnableMethod(this,
+                        &CrashHandlerHostLinux::QueueCrashDumpTask,
+                        info,
+                        signal_fd));
+}
+
+void CrashHandlerHostLinux::QueueCrashDumpTask(BreakpadInfo* info,
+                                               int signal_fd) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   // Send the done signal to the process: it can exit now.
-  memset(&msg, 0, sizeof(msg));
+  struct msghdr msg = {0};
   struct iovec done_iov;
   done_iov.iov_base = const_cast<char*>("\x42");
   done_iov.iov_len = 1;
   msg.msg_iov = &done_iov;
   msg.msg_iovlen = 1;
 
   HANDLE_EINTR(sendmsg(signal_fd, &msg, MSG_DONTWAIT | MSG_NOSIGNAL));
   HANDLE_EINTR(close(signal_fd));
 
-  // Sanitize the string data a bit more
-  guid[kGuidSize] = crash_url[kMaxActiveURLSize] = distro[kDistroSize] = 0;
-
-  BreakpadInfo* info = new BreakpadInfo;
-
-  char* minidump_filename_str = new char[minidump_filename.length() + 1];
-  minidump_filename.copy(minidump_filename_str, minidump_filename.length());
-  minidump_filename_str[minidump_filename.length()] = '\0';
-  info->filename = minidump_filename_str;
-
-  info->process_type_length = process_type_.length();
-  char* process_type_str = new char[info->process_type_length + 1];
-  process_type_.copy(process_type_str, info->process_type_length);
-  process_type_str[info->process_type_length] = '\0';
-  info->process_type = process_type_str;
-
-  info->crash_url_length = strlen(crash_url);
-  info->crash_url = crash_url;
-
-  info->guid_length = strlen(guid);
-  info->guid = guid;
-
-  info->distro_length = strlen(distro);
-  info->distro = distro;
-
-  info->upload = upload;
-  info->process_start_time = uptime;
-
   uploader_thread_->message_loop()->PostTask(
       FROM_HERE,
       NewRunnableFunction(&CrashDumpTask, this, info));
 }
 
 void CrashHandlerHostLinux::WillDestroyCurrentMessageLoop() {
   file_descriptor_watcher_.StopWatchingFileDescriptor();
 
   // If we are quitting and there are crash dumps in the queue, turn them into
   // no-ops.
@@ skipping 45 matching lines @@
 }
 
 void RendererCrashHandlerHostLinux::SetProcessType() {
   process_type_ = "renderer";
 }
 
 // static
 RendererCrashHandlerHostLinux* RendererCrashHandlerHostLinux::GetInstance() {
   return Singleton<RendererCrashHandlerHostLinux>::get();
 }