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

components/breakpad/app/breakpad_linux.cc

Index: components/breakpad/app/breakpad_linux.cc
===================================================================
--- components/breakpad/app/breakpad_linux.cc	(revision 265558)
+++ components/breakpad/app/breakpad_linux.cc	(working copy)
@@ -297,6 +297,7 @@
 
   const char* const mime_boundary_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(MimeWriter);
 };
 
@@ -403,8 +404,7 @@
 // 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);
 
@@ -532,6 +532,10 @@
 #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
@@ -706,8 +710,8 @@
   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.
@@ -1020,7 +1024,106 @@
   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.
@@ -1205,6 +1308,7 @@
   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;
 
@@ -1410,53 +1514,13 @@
       // 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);