breakpad: Fix crash_reporter upload success handling on Chrome OS

components/crash/content/app/breakpad_linux.cc

Index: components/crash/content/app/breakpad_linux.cc
diff --git a/components/crash/content/app/breakpad_linux.cc b/components/crash/content/app/breakpad_linux.cc
index 6e5058e9563ebfbe932835c763dc04712d682979..c91a23d252ec8afe766188bd3e37647b1d7377fa 100644
--- a/components/crash/content/app/breakpad_linux.cc
+++ b/components/crash/content/app/breakpad_linux.cc
@@ -100,6 +100,7 @@ ExceptionHandler* g_breakpad = nullptr;
 #if defined(ADDRESS_SANITIZER)
 const char* g_asan_report_str = nullptr;
 #endif
+
 #if defined(OS_ANDROID)
 #define G_DUMPS_SUPPRESSED_MAGIC 0x5AFECEDE
 uint32_t g_dumps_suppressed = 0;
@@ -1157,23 +1158,32 @@ void LoadDataFromFile(google_breakpad::PageAllocator& allocator,
   LoadDataFromFD(allocator, *fd, true, file_data, size);
 }
 
-// Spawn the appropriate upload process for the current OS:
-// - generic Linux invokes wget.
-// - ChromeOS invokes crash_reporter.
+#if defined(OS_CHROMEOS)
+
+// Invoke crash_reporter to notify it of the crash. The actual upload is handled
+// by crash_uploader, which runs on a schedule.
 // |dumpfile| is the path to the dump data file.
-// |mime_boundary| is only used on Linux.
-// |exe_buf| is only used on CrOS and is the crashing process' name.
-void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
+// |exe_buf| is is the crashing process' name.
+// |status_pipe_fd| is the file descriptor of a pipe that will receive the magic
+// crash complete string.
+void UploadWithCrashReporterOrExit(const BreakpadInfo& info,
                                   const char* dumpfile,
-                                  const char* mime_boundary,
                                   const char* exe_buf,
+                                  int uploader_status_fd,
                                   google_breakpad::PageAllocator* allocator) {
-#if defined(OS_CHROMEOS)
   // CrOS uses crash_reporter instead of wget to report crashes,
   // it needs to know where the crash dump lives and the pid and uid of the
   // crashing process.
   static const char kCrashReporterBinary[] = "/sbin/crash_reporter";
 
+  // crash_reporter writes output to stdout. Connect it to the status pipe fd.
+  if (sys_dup2(uploader_status_fd, STDOUT_FILENO) == -1) {
+    const char err[] = "dup2 failed";
+    WriteLog(err, sizeof(err) - 1);
+    // Continue anyway, as crash_report may succeed even if we can't read its
+    // status.
+  }
+
   char pid_buf[kUint64StringSize];
   uint64_t pid_str_length = my_uint64_len(info.pid);
   my_uint64tos(pid_buf, info.pid, pid_str_length);
@@ -1223,7 +1233,31 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
   };
   static const char msg[] = "Cannot upload crash dump: cannot exec "
                             "/sbin/crash_reporter\n";
-#else
+  execve(args[0], const_cast<char**>(args), environ);
+  WriteLog(msg, sizeof(msg) - 1);
+  sys__exit(1);
+}
+
+#else  // defined(OS_CHROMEOS)
+
+// Invoke wget to upload the crash dump.
+// |dumpfile| is the path to the dump data file.
+// |uploader_status_fd| is the file descriptor to which to write the crash id.
+void UploadWithWgetOrExit(const BreakpadInfo& info,
+                                  const char* dumpfile,

[vapier, 2017/02/06 17:17:44]

indent is off here

[James Cook, 2017/02/06 22:09:05]

Deleted this block.

+                                  const char* mime_boundary,
+                                  int uploader_status_fd,
+                                  google_breakpad::PageAllocator* allocator) {
+  // Configure wget to write to file descriptor 3. Do this before other work

[vapier, 2017/02/06 17:17:44]

does it really need to dup ?  can't it write to /dev/fd/%(status_fd)i directly ?

[James Cook, 2017/02/06 22:09:05]

Changed to write to /dev/fd/<foo> directly. I extracted the int-to-string work
into a separate function since it's fiddly and we do it in 3 places.

+  // that might allocate a file descriptor.
+  const int kWgetOutputFd = 3;
+  const char kWgetOutputFdPath[] = "/dev/fd/3";
+  if (sys_dup2(uploader_status_fd, kWgetOutputFd) == -1) {
+    const char err[] = "dup2 failed";
+    WriteLog(err, sizeof(err) - 1);
+    // Continue anyway since wget may succeed even if we can't read its status.
+  }
+
   // Compress |dumpfile| with gzip.
   const pid_t gzip_child = sys_fork();
   if (gzip_child < 0) {
@@ -1303,17 +1337,18 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
     "--timeout=10",  // Set a timeout so we don't hang forever.
     "--tries=1",     // Don't retry if the upload fails.
     "-O",  // output reply to fd 3
-    "/dev/fd/3",
+    kWgetOutputFdPath,
     nullptr,
   };
   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);
 }
 
+#endif  // defined(OS_CHROMEOS)
+
 // 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|.
