Linux: Improve message handling in CrashHandlerHostLinux to prevent file descriptor leaks.

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/files/scoped_file.h"
 #include "base/format_macros.h"
 #include "base/linux_util.h"
 #include "base/logging.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"
@@ skipping 161 matching lines @@
   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) {
+  if (msg_size < 0) {
     LOG(ERROR) << "Error reading from death signal socket. Crash dumping"
                << " is disabled."
                << " msg_size:" << msg_size
                << " errno:" << errno;
     file_descriptor_watcher_.StopWatchingFileDescriptor();
     return;
   }
+  const bool bad_message = (msg_size != expected_msg_size ||
+                            msg.msg_controllen != kControlMsgSize ||
+                            msg.msg_flags & ~MSG_TRUNC);
+  base::ScopedFD signal_fd;
+  pid_t crashing_pid = -1;
+  if (msg.msg_controllen > 0) {
+    // Walk the control payload and extract the file descriptor and
+    // validated pid.
+    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 size_t len = hdr->cmsg_len -
+            (((uint8_t*)CMSG_DATA(hdr)) - (uint8_t*)hdr);
+        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 (size_t i = 0; i < num_fds; ++i)
+            close(reinterpret_cast<int*>(CMSG_DATA(hdr))[i]);
+          return;
+        } else {

[mdempsky, 2014/06/05 21:53:58]

(Since the above "if" condition ends in a "return;", I'd probably remove the
"else" here and unblock/unindent the following code.)

[Lei Zhang, 2014/06/05 21:57:23]

Done.

+          DCHECK(!signal_fd.is_valid());
+          int fd = reinterpret_cast<int*>(CMSG_DATA(hdr))[0];
+          if (fd >= 0) {

[mdempsky, 2014/06/05 21:53:58]

You can probably DCHECK this?  The kernel should never give us a negative fd.

[Lei Zhang, 2014/06/05 21:57:23]

Done.

+            signal_fd.reset(fd);
+          }
+        }
+      } else if (hdr->cmsg_type == SCM_CREDENTIALS) {
+        DCHECK_EQ(-1, crashing_pid);
+        const struct ucred *cred =
+            reinterpret_cast<struct ucred*>(CMSG_DATA(hdr));
+        crashing_pid = cred->pid;
+      }
+    }
+  }
 
-  if (msg.msg_controllen != kControlMsgSize ||
-      msg.msg_flags & ~MSG_TRUNC) {
+  if (bad_message) {
     LOG(ERROR) << "Received death signal message with the wrong size;"
                << " msg.msg_controllen:" << msg.msg_controllen
                << " msg.msg_flags:" << msg.msg_flags
                << " kCrashContextSize:" << kCrashContextSize
                << " kControlMsgSize:" << kControlMsgSize;
     return;
   }
-
-  // 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 size_t len = hdr->cmsg_len -
-          (((uint8_t*)CMSG_DATA(hdr)) - (uint8_t*)hdr);
-      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 (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;
-    }
-  }
-
-  if (crashing_pid == -1 || signal_fd == -1) {
+  if (crashing_pid == -1 || !signal_fd.is_valid()) {
     LOG(ERROR) << "Death signal message didn't contain all expected control"
                << " messages";
-    if (signal_fd >= 0)
-      close(signal_fd);
     return;
   }
 
   // The crashing TID set inside the compromised context via
   // sys_gettid() in ExceptionHandler::HandleSignal might be wrong (if
   // the kernel supports PID namespacing) and may need to be
   // translated.
   //
   // We expect the crashing thread to be in sys_read(), waiting for us to
   // write to |signal_fd|. Most newer kernels where we have the different pid
@@ skipping 57 matching lines @@
 
 
   BrowserThread::GetBlockingPool()->PostSequencedWorkerTask(
       worker_pool_token_,
       FROM_HERE,
       base::Bind(&CrashHandlerHostLinux::WriteDumpFile,
                  base::Unretained(this),
                  base::Passed(&info),
                  base::Passed(&crash_context),
                  crashing_pid,
-                 signal_fd));
+                 signal_fd.release()));
 }
 
 void CrashHandlerHostLinux::WriteDumpFile(scoped_ptr<BreakpadInfo> info,
                                           scoped_ptr<char[]> crash_context,
                                           pid_t crashing_pid,
                                           int signal_fd) {
   DCHECK(BrowserThread::GetBlockingPool()->IsRunningSequenceOnCurrentThread(
       worker_pool_token_));
 
   // Set |info->distro| here because base::GetLinuxDistro() needs to run on a
@@ skipping 82 matching lines @@
   // no-ops.
   shutting_down_ = true;
   uploader_thread_->Stop();
 }
 
 bool CrashHandlerHostLinux::IsShuttingDown() const {
   return shutting_down_;
 }
 
 }  // namespace breakpad