crash-reporter: Add diagnostics to help diagnose failures in the wild

user_collector.cc

Index: user_collector.cc
diff --git a/user_collector.cc b/user_collector.cc
index 59b460bdca6ae261cfa8bfe2890170a5fc91c2f5..816f2fad467e95b7ddf53b7aeddddc9ebfc981ca 100644
--- a/user_collector.cc
+++ b/user_collector.cc
@@ -6,6 +6,7 @@
 
 #include <grp.h>  // For struct group.
 #include <pwd.h>  // For struct passwd.
+#include <pcrecpp.h>

[petkov, 2011/01/24 18:36:04]

sort

[kmixter1, 2011/01/25 21:28:07]

Done.

 #include <sys/types.h>  // For getpwuid_r, getgrnam_r, WEXITSTATUS.
 
 #include <string>
@@ -18,8 +19,8 @@
 #include "gflags/gflags.h"
 
 #pragma GCC diagnostic ignored "-Wstrict-aliasing"
-DEFINE_bool(core2md_failure_test, false, "Core2md failure test");
-DEFINE_bool(directory_failure_test, false, "Spool directory failure test");
+DEFINE_bool(core2md_failure, false, "Core2md failure test");
+DEFINE_bool(directory_failure, false, "Spool directory failure test");
 DEFINE_string(filter_in, "",
               "Ignore all crashes but this for testing");
 #pragma GCC diagnostic error "-Wstrict-aliasing"