@@ -1352,9 +1387,13 @@ size_t WaitForCrashReportUploadProcess(int fd, size_t bytes_to_read,
 // |buf| should be |expected_len| + 1 characters in size and nullptr terminated.
 bool IsValidCrashReportId(const char* buf, size_t bytes_read,
                           size_t expected_len) {
-  if (bytes_read != expected_len)
+  if (bytes_read != expected_len) {
+    static const char msg[] = "Unexpected crash report id length\n";
+    WriteLog(msg, sizeof(msg) - 1);
     return false;
+  }
 #if defined(OS_CHROMEOS)
+  // See kSuccessMagic in platform2/crash-reporter/chrome_collector.cc.
   return my_strcmp(buf, "_sys_cr_finished") == 0;
 #else
   for (size_t i = 0; i < bytes_read; ++i) {
@@ -1372,7 +1411,7 @@ void HandleCrashReportId(const char* buf, size_t bytes_read,
   if (!IsValidCrashReportId(buf, bytes_read, expected_len)) {
 #if defined(OS_CHROMEOS)
     static const char msg[] =
-        "System crash-reporter failed to process crash report.";
+        "System crash_reporter failed to process crash report.";
 #else
     static const char msg[] = "Failed to get crash dump id.";
 #endif
@@ -1447,13 +1486,33 @@ const char* GetCrashingProcessName(const BreakpadInfo& info,
 }
 #endif
 
+// Attempts to close all open file descriptors other than stdin, stdout and

[vapier, 2017/02/06 17:17:44]

this move doesn't seem related ?

[James Cook, 2017/02/06 22:09:05]

No, but HandleCrashDump() is 400 lines long, so I'm trying to clean it up a
little bit. I find this file difficult to read.

+// stderr (0, 1, and 2).
+void CloseAllFileDescriptors() {
+  const int fd = sys_open("/proc/self/fd", O_DIRECTORY | O_RDONLY, 0);
+  if (fd < 0) {
+    for (unsigned i = 3; i < 8192; ++i)
+      IGNORE_RET(sys_close(i));
+  } else {
+    google_breakpad::DirectoryReader reader(fd);
+    const char* name;
+    while (reader.GetNextEntry(&name)) {
+      int i;
+      if (my_strtoui(&i, name) && i > 2 && i != fd)
+        IGNORE_RET(sys_close(i));
+      reader.PopEntry();
+    }
+
+    IGNORE_RET(sys_close(fd));
+  }
+}
+
 void HandleCrashDump(const BreakpadInfo& info) {
   int dumpfd;
   bool keep_fd = false;
   size_t dump_size;
   uint8_t* dump_data;
   google_breakpad::PageAllocator allocator;
-  const char* exe_buf = nullptr;
 
   if (GetCrashReporterClient()->HandleCrashDump(info.filename)) {
     return;
@@ -1463,7 +1522,7 @@ void HandleCrashDump(const BreakpadInfo& info) {
   // Grab the crashing process' name now, when it should still be available.
   // If we try to do this later in our grandchild the crashing process has
   // already terminated.
-  exe_buf = GetCrashingProcessName(info, &allocator);
+  const char* exe_buf = GetCrashingProcessName(info, &allocator);
 #endif
 
   if (info.fd != -1) {
@@ -1779,22 +1838,7 @@ void HandleCrashDump(const BreakpadInfo& info) {
     // hold them open for too long.
     //
     // Thus, we have to loop and try and close everything.
-    const int fd = sys_open("/proc/self/fd", O_DIRECTORY | O_RDONLY, 0);
-    if (fd < 0) {
-      for (unsigned i = 3; i < 8192; ++i)
-        IGNORE_RET(sys_close(i));
-    } else {
-      google_breakpad::DirectoryReader reader(fd);
-      const char* name;
-      while (reader.GetNextEntry(&name)) {
-        int i;
-        if (my_strtoui(&i, name) && i > 2 && i != fd)
-          IGNORE_RET(sys_close(i));
-        reader.PopEntry();
-      }
-
-      IGNORE_RET(sys_close(fd));
-    }
+    CloseAllFileDescriptors();
 
     IGNORE_RET(sys_setsid());
 
@@ -1805,15 +1849,20 @@ void HandleCrashDump(const BreakpadInfo& info) {
       const pid_t upload_child = sys_fork();
       if (!upload_child) {
         // Upload process.
-        IGNORE_RET(sys_close(fds[0]));
-        IGNORE_RET(sys_dup2(fds[1], 3));
-        ExecUploadProcessOrTerminate(info, temp_file, mime_boundary, exe_buf,
-                                     &allocator);
+        IGNORE_RET(sys_close(fds[0]));  // Close read end of pipe.
+        const int pipe_write_fd = fds[1];
+#if defined(OS_CHROMEOS)
+        UploadWithCrashReporterOrExit(info, temp_file, exe_buf, pipe_write_fd,
+                                      &allocator);
+#else
+        UploadWithWgetOrExit(info, temp_file, mime_boundary, pipe_write_fd,
+                             &allocator);
+#endif
       }
 
       // Helper process.
       if (upload_child > 0) {
-        IGNORE_RET(sys_close(fds[1]));
+        IGNORE_RET(sys_close(fds[1]));  // Close write end of pipe.
 
         const size_t kCrashIdLength = 16;
         char id_buf[kCrashIdLength + 1];