breakpad: Fix crash_reporter upload success handling on Chrome OS

components/crash/content/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/crash/content/app/breakpad_linux.h"
 
 #include <fcntl.h>
@@ skipping 82 matching lines @@
 
 bool g_is_crash_reporter_enabled = false;
 uint64_t g_process_start_time = 0;
 pid_t g_pid = 0;
 char* g_crash_log_path = nullptr;
 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;
 char* g_process_type = nullptr;
 ExceptionHandler* g_microdump = nullptr;
 int g_signal_code_pipe_fd = -1;
 
 class MicrodumpInfo {
  public:
   MicrodumpInfo()
@@ skipping 1037 matching lines @@
 
   if (*fd < 0) {
     static const char msg[] = "Cannot upload crash dump: failed to open\n";
     WriteLog(msg, sizeof(msg) - 1);
     return;
   }
 
   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);
   pid_buf[pid_str_length] = '\0';
 
   char uid_buf[kUint64StringSize];
   uid_t uid = geteuid();
   uint64_t uid_str_length = my_uint64_len(uid);
   my_uint64tos(uid_buf, uid, uid_str_length);
   uid_buf[uid_str_length] = '\0';
@@ skipping 29 matching lines @@
   const char* args[] = {
     kCrashReporterBinary,
     chrome_flag,
     pid_flag,
     uid_flag,
     exe_flag,
     nullptr,
   };
   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) {
     static const char msg[] = "sys_fork() for gzip process failed.\n";
     WriteLog(msg, sizeof(msg) - 1);
     sys__exit(1);
   }
   if (!gzip_child) {
     // gzip process.
     const char* args[] = {
@@ skipping 59 matching lines @@
   static const char kWgetBinary[] = "/usr/bin/wget";
   const char* args[] = {
     kWgetBinary,
     header_content_encoding,
     header_content_type,
     post_file,
     kUploadURL,
     "--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|.
 // |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.
@@ skipping 18 matching lines @@
     // |ret| == 0 -> timed out, continue waiting.
     // or |bytes_read| < |bytes_to_read| still, keep reading.
   }
   buf[bytes_to_read] = 0;  // Always NUL terminate the buffer.
   return bytes_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) {
     if (!my_isxdigit(buf[i]))
       return false;
   }
   return true;
 #endif
 }
 
 // |buf| should be |expected_len| + 1 characters in size and nullptr 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[] =
-        "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
     WriteLog(msg, sizeof(msg) - 1);
     WriteNewline();
 
     static const char id_msg[] = "Report Id: ";
     WriteLog(id_msg, sizeof(id_msg) - 1);
     WriteLog(buf, bytes_read);
     WriteNewline();
@@ skipping 54 matching lines @@
       if (name)
         return name + 1;
       return link;
     }
   }
   // Either way too long, or a read error.
   return "chrome-crash-unknown-process";
 }
 #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;
   }
 
 #if defined(OS_CHROMEOS)
   // 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) {
     // Dump is provided with an open FD.
     keep_fd = true;
     dumpfd = info.fd;
 
     // The FD is pointing to the end of the file.
     // Rewind, we'll read the data next.
     if (lseek(dumpfd, 0, SEEK_SET) == -1) {
@@ skipping 295 matching lines @@
     //
     // This code is called both when a browser is crashing (in which case,
     // nothing really matters any more) and when a renderer/plugin crashes, in
     // which case we need to continue.
     //
     // Since we are a multithreaded app, if we were just to fork(), we might
     // grab file descriptors which have just been created in another thread and
     // 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());
 
     // Leave one end of a pipe in the upload process and watch for it getting
     // closed by the upload process exiting.
     int fds[2];
     if (sys_pipe(fds) >= 0) {
       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];
         size_t bytes_read =
             WaitForCrashReportUploadProcess(fds[0], kCrashIdLength, id_buf);
         HandleCrashReportId(id_buf, bytes_read, kCrashIdLength);
 
         if (sys_waitpid(upload_child, nullptr, WNOHANG) == 0) {
           // Upload process is still around, kill it.
           sys_kill(upload_child, SIGKILL);
@@ skipping 147 matching lines @@
 void SuppressDumpGeneration() {
   g_dumps_suppressed = G_DUMPS_SUPPRESSED_MAGIC;
 }
 #endif  // OS_ANDROID
 
 bool IsCrashReporterEnabled() {
   return g_is_crash_reporter_enabled;
 }
 
 }  // namespace breakpad