@@ -68,7 +69,7 @@ UserCollector::~UserCollector() {
 
 std::string UserCollector::GetPattern(bool enabled) const {
   if (enabled) {
-    return StringPrintf("|%s --signal=%%s --pid=%%p", our_path_.c_str());
+    return StringPrintf("|%s --user=%%p:%%s:%%e", our_path_.c_str());

[petkov, 2011/01/24 18:36:04]

I guess no concern about %%e containing spaces?

[petkov, 2011/01/24 18:36:04]

add a comment why you're combining the values into a single option?

[kmixter1, 2011/01/25 21:28:07]

It's annoying but the kernel doesn't support quoting parameters in core_pattern,
and %%e can contain spaces.  If this occurs, only the part before the space will
be used - and it's only used if the normal way of getting the command line fails
(via proc).  So I'm not too concerned.  Added a comment.

[kmixter1, 2011/01/25 21:28:07]

Done.

   } else {
     return "core";
   }
@@ -169,12 +170,6 @@ bool UserCollector::GetIdFromStatus(const char *prefix,
   return true;
 }
 
-void UserCollector::LogCollectionError(const std::string &error_message) {
-  error_log_.append(error_message.c_str());
-  error_log_.append("\n");
-  logger_->LogError(error_message.c_str());
-}
-
 void UserCollector::EnqueueCollectionErrorLog(pid_t pid,
                                               const std::string &exec) {
   FilePath crash_path;
@@ -184,12 +179,24 @@ void UserCollector::EnqueueCollectionErrorLog(pid_t pid,
     return;
   }
   std::string dump_basename = FormatDumpBasename(exec, time(NULL), pid);
+  std::string error_log = logger_->get_accumulator();
+  FilePath diag_log_path = GetCrashPath(crash_path, dump_basename, "diaglog");
+  if (GetLogContents(FilePath(kDefaultLogConfig), kCollectionErrorSignature,
+                     diag_log_path)) {
+    // We load the contents of diag_log into memory and append it to
+    // the error log.  We cannot just append to files because we need
+    // to always create new files to prevent attack.
+    std::string diag_log_contents;
+    file_util::ReadFileToString(diag_log_path, &diag_log_contents);
+    error_log.append(diag_log_contents);
+    file_util::Delete(diag_log_path, false);
+  }
   FilePath log_path = GetCrashPath(crash_path, dump_basename, "log");
   FilePath meta_path = GetCrashPath(crash_path, dump_basename, "meta");
   // We must use WriteNewFile instead of file_util::WriteFile as we do
   // not want to write with root access to a symlink that an attacker
   // might have created.
-  WriteNewFile(log_path, error_log_.data(), error_log_.length());
+  WriteNewFile(log_path, error_log.data(), error_log.length());
   AddCrashMetaData("sig", kCollectionErrorSignature);
   WriteCrashMetaData(meta_path, exec, log_path.value());
 }
@@ -197,14 +204,13 @@ void UserCollector::EnqueueCollectionErrorLog(pid_t pid,
 bool UserCollector::CopyOffProcFiles(pid_t pid,
                                      const FilePath &container_dir) {
   if (!file_util::CreateDirectory(container_dir)) {
-    LogCollectionError(StringPrintf("Could not create %s",
-                                    container_dir.value().c_str()));
+    logger_->LogError("Could not create %s",
+                      container_dir.value().c_str());
     return false;
   }
   FilePath process_path = GetProcessPath(pid);
   if (!file_util::PathExists(process_path)) {
-    LogCollectionError(StringPrintf("Path %s does not exist",
-                                    process_path.value().c_str()));
+    logger_->LogError("Path %s does not exist", process_path.value().c_str());
     return false;
   }
   static const char *proc_files[] = {
@@ -217,8 +223,7 @@ bool UserCollector::CopyOffProcFiles(pid_t pid,
   for (unsigned i = 0; i < arraysize(proc_files); ++i) {
     if (!file_util::CopyFile(process_path.Append(proc_files[i]),
                              container_dir.Append(proc_files[i]))) {
-      LogCollectionError(StringPrintf("Could not copy %s file",
-                                      proc_files[i]));
+      logger_->LogError("Could not copy %s file", proc_files[i]);
       return false;
     }
   }
@@ -230,24 +235,27 @@ bool UserCollector::GetCreatedCrashDirectory(pid_t pid,
                                              bool *out_of_capacity) {
   FilePath process_path = GetProcessPath(pid);
   std::string status;
-  if (FLAGS_directory_failure_test) {
-    LogCollectionError("Purposefully failing to create spool directory");
+  if (FLAGS_directory_failure) {
+    logger_->LogError("Purposefully failing to create spool directory");
     return false;
   }
   if (!file_util::ReadFileToString(process_path.Append("status"),
                                    &status)) {
-    LogCollectionError("Could not read status file");
+    logger_->LogError("Could not read status file");
+    logger_->LogInfo("Path %s FileExists: %d",
+                     process_path.value().c_str(),
+                     file_util::DirectoryExists(process_path));
     return false;
   }
   int process_euid;
   if (!GetIdFromStatus(kUserId, kIdEffective, status, &process_euid)) {
-    LogCollectionError("Could not find euid in status file");
+    logger_->LogError("Could not find euid in status file");
     return false;
   }
   if (!GetCreatedCrashDirectoryByEuid(process_euid,
                                       crash_file_path,
                                       out_of_capacity)) {
-    LogCollectionError("Could not create crash directory");
+    logger_->LogError("Could not create crash directory");
     return false;
   }
   return true;
@@ -260,7 +268,7 @@ bool UserCollector::CopyStdinToCoreFile(const FilePath &core_path) {
     return true;
   }
 
-  LogCollectionError("Could not write core file");
+  logger_->LogError("Could not write core file");
   // If the file system was full, make sure we remove any remnants.
   file_util::Delete(core_path, false);
   return false;
@@ -277,7 +285,7 @@ bool UserCollector::RunCoreToMinidump(const FilePath &core_path,
   core2md_arguments.push_back(procfs_directory.value().c_str());
   core2md_arguments.push_back(minidump_path.value().c_str());
 
-  if (FLAGS_core2md_failure_test) {
+  if (FLAGS_core2md_failure) {
     // To test how core2md errors are propagaged, cause an error
     // by forgetting a required argument.
     core2md_arguments.pop_back();
@@ -289,16 +297,16 @@ bool UserCollector::RunCoreToMinidump(const FilePath &core_path,
   std::string output;
   file_util::ReadFileToString(output_path, &output);
   if (errorlevel != 0) {
-    LogCollectionError(StringPrintf("Problem during %s [result=%d]: %s",
-                                    kCoreToMinidumpConverterPath,
-                                    errorlevel,
-                                    output.c_str()));
+    logger_->LogError("Problem during %s [result=%d]: %s",
+                      kCoreToMinidumpConverterPath,
+                      errorlevel,
+                      output.c_str());
     return false;
   }
 
   if (!file_util::PathExists(minidump_path)) {
-    LogCollectionError(StringPrintf("Minidump file %s was not created",
-                                    minidump_path.value().c_str()));
+    logger_->LogError("Minidump file %s was not created",
+                      minidump_path.value().c_str());
     return false;
   }
   return true;
@@ -334,7 +342,7 @@ bool UserCollector::ConvertAndEnqueueCrash(int pid,
                                            bool *out_of_capacity) {
   FilePath crash_path;
   if (!GetCreatedCrashDirectory(pid, &crash_path, out_of_capacity)) {
-    LogCollectionError("Unable to find/create process-specific crash path");
+    logger_->LogError("Unable to find/create process-specific crash path");
     return false;
   }
 
@@ -376,15 +384,34 @@ bool UserCollector::ConvertAndEnqueueCrash(int pid,
   return true;
 }
 
-bool UserCollector::HandleCrash(int signal, int pid, const char *force_exec) {
+bool UserCollector::ParseCrashAttributes(const std::string &crash_attributes,
+                                         pid_t *pid, int *signal,
+                                         std::string *kernel_supplied_name) {
+  pcrecpp::RE re("(\\d+):(\\d+):(.*)");
+  return re.FullMatch(crash_attributes, pid, signal, kernel_supplied_name);
+}
+
+bool UserCollector::HandleCrash(const std::string &crash_attributes,
+                                const char *force_exec) {
   CHECK(initialized_);
+  int pid = 0;
+  int signal = 0;
+  std::string kernel_supplied_name;
+
+  if (!ParseCrashAttributes(crash_attributes, &pid, &signal,
+                            &kernel_supplied_name)) {
+    logger_->LogError("Invalid parameter: --user=%s", crash_attributes.c_str());
+    return false;
+  }
+
   std::string exec;
   if (force_exec) {
     exec.assign(force_exec);
   } else if (!GetExecutableBaseNameFromPid(pid, &exec)) {
-    // If for some reason we don't have the base name, avoid completely
-    // failing by indicating an unknown name.
-    exec = "unknown";
+    // If we cannot find the exec name, use the kernel supplied name.
+    // We don't always use the kernel's since it truncates the name to
+    // 16 characters.
+    exec = StringPrintf("supplied_%s", kernel_supplied_name.c_str());
   }
 
   // Allow us to test the crash reporting mechanism successfully even if
@@ -394,7 +421,7 @@ bool UserCollector::HandleCrash(int signal, int pid, const char *force_exec) {
        FLAGS_filter_in != exec)) {
     // We use a different format message to make it more obvious in tests
     // which crashes are test generated and which are real.
-    logger_->LogWarning("Ignoring crash from %s[%d] while filter_in=%s",
+    logger_->LogWarning("Ignoring crash from %s[%d] while filter_in=%s.",
                         exec.c_str(), pid, FLAGS_filter_in.c_str());
     return true;
   }
@@ -421,7 +448,11 @@ bool UserCollector::HandleCrash(int signal, int pid, const char *force_exec) {
 
     if (generate_diagnostics_) {
       bool out_of_capacity = false;
-      if (!ConvertAndEnqueueCrash(pid, exec, &out_of_capacity)) {
+      logger_->set_accumulating(true);
+      bool convert_and_enqueue_result =
+          ConvertAndEnqueueCrash(pid, exec, &out_of_capacity);
+      logger_->set_accumulating(false);
+      if (!convert_and_enqueue_result) {
         if (!out_of_capacity)
           EnqueueCollectionErrorLog(pid, exec);
         return false;