Changing minidump process generation to be in-process on Android.

chrome/app/breakpad_linux.cc

Index: chrome/app/breakpad_linux.cc
diff --git a/chrome/app/breakpad_linux.cc b/chrome/app/breakpad_linux.cc
index afa87a0c3286526a255087707efae3e155755641..34401f9c697d5fc4df5a7e6dafb0b59a22bf1472 100644
--- a/chrome/app/breakpad_linux.cc
+++ b/chrome/app/breakpad_linux.cc
@@ -93,6 +93,9 @@ ExceptionHandler* g_breakpad = NULL;
 #if defined(ADDRESS_SANITIZER)
 const char* g_asan_report_str = NULL;
 #endif
+#if defined(OS_ANDROID)
+char* g_process_type = NULL;
+#endif
 
 // Writes the value |v| as 16 hex characters to the memory pointed at by
 // |output|.
@@ -126,6 +129,15 @@ uint64_t kernel_timeval_to_ms(struct kernel_timeval *tv) {
 // String buffer size to use to convert a uint64_t to string.
 size_t kUint64StringSize = 21;
 
+static void SetProcessStartTime() {
+  // Set the base process start time value.
+  struct timeval tv;
+  if (!gettimeofday(&tv, NULL))
+    g_process_start_time = timeval_to_ms(&tv);
+  else
+    g_process_start_time = 0;
+}
+
 // uint64_t version of my_int_len() from
 // breakpad/src/common/linux/linux_libc_support.h. Return the length of the
 // given, non-negative integer when expressed in base 10.
@@ -179,6 +191,30 @@ char* my_strncat(char *dest, const char* src, size_t len) {
 }
 #endif
 
+// Populates the passed in allocated strings and their sizes with the GUID,
+// crash url and distro of the crashing process.
+// The passed strings are expected to be at least kGuidSize, kMaxActiveURLSize
+// and kDistroSize bytes long respectively.
+void PopulateGUIDAndURLAndDistro(char* guid, size_t* guid_len_param,
+                                 char* crash_url, size_t* crash_url_len_param,
+                                 char* distro, size_t* distro_len_param) {
+  size_t guid_len = std::min(my_strlen(child_process_logging::g_client_id),
+                             kGuidSize);
+  size_t crash_url_len =
+      std::min(my_strlen(child_process_logging::g_active_url),
+               kMaxActiveURLSize);
+  size_t distro_len = std::min(my_strlen(base::g_linux_distro), kDistroSize);
+  memcpy(guid, child_process_logging::g_client_id, guid_len);
+  memcpy(crash_url, child_process_logging::g_active_url, crash_url_len);
+  memcpy(distro, base::g_linux_distro, distro_len);
+  if (guid_len_param)
+    *guid_len_param = guid_len;
+  if (crash_url_len_param)
+    *crash_url_len_param = crash_url_len;
+  if (distro_len_param)
+    *distro_len_param = distro_len;
+}
+
 // MIME substrings.
 const char g_rn[] = "\r\n";
 const char g_form_data_msg[] = "Content-Disposition: form-data; name=\"";
@@ -414,13 +450,17 @@ bool CrashDone(const MinidumpDescriptor& minidump,
                const bool succeeded) {
   // WARNING: this code runs in a compromised context. It may not call into
   // libc nor allocate memory normally.
-  if (!succeeded)
+  if (!succeeded) {
+    const char msg[] = "Failed to generate minidump.";
+    WriteLog(msg, sizeof(msg) - 1);
     return false;
+  }
 
   DCHECK(!minidump.IsFD());
 
-  BreakpadInfo info;
+  BreakpadInfo info = {0};
   info.filename = minidump.path();
+  info.fd = minidump.fd();
 #if defined(ADDRESS_SANITIZER)
   google_breakpad::PageAllocator allocator;
   const size_t log_path_len = my_strlen(minidump.path());
@@ -466,6 +506,46 @@ bool CrashDoneUpload(const MinidumpDescriptor& minidump,
 }
 #endif
 
+#if defined(OS_ANDROID)

[Lei Zhang, 2012/10/23 21:34:58]

can we put this closer to NonBrowserCrashHandler() below?

[Jay Civelli, 2012/10/24 00:12:53]

Done.

+bool CrashDoneInProcessNoUpload(
+    const google_breakpad::MinidumpDescriptor& descriptor,
+    void* context,
+    const bool succeeded) {
+  // WARNING: this code runs in a compromised context. It may not call into
+  // libc nor allocate memory normally.
+  if (!succeeded) {
+    static const char msg[] = "Crash dump generation failed.\n";
+    WriteLog(msg, sizeof(msg) - 1);
+    return false;
+  }
+
+  // Start constructing the message to send to the browser.
+  char guid[kGuidSize + 1] = {0};
+  char crash_url[kMaxActiveURLSize + 1] = {0};
+  char distro[kDistroSize + 1] = {0};
+  size_t guid_length = 0;
+  size_t crash_url_length = 0;
+  size_t distro_length = 0;
+  PopulateGUIDAndURLAndDistro(guid, &guid_length, crash_url, &crash_url_length,
+                              distro, &distro_length);
+  BreakpadInfo info = {0};
+  info.filename = NULL;
+  info.fd = descriptor.fd();
+  info.process_type = g_process_type;
+  info.process_type_length = my_strlen(g_process_type);
+  info.crash_url = crash_url;
+  info.crash_url_length = crash_url_length;
+  info.guid = guid;
+  info.guid_length = guid_length;
+  info.distro = distro;
+  info.distro_length = distro_length;
+  info.upload = false;
+  info.process_start_time = g_process_start_time;
+  HandleCrashDump(info);
+  return true;
+}
+#endif
+
 #if defined(ADDRESS_SANITIZER)
 extern "C"
 void __asan_set_error_report_callback(void (*cb)(const char*));
@@ -520,6 +600,7 @@ void EnableCrashDumping(bool unattended) {
 #endif
 }
 
+#if !defined(OS_ANDROID)
 // Non-Browser = Extension, Gpu, Plugins, Ppapi and Renderer
 bool NonBrowserCrashHandler(const void* crash_context,
                             size_t crash_context_size,
@@ -528,7 +609,7 @@ bool NonBrowserCrashHandler(const void* crash_context,
   int fds[2] = { -1, -1 };
   if (sys_socketpair(AF_UNIX, SOCK_STREAM, 0, fds) < 0) {
     static const char msg[] = "Failed to create socket for crash dumping.\n";
-    WriteLog(msg, sizeof(msg)-1);
+    WriteLog(msg, sizeof(msg) - 1);
     return false;
   }
 
@@ -536,16 +617,7 @@ bool NonBrowserCrashHandler(const void* crash_context,
   char guid[kGuidSize + 1] = {0};
   char crash_url[kMaxActiveURLSize + 1] = {0};
   char distro[kDistroSize + 1] = {0};
-  const size_t guid_len =
-      std::min(my_strlen(child_process_logging::g_client_id), kGuidSize);
-  const size_t crash_url_len =
-      std::min(my_strlen(child_process_logging::g_active_url),
-               kMaxActiveURLSize);
-  const size_t distro_len =
-      std::min(my_strlen(base::g_linux_distro), kDistroSize);
-  memcpy(guid, child_process_logging::g_client_id, guid_len);
-  memcpy(crash_url, child_process_logging::g_active_url, crash_url_len);
-  memcpy(distro, base::g_linux_distro, distro_len);
+  PopulateGUIDAndURLAndDistro(guid, NULL, crash_url, NULL, distro, NULL);
 
   char b;  // Dummy variable for sys_read below.
   const char* b_addr = &b;  // Get the address of |b| so we can create the
@@ -603,7 +675,7 @@ bool NonBrowserCrashHandler(const void* crash_context,
 
   if (HANDLE_EINTR(sys_sendmsg(fd, &msg, 0)) < 0) {
     static const char errmsg[] = "Failed to tell parent about crash.\n";
-    WriteLog(errmsg, sizeof(errmsg)-1);
+    WriteLog(errmsg, sizeof(errmsg) - 1);
     IGNORE_RET(sys_close(fds[1]));
     return false;
   }
@@ -611,7 +683,7 @@ bool NonBrowserCrashHandler(const void* crash_context,
 
   if (HANDLE_EINTR(sys_read(fds[0], &b, 1)) != 1) {
     static const char errmsg[] = "Parent failed to complete crash dump.\n";
-    WriteLog(errmsg, sizeof(errmsg)-1);
+    WriteLog(errmsg, sizeof(errmsg) - 1);
   }
 
 #if defined(OS_ANDROID)

[Lei Zhang, 2012/10/23 21:34:58]

This is now a defined(OS_ANDROID) inside a !defined(OS_ANDROID) block.

[Jay Civelli, 2012/10/24 00:12:53]

Removed. Also moved that function next to EnableNonBrowserCrashDumping where it
is used.

@@ -623,7 +695,42 @@ bool NonBrowserCrashHandler(const void* crash_context,
   return true;
 #endif
 }
+#endif
 
+#if defined(OS_ANDROID)
+void EnableNonBrowserCrashDumping(int minidump_fd) {
+  // This will guarantee that the BuildInfo has been initialized and subsequent
+  // calls will not require memory allocation.
+  base::android::BuildInfo::GetInstance();
+  child_process_logging::SetClientId("Android");
+
+  // On Android, the current sandboxing uses process isolation, in which the
+  // child process runs with a different UID. That breaks the normal crash
+  // reporting where the browser process generates the minidump by inspecting
+  // the child process. This is because the browser process now does not have
+  // the permission to access the states of the child process (as it has a
+  // different UID).
+  // TODO(jcivelli): http://b/issue?id=6776356 we should use a watchdog
+  // process forked from the renderer process that generates the minidump.
+  if (minidump_fd == -1) {
+    LOG(ERROR) << "Minidump file descriptor not found, crash reporting will "
+        " not work.";
+    return;
+  }
+  SetProcessStartTime();
+
+  g_is_crash_reporter_enabled = true;
+  // Save the process type (it is leaked).
+  const CommandLine& parsed_command_line = *CommandLine::ForCurrentProcess();
+  const std::string process_type =
+      parsed_command_line.GetSwitchValueASCII(switches::kProcessType);
+  const size_t process_type_len = process_type.size() + 1;
+  g_process_type = new char[process_type_len];
+  strncpy(g_process_type, process_type.c_str(), process_type_len);
+  new google_breakpad::ExceptionHandler(MinidumpDescriptor(minidump_fd),
+      NULL, CrashDoneInProcessNoUpload, NULL, true, -1);
+}
+#else
 void EnableNonBrowserCrashDumping() {
   const int fd = base::GlobalDescriptors::GetInstance()->Get(kCrashDumpSignal);
   g_is_crash_reporter_enabled = true;
@@ -644,51 +751,91 @@ void EnableNonBrowserCrashDumping() {
       -1);
   g_breakpad->set_crash_handler(NonBrowserCrashHandler);
 }
+#endif
 
 }  // namespace
 
-void LoadDataFromFile(google_breakpad::PageAllocator& allocator,
-                      const BreakpadInfo& info, const char* filename,
-                      int* fd, uint8_t** file_data, size_t* size) {
-  // WARNING: this code runs in a compromised context. It may not call into
-  // libc nor allocate memory normally.
-  *fd = sys_open(filename, O_RDONLY, 0);
-  *size = 0;
-
-  if (*fd < 0) {
-    static const char msg[] = "Cannot upload crash dump: failed to open\n";
-    WriteLog(msg, sizeof(msg));
-    return;
-  }
+void LoadDataFromFD(google_breakpad::PageAllocator& allocator,
+                    int fd, bool close_fd, uint8_t** file_data, size_t* size) {
   STAT_STRUCT st;
-  if (FSTAT_FUNC(*fd, &st) != 0) {
+  if (FSTAT_FUNC(fd, &st) != 0) {
     static const char msg[] = "Cannot upload crash dump: stat failed\n";
-    WriteLog(msg, sizeof(msg));
-    IGNORE_RET(sys_close(*fd));
+    WriteLog(msg, sizeof(msg) - 1);
+    if (close_fd)
+      IGNORE_RET(sys_close(fd));
     return;
   }
 
   *file_data = reinterpret_cast<uint8_t*>(allocator.Alloc(st.st_size));
   if (!(*file_data)) {
     static const char msg[] = "Cannot upload crash dump: cannot alloc\n";
-    WriteLog(msg, sizeof(msg));
-    IGNORE_RET(sys_close(*fd));
+    WriteLog(msg, sizeof(msg) - 1);
+    if (close_fd)
+      IGNORE_RET(sys_close(fd));
     return;
   }
   my_memset(*file_data, 0xf, st.st_size);
 
   *size = st.st_size;
-  sys_read(*fd, *file_data, *size);
-  IGNORE_RET(sys_close(*fd));
+  int byte_read = sys_read(fd, *file_data, *size);
+  if (byte_read == -1) {
+    static const char msg[] = "Cannot upload crash dump: read failed\n";
+    WriteLog(msg, sizeof(msg) - 1);
+    if (close_fd)
+      IGNORE_RET(sys_close(fd));
+    return;
+  }
+
+  if (close_fd)
+    IGNORE_RET(sys_close(fd));
+}
+
+void LoadDataFromFile(google_breakpad::PageAllocator& allocator,
+                      const char* filename,
+                      int* fd, uint8_t** file_data, size_t* size) {
+  // WARNING: this code runs in a compromised context. It may not call into
+  // libc nor allocate memory normally.
+  *fd = sys_open(filename, O_RDONLY, 0);
+  *size = 0;
+
+  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);
 }
 
 void HandleCrashDump(const BreakpadInfo& info) {
   int dumpfd;
+  bool keep_fd = false;
   size_t dump_size;
   uint8_t* dump_data;
   google_breakpad::PageAllocator allocator;
-  LoadDataFromFile(allocator, info, info.filename,
-                   &dumpfd, &dump_data, &dump_size);
+
+  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) {
+      static const char msg[] = "Cannot upload crash dump: failed to "
+          "reposition minidump FD\n";
+      WriteLog(msg, sizeof(msg) - 1);
+      IGNORE_RET(sys_close(dumpfd));
+      return;
+    }
+    LoadDataFromFD(allocator, info.fd, false, &dump_data, &dump_size);
+  } else {
+    // Dump is provided with a path.
+    keep_fd = false;
+    LoadDataFromFile(allocator, info.filename, &dumpfd, &dump_data, &dump_size);
+  }
+
+  // TODO(jcivelli): make log work when using FDs.
 #if defined(ADDRESS_SANITIZER)
   int logfd;
   size_t log_size;
@@ -712,33 +859,45 @@ void HandleCrashDump(const BreakpadInfo& info) {
       "/tmp/chromium-upload-XXXXXXXXXXXXXXXX";
   char temp_file[sizeof(temp_file_template)];
   int temp_file_fd = -1;
-  if (info.upload) {
-    memcpy(temp_file, temp_file_template, sizeof(temp_file_template));
-
-    for (unsigned i = 0; i < 10; ++i) {
-      uint64_t t;
-      sys_read(ufd, &t, sizeof(t));
-      write_uint64_hex(temp_file + sizeof(temp_file) - (16 + 1), t);
-
-      temp_file_fd = sys_open(temp_file, O_WRONLY | O_CREAT | O_EXCL, 0600);
-      if (temp_file_fd >= 0)
-        break;
-    }
-
-    if (temp_file_fd < 0) {
-      static const char msg[] = "Failed to create temporary file in /tmp: "
-          "cannot upload crash dump\n";
+  if (keep_fd) {
+    temp_file_fd = dumpfd;
+    // Rewind the destination, we are going to overwrite it.
+    if (lseek(dumpfd, 0, SEEK_SET) == -1) {
+      static const char msg[] = "Cannot upload crash dump: failed to "
+          "reposition minidump FD (2)\n";
       WriteLog(msg, sizeof(msg) - 1);
-      IGNORE_RET(sys_close(ufd));
+      IGNORE_RET(sys_close(dumpfd));
       return;
     }
   } else {
-    temp_file_fd = sys_open(info.filename, O_WRONLY, 0600);
-    if (temp_file_fd < 0) {
-      static const char msg[] = "Failed to save crash dump: failed to open\n";
-      WriteLog(msg, sizeof(msg) - 1);
-      IGNORE_RET(sys_close(ufd));
-      return;
+    if (info.upload) {
+      memcpy(temp_file, temp_file_template, sizeof(temp_file_template));
+
+      for (unsigned i = 0; i < 10; ++i) {
+        uint64_t t;
+        sys_read(ufd, &t, sizeof(t));
+        write_uint64_hex(temp_file + sizeof(temp_file) - (16 + 1), t);
+
+        temp_file_fd = sys_open(temp_file, O_WRONLY | O_CREAT | O_EXCL, 0600);
+        if (temp_file_fd >= 0)
+          break;
+      }
+
+      if (temp_file_fd < 0) {
+        static const char msg[] = "Failed to create temporary file in /tmp: "
+            "cannot upload crash dump\n";
+        WriteLog(msg, sizeof(msg) - 1);
+        IGNORE_RET(sys_close(ufd));
+        return;
+      }
+    } else {
+      temp_file_fd = sys_open(info.filename, O_WRONLY, 0600);
+      if (temp_file_fd < 0) {
+        static const char msg[] = "Failed to save crash dump: failed to open\n";
+        WriteLog(msg, sizeof(msg) - 1);
+        IGNORE_RET(sys_close(ufd));
+        return;
+      }
     }
   }
 
@@ -1068,34 +1227,41 @@ void HandleCrashDump(const BreakpadInfo& info) {
   IGNORE_RET(sys_close(temp_file_fd));
 
 #if defined(OS_ANDROID)
-  __android_log_write(ANDROID_LOG_WARN,
-                      kGoogleBreakpad,
-                      "Output crash dump file:");
-  __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, info.filename);
-
-  char pid_buf[kUint64StringSize];
-  uint64_t pid_str_len = my_uint64_len(info.pid);
-  my_uint64tos(pid_buf, info.pid, pid_str_len);
-
-  // -1 because we won't need the null terminator on the original filename.
-  size_t done_filename_len = my_strlen(info.filename) + pid_str_len - 1;
-  char* done_filename = reinterpret_cast<char*>(
-      allocator.Alloc(done_filename_len));
-  // Rename the file such that the pid is the suffix in order to signal other
-  // processes that the minidump is complete. The advantage of using the pid as
-  // the suffix is that it is trivial to associate the minidump with the
-  // crashed process.
-  // Finally, note strncpy prevents null terminators from
-  // being copied. Pad the rest with 0's.
-  my_strncpy(done_filename, info.filename, done_filename_len);
-  // Append the suffix a null terminator should be added.
-  my_strncat(done_filename, pid_buf, pid_str_len);
-  // Rename the minidump file to signal that it is complete.
-  if (rename(info.filename, done_filename)) {
-    __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, "Failed to rename:");
-    __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, info.filename);
-    __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, "to");
-    __android_log_write(ANDROID_LOG_WARN, kGoogleBreakpad, done_filename);
+  if (info.filename) {
+    int filename_length = my_strlen(info.filename);
+
+    // If this was a file, we need to copy it to the right place and use the
+    // right file name so it gets uploaded by the browser.
+    const char msg[] = "Output crash dump file:";
+    WriteLog(msg, sizeof(msg) - 1);
+    WriteLog(info.filename, filename_length - 1);
+
+    char pid_buf[kUint64StringSize];
+    uint64_t pid_str_length = my_uint64_len(info.pid);
+    my_uint64tos(pid_buf, info.pid, pid_str_length);
+
+    // -1 because we won't need the null terminator on the original filename.
+    unsigned done_filename_len = filename_length -1 + pid_str_length;
+    char* done_filename = reinterpret_cast<char*>(
+        allocator.Alloc(done_filename_len));
+    // Rename the file such that the pid is the suffix in order signal to other
+    // processes that the minidump is complete. The advantage of using the pid
+    // as the suffix is that it is trivial to associate the minidump with the
+    // crashed process.
+    // Finally, note strncpy prevents null terminators from
+    // being copied. Pad the rest with 0's.
+    my_strncpy(done_filename, info.filename, done_filename_len);
+    // Append the suffix a null terminator should be added.
+    my_strncat(done_filename, pid_buf, pid_str_length);
+    // Rename the minidump file to signal that it is complete.
+    if (rename(info.filename, done_filename)) {
+      const char failed_msg[] = "Failed to rename:";
+      WriteLog(failed_msg, sizeof(failed_msg) - 1);
+      WriteLog(info.filename, filename_length - 1);
+      const char to_msg[] = "to";
+      WriteLog(to_msg, sizeof(to_msg) - 1);
+      WriteLog(done_filename, done_filename_len - 1);
+    }
   }
 #endif
 
@@ -1276,8 +1442,10 @@ void InitCrashReporter() {
              process_type == switches::kZygoteProcess ||
              process_type == switches::kGpuProcess) {
 #if defined(OS_ANDROID)
-    child_process_logging::SetClientId("Android");
-#endif
+    NOTREACHED() << "Breakpad initialized with InitCrashReporter() instead of "
+      "InitNonBrowserCrashReporter in non browser process.";

[Lei Zhang, 2012/10/23 21:34:58]

slightly better to say "... in " << process_type << " process.";

[Jay Civelli, 2012/10/24 00:12:53]

Done.

+    return;
+#else
     // We might be chrooted in a zygote or renderer process so we cannot call
     // GetCollectStatsConsent because that needs access the the user's home
     // dir. Instead, we set a command line flag for these processes.
@@ -1296,14 +1464,11 @@ void InitCrashReporter() {
       child_process_logging::SetClientId(switch_value);
     }
     EnableNonBrowserCrashDumping();
+    LOG(INFO) << "Non Browser crash dumping enabled for: " << process_type;

[Lei Zhang, 2012/10/23 21:34:58]

VLOG() ?

[Jay Civelli, 2012/10/24 00:12:53]

Done.

+#endif  // #if defined(OS_ANDROID)
   }
 
-  // Set the base process start time value.
-  struct timeval tv;
-  if (!gettimeofday(&tv, NULL))
-    g_process_start_time = timeval_to_ms(&tv);
-  else
-    g_process_start_time = 0;
+  SetProcessStartTime();
 
   logging::SetDumpWithoutCrashingFunction(&DumpProcess);
 #if defined(ADDRESS_SANITIZER)
@@ -1312,6 +1477,14 @@ void InitCrashReporter() {
 #endif
 }
 
+#if defined(OS_ANDROID)
+void InitNonBrowserCrashReporterForAndroid(int minidump_fd) {
+  const CommandLine* command_line = CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kEnableCrashReporter))
+    EnableNonBrowserCrashDumping(minidump_fd);
+}
+#endif  // OS_ANDROID
+
 bool IsCrashReporterEnabled() {
   return g_is_crash_reporter_enabled;
 }