Breakpad Linux: Prevent some memory leaks. (Try 2)

components/breakpad/browser/crash_handler_host_linux.cc

 // Copyright 2013 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 "components/breakpad/browser/crash_handler_host_linux.h"
 
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/socket.h>
 #include <sys/syscall.h>
 #include <unistd.h>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/format_macros.h"
 #include "base/linux_util.h"
 #include "base/logging.h"
-#include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/rand_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.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 "components/breakpad/app/breakpad_linux_impl.h"
 #include "content/public/browser/browser_thread.h"
 
 #if defined(OS_ANDROID)
 #include <sys/linux-syscalls.h>
 
 #define SYS_read __NR_read
 #endif
 
 using content::BrowserThread;
 using google_breakpad::ExceptionHandler;
 
 namespace breakpad {
 
 namespace {
 
+const size_t kNumFDs = 1;
 // The length of the control message:
-const unsigned kControlMsgSize =
-    CMSG_SPACE(sizeof(int)) + CMSG_SPACE(sizeof(struct ucred));
+const size_t kControlMsgSize =
+    CMSG_SPACE(kNumFDs * sizeof(int)) + CMSG_SPACE(sizeof(struct ucred));
 // The length of the regular payload:
-const unsigned kCrashContextSize = sizeof(ExceptionHandler::CrashContext);
+const size_t kCrashContextSize = sizeof(ExceptionHandler::CrashContext);
 
 // Handles the crash dump and frees the allocated BreakpadInfo struct.
-void CrashDumpTask(CrashHandlerHostLinux* handler, BreakpadInfo* info) {
+void CrashDumpTask(CrashHandlerHostLinux* handler,
+                   scoped_ptr<BreakpadInfo> info) {
   if (handler->IsShuttingDown() && info->upload) {
     base::DeleteFile(base::FilePath(info->filename), false);
 #if defined(ADDRESS_SANITIZER)
     base::DeleteFile(base::FilePath(info->log_filename), false);
 #endif
     return;
   }
 
   HandleCrashDump(*info);
   delete[] info->filename;
 #if defined(ADDRESS_SANITIZER)
   delete[] info->log_filename;
+  delete[] info->asan_report_str;
 #endif
   delete[] info->process_type;
   delete[] info->distro;
   delete info->crash_keys;
-  delete info;
 }
 
 }  // namespace
 
 // Since instances of CrashHandlerHostLinux are leaked, they are only destroyed
 // at the end of the processes lifetime, which is greater in span than the
 // lifetime of the IO message loop. Thus, all calls to base::Bind() use
 // non-refcounted pointers.
 
 CrashHandlerHostLinux::CrashHandlerHostLinux(const std::string& process_type,
                                              const base::FilePath& dumps_path,
                                              bool upload)
     : process_type_(process_type),
       dumps_path_(dumps_path),
       upload_(upload),
       shutting_down_(false),
       worker_pool_token_(BrowserThread::GetBlockingPool()->GetSequenceToken()) {
   int fds[2];
   // We use SOCK_SEQPACKET rather than SOCK_DGRAM to prevent the process from
   // sending datagrams to other sockets on the system. The sandbox may prevent
   // the process from calling socket() to create new sockets, but it'll still
   // inherit some sockets. With PF_UNIX+SOCK_DGRAM, it can call sendmsg to send
   // a datagram to any (abstract) socket on the same system. With
   // SOCK_SEQPACKET, this is prevented.
-  CHECK_EQ(socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds), 0);
+  CHECK_EQ(0, socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds));
   static const int on = 1;
 
   // Enable passcred on the server end of the socket
-  CHECK_EQ(setsockopt(fds[1], SOL_SOCKET, SO_PASSCRED, &on, sizeof(on)), 0);
+  CHECK_EQ(0, setsockopt(fds[1], SOL_SOCKET, SO_PASSCRED, &on, sizeof(on)));
 
   process_socket_ = fds[0];
   browser_socket_ = fds[1];
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&CrashHandlerHostLinux::Init, base::Unretained(this)));
 }
 
 CrashHandlerHostLinux::~CrashHandlerHostLinux() {
@@ skipping 14 matching lines @@
       base::MessageLoopForIO::WATCH_READ,
       &file_descriptor_watcher_, this));
   ml->AddDestructionObserver(this);
 }
 
 void CrashHandlerHostLinux::OnFileCanWriteWithoutBlocking(int fd) {
   NOTREACHED();
 }
 
 void CrashHandlerHostLinux::OnFileCanReadWithoutBlocking(int fd) {
-  DCHECK_EQ(fd, browser_socket_);
+  DCHECK_EQ(browser_socket_, fd);
 
   // 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 message sender is in components/breakpad/app/breakpad_linux.cc.
 
   struct msghdr msg = {0};
   struct iovec iov[kCrashIovSize];
 
-  // Freed in WriteDumpFile();
-  char* crash_context = new char[kCrashContextSize];
-  // Freed in CrashDumpTask();
-  char* distro = new char[kDistroSize + 1];
+  scoped_ptr<char[]> crash_context(new char[kCrashContextSize]);
 #if defined(ADDRESS_SANITIZER)
-  asan_report_str_ = new char[kMaxAsanReportSize + 1];
+  scoped_ptr<char[]> asan_report(new char[kMaxAsanReportSize + 1]);
 #endif
 
-  // Freed in CrashDumpTask().
-  CrashKeyStorage* crash_keys = new CrashKeyStorage;
+  scoped_ptr<CrashKeyStorage> crash_keys(new CrashKeyStorage);
   google_breakpad::SerializedNonAllocatingMap* serialized_crash_keys;
   size_t crash_keys_size = crash_keys->Serialize(
       const_cast<const google_breakpad::SerializedNonAllocatingMap**>(
           &serialized_crash_keys));
 
   char* tid_buf_addr = NULL;
   int tid_fd = -1;
   uint64_t uptime;
   size_t oom_size;
   char control[kControlMsgSize];
   const ssize_t expected_msg_size =
       kCrashContextSize +
-      kDistroSize + 1 +
       sizeof(tid_buf_addr) + sizeof(tid_fd) +
       sizeof(uptime) +
 #if defined(ADDRESS_SANITIZER)
       kMaxAsanReportSize + 1 +
 #endif
       sizeof(oom_size) +
       crash_keys_size;
-  iov[0].iov_base = crash_context;
+  iov[0].iov_base = crash_context.get();
   iov[0].iov_len = kCrashContextSize;
-  iov[1].iov_base = distro;
-  iov[1].iov_len = kDistroSize + 1;
-  iov[2].iov_base = &tid_buf_addr;
-  iov[2].iov_len = sizeof(tid_buf_addr);
-  iov[3].iov_base = &tid_fd;
-  iov[3].iov_len = sizeof(tid_fd);
-  iov[4].iov_base = &uptime;
-  iov[4].iov_len = sizeof(uptime);
-  iov[5].iov_base = &oom_size;
-  iov[5].iov_len = sizeof(oom_size);
-  iov[6].iov_base = serialized_crash_keys;
-  iov[6].iov_len = crash_keys_size;
-#if defined(ADDRESS_SANITIZER)
-  iov[7].iov_base = asan_report_str_;
-  iov[7].iov_len = kMaxAsanReportSize + 1;
+  iov[1].iov_base = &tid_buf_addr;
+  iov[1].iov_len = sizeof(tid_buf_addr);
+  iov[2].iov_base = &tid_fd;
+  iov[2].iov_len = sizeof(tid_fd);
+  iov[3].iov_base = &uptime;
+  iov[3].iov_len = sizeof(uptime);
+  iov[4].iov_base = &oom_size;
+  iov[4].iov_len = sizeof(oom_size);
+  iov[5].iov_base = serialized_crash_keys;
+  iov[5].iov_len = crash_keys_size;
+#if !defined(ADDRESS_SANITIZER)
+  COMPILE_ASSERT(5 == kCrashIovSize - 1, Incorrect_Number_Of_Iovec_Members);
+#else
+  iov[6].iov_base = asan_report.get();
+  iov[6].iov_len = kMaxAsanReportSize + 1;
+  COMPILE_ASSERT(6 == kCrashIovSize - 1, Incorrect_Number_Of_Iovec_Members);
 #endif
   msg.msg_iov = iov;
   msg.msg_iovlen = kCrashIovSize;
   msg.msg_control = control;
   msg.msg_controllen = kControlMsgSize;
 
   const ssize_t msg_size = HANDLE_EINTR(recvmsg(browser_socket_, &msg, 0));
   if (msg_size != expected_msg_size) {
     LOG(ERROR) << "Error reading from death signal socket. Crash dumping"
                << " is disabled."
@@ skipping 14 matching lines @@
   }
 
   // Walk the control payload an extract the file descriptor and validated pid.
   pid_t crashing_pid = -1;
   int signal_fd = -1;
   for (struct cmsghdr *hdr = CMSG_FIRSTHDR(&msg); hdr;
        hdr = CMSG_NXTHDR(&msg, hdr)) {
     if (hdr->cmsg_level != SOL_SOCKET)
       continue;
     if (hdr->cmsg_type == SCM_RIGHTS) {
-      const unsigned len = hdr->cmsg_len -
+      const size_t len = hdr->cmsg_len -
           (((uint8_t*)CMSG_DATA(hdr)) - (uint8_t*)hdr);
-      DCHECK_EQ(len % sizeof(int), 0u);
-      const unsigned num_fds = len / sizeof(int);
-      if (num_fds != 1) {
+      DCHECK_EQ(0U, len % sizeof(int));
+      const size_t num_fds = len / sizeof(int);
+      if (num_fds != kNumFDs) {
         // A nasty process could try and send us too many descriptors and
         // force a leak.
         LOG(ERROR) << "Death signal contained wrong number of descriptors;"
                    << " num_fds:" << num_fds;
-        for (unsigned i = 0; i < num_fds; ++i)
+        for (size_t i = 0; i < num_fds; ++i)
           close(reinterpret_cast<int*>(CMSG_DATA(hdr))[i]);
         return;
       } else {
         signal_fd = reinterpret_cast<int*>(CMSG_DATA(hdr))[0];
       }
     } else if (hdr->cmsg_type == SCM_CREDENTIALS) {
       const struct ucred *cred =
           reinterpret_cast<struct ucred*>(CMSG_DATA(hdr));
       crashing_pid = cred->pid;
     }
@@ skipping 35 matching lines @@
     // guess here and assume the crashing thread is the thread group
     // leader.  If procfs syscall is not supported by the kernel, then
     // we assume the kernel also does not support TID namespacing and
     // trust the TID passed by the crashing process.
     LOG(WARNING) << "Could not translate tid - assuming crashing thread is "
         "thread group leader; syscall_supported=" << syscall_supported;
     crashing_tid = crashing_pid;
   }
 
   ExceptionHandler::CrashContext* bad_context =
-      reinterpret_cast<ExceptionHandler::CrashContext*>(crash_context);
+      reinterpret_cast<ExceptionHandler::CrashContext*>(crash_context.get());
   bad_context->tid = crashing_tid;
 
-  // Freed in CrashDumpTask();
-  BreakpadInfo* info = new BreakpadInfo;
+  scoped_ptr<BreakpadInfo> info(new BreakpadInfo);
 
   info->fd = -1;
   info->process_type_length = process_type_.length();
+  // Freed in CrashDumpTask().
   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->distro_length = strlen(distro);
-  info->distro = distro;
+  std::string distro = base::GetLinuxDistro();

[jochen (gone - plz use gerrit), 2014/05/22 10:52:28]

this method crashes in thread restrictions :-/

+  info->distro_length = distro.length();
+  // Freed in CrashDumpTask().
+  char* distro_str = new char[info->distro_length + 1];
+  distro.copy(distro_str, info->distro_length);
+  distro_str[info->distro_length] = '\0';
+  info->distro = distro_str;
+
+  // Memory released from scoped_ptrs below are also freed in CrashDumpTask().
+  info->crash_keys = crash_keys.release();
+#if defined(ADDRESS_SANITIZER)
+  asan_report[kMaxAsanReportSize] = '\0';
+  info->asan_report_str = asan_report.release();
+  info->asan_report_length = strlen(info->asan_report_str);
+#endif
+
+  info->process_start_time = uptime;
+  info->oom_size = oom_size;
 #if defined(OS_ANDROID)
   // Nothing gets uploaded in android.
   info->upload = false;
 #else
   info->upload = upload_;
 #endif
 
-  info->crash_keys = crash_keys;
-
-#if defined(ADDRESS_SANITIZER)
-  info->asan_report_str = asan_report_str_;
-  info->asan_report_length = strlen(asan_report_str_);
-#endif
-  info->process_start_time = uptime;
-  info->oom_size = oom_size;
 
   BrowserThread::GetBlockingPool()->PostSequencedWorkerTask(
       worker_pool_token_,
       FROM_HERE,
       base::Bind(&CrashHandlerHostLinux::WriteDumpFile,
                  base::Unretained(this),
-                 info,
+                 base::Passed(&info),
+                 base::Passed(&crash_context),
                  crashing_pid,
-                 crash_context,
                  signal_fd));
 }
 
-void CrashHandlerHostLinux::WriteDumpFile(BreakpadInfo* info,
+void CrashHandlerHostLinux::WriteDumpFile(scoped_ptr<BreakpadInfo> info,
+                                          scoped_ptr<char[]> crash_context,
                                           pid_t crashing_pid,
-                                          char* crash_context,
                                           int signal_fd) {
   DCHECK(BrowserThread::GetBlockingPool()->IsRunningSequenceOnCurrentThread(
       worker_pool_token_));
 
   base::FilePath dumps_path("/tmp");
   PathService::Get(base::DIR_TEMP, &dumps_path);
   if (!info->upload)
     dumps_path = dumps_path_;
-  const uint64 rand = base::RandUint64();
   const std::string minidump_filename =
       base::StringPrintf("%s/chromium-%s-minidump-%016" PRIx64 ".dmp",
                          dumps_path.value().c_str(),
                          process_type_.c_str(),
-                         rand);
+                         base::RandUint64());
 
   if (!google_breakpad::WriteMinidump(minidump_filename.c_str(),
                                       kMaxMinidumpFileSize,
-                                      crashing_pid, crash_context,
+                                      crashing_pid,
+                                      crash_context.get(),
                                       kCrashContextSize,
                                       google_breakpad::MappingList(),
                                       google_breakpad::AppMemoryList())) {
     LOG(ERROR) << "Failed to write crash dump for pid " << crashing_pid;
   }
 #if defined(ADDRESS_SANITIZER)
   // Create a temporary file holding the AddressSanitizer report.
-  const std::string log_filename =
-      base::StringPrintf("%s/chromium-%s-minidump-%016" PRIx64 ".log",
-                         dumps_path.value().c_str(),
-                         process_type_.c_str(),
-                         rand);
-  FILE* logfile = fopen(log_filename.c_str(), "w");
-  CHECK(logfile);
-  fprintf(logfile, "%s", asan_report_str_);
-  fclose(logfile);
+  const base::FilePath log_path =
+      base::FilePath(minidump_filename).ReplaceExtension("log");
+  base::WriteFile(log_path, info->asan_report_str, info->asan_report_length);
 #endif
 
-  delete[] crash_context;
-
-  // Freed in CrashDumpTask();
+  // Freed in CrashDumpTask().
   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;
 #if defined(ADDRESS_SANITIZER)
+  // Freed in CrashDumpTask().
   char* minidump_log_filename_str = new char[minidump_filename.length() + 1];
   minidump_filename.copy(minidump_log_filename_str, minidump_filename.length());
   memcpy(minidump_log_filename_str + minidump_filename.length() - 3, "log", 3);
   minidump_log_filename_str[minidump_filename.length()] = '\0';
   info->log_filename = minidump_log_filename_str;
 #endif
   info->pid = crashing_pid;
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&CrashHandlerHostLinux::QueueCrashDumpTask,
                  base::Unretained(this),
-                 info,
+                 base::Passed(&info),
                  signal_fd));
 }
 
-void CrashHandlerHostLinux::QueueCrashDumpTask(BreakpadInfo* info,
+void CrashHandlerHostLinux::QueueCrashDumpTask(scoped_ptr<BreakpadInfo> info,
                                                int signal_fd) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   // Send the done signal to the process: it can exit now.
   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;
 
-  (void) HANDLE_EINTR(sendmsg(signal_fd, &msg, MSG_DONTWAIT | MSG_NOSIGNAL));
+  HANDLE_EINTR(sendmsg(signal_fd, &msg, MSG_DONTWAIT | MSG_NOSIGNAL));
   close(signal_fd);
 
   uploader_thread_->message_loop()->PostTask(
       FROM_HERE,
-      base::Bind(&CrashDumpTask, base::Unretained(this), info));
+      base::Bind(&CrashDumpTask, base::Unretained(this), base::Passed(&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.
   shutting_down_ = true;
   uploader_thread_->Stop();
 }
 
 bool CrashHandlerHostLinux::IsShuttingDown() const {
   return shutting_down_;
 }
 
 }  // namespace breakpad