Linux Breakpad: Keep reading from the crash upload progress until a complete crash id has been rece…

components/breakpad/app/breakpad_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.
 
 // For linux_syscall_support.h. This makes it safe to call embedded system
 // calls when in seccomp mode.
 
 #include "components/breakpad/app/breakpad_linux.h"
 
 #include <fcntl.h>
@@ skipping 279 matching lines @@
   void AddItemWithoutTrailingSpaces(const void* base, size_t size);
 
   struct kernel_iovec iov_[kIovCapacity];
   int iov_index_;
 
   // Output file descriptor.
   int fd_;
 
   const char* const mime_boundary_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(MimeWriter);
 };
 
 MimeWriter::MimeWriter(int fd, const char* const mime_boundary)
     : iov_index_(0),
       fd_(fd),
       mime_boundary_(mime_boundary) {
 }
 
 MimeWriter::~MimeWriter() {
@@ skipping 86 matching lines @@
 
 void MimeWriter::AddItemWithoutTrailingSpaces(const void* base, size_t size) {
   AddItem(base, LengthWithoutTrailingSpaces(static_cast<const char*>(base),
                                             size));
 }
 
 #if defined(OS_CHROMEOS)
 // This subclass is used on Chromium OS to report crashes in a format easy for
 // the central crash reporting facility to understand.
 // Format is <name>:<data length in decimal>:<data>
-class CrashReporterWriter : public MimeWriter
-{
+class CrashReporterWriter : public MimeWriter {
  public:
   explicit CrashReporterWriter(int fd);
 
   virtual void AddBoundary() OVERRIDE;
 
   virtual void AddEnd() OVERRIDE;
 
   virtual void AddPairData(const char* msg_type,
                            size_t msg_type_size,
                           const char* msg_data,
@@ skipping 107 matching lines @@
 #endif
 
 size_t WriteLog(const char* buf, size_t nbytes) {
 #if defined(OS_ANDROID)
   return __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, buf);
 #else
   return sys_write(2, buf, nbytes);
 #endif
 }
 
+size_t WriteNewline() {
+  return WriteLog("\n", 1);
+}
+
 #if defined(OS_ANDROID)
 // Android's native crash handler outputs a diagnostic tombstone to the device
 // log. By returning false from the HandlerCallbacks, breakpad will reinstall
 // the previous (i.e. native) signal handlers before returning from its own
 // handler. A Chrome build fingerprint is written to the log, so that the
 // specific build of Chrome and the location of the archived Chrome symbols can
 // be determined directly from it.
 bool FinalizeCrashDoneAndroid() {
   base::android::BuildInfo* android_build_info =
       base::android::BuildInfo::GetInstance();
@@ skipping 154 matching lines @@
   info.distro_length = distro_length;
   info.upload = false;
   info.process_start_time = g_process_start_time;
   info.pid = g_pid;
   info.crash_keys = g_crash_keys;
   HandleCrashDump(info);
   bool finalize_result = FinalizeCrashDoneAndroid();
   base::android::BuildInfo* android_build_info =
       base::android::BuildInfo::GetInstance();
   if (android_build_info->sdk_int() >= 18 &&
-      strcmp(android_build_info->build_type(), "eng") != 0 &&
-      strcmp(android_build_info->build_type(), "userdebug") != 0) {
+      my_strcmp(android_build_info->build_type(), "eng") != 0 &&
+      my_strcmp(android_build_info->build_type(), "userdebug") != 0) {
     // On JB MR2 and later, the system crash handler displays a dialog. For
     // renderer crashes, this is a bad user experience and so this is disabled
     // for user builds of Android.
     // TODO(cjhopman): There should be some way to recover the crash stack from
     // non-uploading user clients. See http://crbug.com/273706.
     __android_log_write(ANDROID_LOG_WARN,
                         kGoogleBreakpad,
                         "Tombstones are disabled on JB MR2+ user builds.");
     __android_log_write(ANDROID_LOG_WARN,
                         kGoogleBreakpad,
@@ skipping 292 matching lines @@
     NULL,
   };
   static const char msg[] = "Cannot upload crash dump: cannot exec "
                             "/usr/bin/wget\n";
 #endif
   execve(args[0], const_cast<char**>(args), environ);
   WriteLog(msg, sizeof(msg) - 1);
   sys__exit(1);
 }
 
+// Runs in the helper process to wait for the upload process running
+// ExecUploadProcessOrTerminate() to finish. Returns the number of bytes written
+// to |fd| and save the written contents to |buf|.
+// |buf| needs to be big enough to hold |bytes_to_read| + 1 characters.
+size_t WaitForCrashReportUploadProcess(int fd, size_t bytes_to_read,
+                                       char* buf) {
+  size_t bytes_read = 0;
+
+  // Upload should finish in about 10 seconds. Add a few more 500 ms
+  // internals to account for process startup time.
+  for (size_t wait_count = 0; wait_count < 24; ++wait_count) {
+    struct kernel_pollfd poll_fd;
+    poll_fd.fd = fd;
+    poll_fd.events = POLLIN | POLLPRI | POLLERR;
+    int ret = sys_poll(&poll_fd, 1, 500);
+    if (ret < 0) {
+      // Error
+      break;
+    } else if (ret > 0) {
+      // There is data to read.
+      ssize_t len = HANDLE_EINTR(
+          sys_read(fd, buf + bytes_read, bytes_to_read - bytes_read));
+      if (len < 0)
+        break;
+      bytes_read += len;
+      if (bytes_read == bytes_to_read)
+        break;
+    }
+    // |ret| == 0 -> timed out, continue waiting.
+    // or |bytes_read| < |bytes_to_read| still, keep reading.
+  }
+  buf[bytes_to_read] = 0;  // Always NULL terminate the buffer.

[vapier, 2014/04/24 00:47:39]

you NUL terminate the buffer ;)

[Lei Zhang, 2014/04/24 01:30:47]

Done.

+  return bytes_read;
+}
+
+// |buf| should be |expected_len| + 1 characters in size and NULL terminated.
+bool IsValidCrashReportId(const char* buf, size_t bytes_read,
+                          size_t expected_len) {
+  if (bytes_read != expected_len)
+    return false;
+#if defined(OS_CHROMEOS)
+  return my_strcmp(buf, "_sys_cr_finished") == 0;
+#else
+  for (size_t i = 0; i <= bytes_read; ++i) {
+    char c = buf[i];
+    if ((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f'))

[vapier, 2014/04/24 00:47:39]

i was suggesting ctypes also to avoid case issues.  but you can handle that with
0x20:

 bool my_isxdigit(char c) {
   return (c >= '0' && c <= '9') || ((c | 0x20) >= 'a' && (c | 0x20) <= 'f');
 }

[Lei Zhang, 2014/04/24 01:30:47]

Done. It's all lower case right now, but why not handle it?

+      continue;
+    return false;
+  }
+  return true;
+#endif
+}
+
+// |buf| should be |expected_len| + 1 characters in size and NULL terminated.
+void HandleCrashReportId(const char* buf, size_t bytes_read,
+                         size_t expected_len) {
+  WriteNewline();
+  if (!IsValidCrashReportId(buf, bytes_read, expected_len)) {
+#if defined(OS_CHROMEOS)
+    static const char msg[] = "Crash_reporter failed to process crash report";
+#else
+    static const char msg[] = "Failed to get crash dump id.";
+#endif
+    WriteLog(msg, sizeof(msg) - 1);
+    WriteNewline();

[vapier, 2014/04/24 00:47:39]

why not embed the newline ?  you do below ...

[Lei Zhang, 2014/04/24 01:30:47]

WriteLog() below is a bit more complicated. There's a common path here.

+    return;
+  }
+
+#if defined(OS_CHROMEOS)
+  static const char msg[] = "Crash dump received by crash_reporter\n";
+  WriteLog(msg, sizeof(msg) - 1);
+#else
+  // Write crash dump id to stderr.
+  static const char msg[] = "Crash dump id: ";
+  WriteLog(msg, sizeof(msg) - 1);
+  WriteLog(buf, my_strlen(buf));
+  WriteNewline();
+
+  // Write crash dump id to crash log as: seconds_since_epoch,crash_id
+  struct kernel_timeval tv;
+  if (g_crash_log_path && !sys_gettimeofday(&tv, NULL)) {
+    uint64_t time = kernel_timeval_to_ms(&tv) / 1000;
+    char time_str[kUint64StringSize];
+    const unsigned time_len = my_uint64_len(time);
+    my_uint64tos(time_str, time, time_len);
+
+    const int kLogOpenFlags = O_CREAT | O_WRONLY | O_APPEND;

[vapier, 2014/04/24 00:47:39]

in general, any reason to not use O_CLOEXEC ?

in old kernels, it'll be silently ignored.

[Lei Zhang, 2014/04/24 01:30:47]

We probably wrote this when it was still brand new. Does this matter here? This
is in the helper process which never execs.

[vapier, 2014/04/24 02:16:48]

it frequently doesn't matter, but O_CLOEXEC is a defensive thing.  we start off
not expecting many scenarios, but code paths change over time :).

+    int log_fd = sys_open(g_crash_log_path, kLogOpenFlags, 0600);
+    if (log_fd > 0) {
+      sys_write(log_fd, time_str, time_len);
+      sys_write(log_fd, ",", 1);
+      sys_write(log_fd, buf, my_strlen(buf));
+      sys_write(log_fd, "\n", 1);
+      IGNORE_RET(sys_close(log_fd));
+    }
+  }
+#endif
+}
+
 #if defined(OS_CHROMEOS)
 const char* GetCrashingProcessName(const BreakpadInfo& info,
                                    google_breakpad::PageAllocator* allocator) {
   // Symlink to process binary is at /proc/###/exe.
   char linkpath[kUint64StringSize + sizeof("/proc/") + sizeof("/exe")] =
     "/proc/";
   uint64_t pid_value_len = my_uint64_len(info.pid);
   my_uint64tos(linkpath + sizeof("/proc/") - 1, info.pid, pid_value_len);
   linkpath[sizeof("/proc/") - 1 + pid_value_len] = '\0';
   my_strlcat(linkpath, "/exe", sizeof(linkpath));
@@ skipping 165 matching lines @@
   //   Content-Type: application/octet-stream \r\n \r\n
   //   <dump contents>
   //   \r\n BOUNDARY -- \r\n
 
 #if defined(OS_CHROMEOS)
   CrashReporterWriter writer(temp_file_fd);
 #else
   MimeWriter writer(temp_file_fd, mime_boundary);
 #endif
   {
+    // TODO(thestig) Do not use this inside a compromised context.
     std::string product_name;
     std::string version;
 
     GetBreakpadClient()->GetProductNameAndVersion(&product_name, &version);
 
     writer.AddBoundary();
     writer.AddPairString("prod", product_name.c_str());
     writer.AddBoundary();
     writer.AddPairString("ver", version.c_str());
     writer.AddBoundary();
@@ skipping 185 matching lines @@
         // Upload process.
         IGNORE_RET(sys_close(fds[0]));
         IGNORE_RET(sys_dup2(fds[1], 3));
         ExecUploadProcessOrTerminate(info, temp_file, mime_boundary, exe_buf,
                                      &allocator);
       }
 
       // Helper process.
       if (upload_child > 0) {
         IGNORE_RET(sys_close(fds[1]));
-        char id_buf[17];  // Crash report IDs are expected to be 16 chars.
-        ssize_t len = -1;
-        // Upload should finish in about 10 seconds. Add a few more 500 ms
-        // internals to account for process startup time.
-        for (size_t wait_count = 0; wait_count < 24; ++wait_count) {
-          struct kernel_pollfd poll_fd;
-          poll_fd.fd = fds[0];
-          poll_fd.events = POLLIN | POLLPRI | POLLERR;
-          int ret = sys_poll(&poll_fd, 1, 500);
-          if (ret < 0) {
-            // Error
-            break;
-          } else if (ret > 0) {
-            // There is data to read.
-            len = HANDLE_EINTR(sys_read(fds[0], id_buf, sizeof(id_buf) - 1));
-            break;
-          }
-          // ret == 0 -> timed out, continue waiting.
-        }
-        if (len > 0) {
-          // Write crash dump id to stderr.
-          id_buf[len] = 0;
-          static const char msg[] = "\nCrash dump id: ";
-          WriteLog(msg, sizeof(msg) - 1);
-          WriteLog(id_buf, my_strlen(id_buf));
-          WriteLog("\n", 1);
 
-          // Write crash dump id to crash log as: seconds_since_epoch,crash_id
-          struct kernel_timeval tv;
-          if (g_crash_log_path && !sys_gettimeofday(&tv, NULL)) {
-            uint64_t time = kernel_timeval_to_ms(&tv) / 1000;
-            char time_str[kUint64StringSize];
-            const unsigned time_len = my_uint64_len(time);
-            my_uint64tos(time_str, time, time_len);
+        const size_t kCrashIdLength = 16;
+        char id_buf[kCrashIdLength + 1];
+        size_t bytes_read =
+            WaitForCrashReportUploadProcess(fds[0], kCrashIdLength, id_buf);
+        HandleCrashReportId(id_buf, bytes_read, kCrashIdLength);
 
-            int log_fd = sys_open(g_crash_log_path,
-                                  O_CREAT | O_WRONLY | O_APPEND,
-                                  0600);
-            if (log_fd > 0) {
-              sys_write(log_fd, time_str, time_len);
-              sys_write(log_fd, ",", 1);
-              sys_write(log_fd, id_buf, my_strlen(id_buf));
-              sys_write(log_fd, "\n", 1);
-              IGNORE_RET(sys_close(log_fd));
-            }
-          }
-        }
         if (sys_waitpid(upload_child, NULL, WNOHANG) == 0) {
           // Upload process is still around, kill it.
           sys_kill(upload_child, SIGKILL);
         }
       }
     }
 
     // Helper process.
     IGNORE_RET(sys_unlink(info.filename));
 #if defined(ADDRESS_SANITIZER)
@@ skipping 75 matching lines @@
     }
   }
 }
 #endif  // OS_ANDROID
 
 bool IsCrashReporterEnabled() {
   return g_is_crash_reporter_enabled;
 }
 
 }  // namespace breakpad