am cbc6b915: am d4555689: Merge "crash_reporter: Enable core dumps"

* commit 'cbc6b915c90b31c2c1cc9a56d23280ac51f6d329':
  crash_reporter: Enable core dumps

crash_reporter/crash_collector.cc

@@ -36,7 +36,7 @@ const char kDefaultUserName[] = "chronos";
 const char kLeaveCoreFile[] = "/root/.leave_core";
 const char kLsbRelease[] = "/etc/lsb-release";
 const char kShellPath[] = "/bin/sh";
-const char kSystemCrashPath[] = "/var/spool/crash";
+const char kSystemCrashPath[] = "/data/misc/crash_reporter/crash";
 const char kUploadVarPrefix[] = "upload_var_";
 const char kUploadFilePrefix[] = "upload_file_";
 
@@ -148,23 +148,13 @@ FilePath CrashCollector::GetCrashPath(const FilePath &crash_directory,
 }
 
 FilePath CrashCollector::GetCrashDirectoryInfo(
-    uid_t process_euid,
-    uid_t default_user_id,
-    gid_t default_user_group,
     mode_t *mode,
     uid_t *directory_owner,
     gid_t *directory_group) {
-  if (process_euid == default_user_id) {
+  *mode = kSystemCrashPathMode;
-    *mode = kUserCrashPathMode;
+  *directory_owner = kRootOwner;
-    *directory_owner = default_user_id;
+  *directory_group = kRootGroup;
-    *directory_group = default_user_group;
+  return FilePath(kSystemCrashPath);
-    return FilePath(kFallbackUserCrashPath);
-  } else {
-    *mode = kSystemCrashPathMode;
-    *directory_owner = kRootOwner;
-    *directory_group = kRootGroup;
-    return FilePath(kSystemCrashPath);
-  }
 }
 
 bool CrashCollector::GetUserInfoFromName(const std::string &name,
@@ -188,9 +178,6 @@ bool CrashCollector::GetUserInfoFromName(const std::string &name,
 bool CrashCollector::GetCreatedCrashDirectoryByEuid(uid_t euid,
                                                     FilePath *crash_directory,
                                                     bool *out_of_capacity) {
-  uid_t default_user_id;
-  gid_t default_user_group;
-
   if (out_of_capacity) *out_of_capacity = false;
 
   // For testing.
@@ -199,20 +186,11 @@ bool CrashCollector::GetCreatedCrashDirectoryByEuid(uid_t euid,
     return true;
   }
 
-  if (!GetUserInfoFromName(kDefaultUserName,
-                           &default_user_id,
-                           &default_user_group)) {
-    LOG(ERROR) << "Could not find default user info";
-    return false;
-  }
   mode_t directory_mode;
   uid_t directory_owner;
   gid_t directory_group;
   *crash_directory =
-      GetCrashDirectoryInfo(euid,
+      GetCrashDirectoryInfo(&directory_mode,
-                            default_user_id,
-                            default_user_group,
-                            &directory_mode,
                             &directory_owner,
                             &directory_group);
 
@@ -238,6 +216,8 @@ bool CrashCollector::GetCreatedCrashDirectoryByEuid(uid_t euid,
 
   if (!CheckHasCapacity(*crash_directory)) {
     if (out_of_capacity) *out_of_capacity = true;
+    LOG(ERROR) << "Directory " << crash_directory->value()
+               << " is out of capacity.";
     return false;
   }
 
@@ -309,6 +289,8 @@ bool CrashCollector::GetExecutableBaseNameFromPid(pid_t pid,
 bool CrashCollector::CheckHasCapacity(const FilePath &crash_directory) {
   DIR* dir = opendir(crash_directory.value().c_str());
   if (!dir) {
+    LOG(WARNING) << "Unable to open crash directory "
+                 << crash_directory.value();
     return false;
   }
   struct dirent ent_buf;

crash_reporter/crash_collector.h

@@ -72,12 +72,9 @@ class CrashCollector {
     forced_crash_directory_ = forced_directory;
   }
 
-  base::FilePath GetCrashDirectoryInfo(uid_t process_euid,
+  base::FilePath GetCrashDirectoryInfo(mode_t *mode,
-                                 uid_t default_user_id,
+                                       uid_t *directory_owner,
-                                 gid_t default_user_group,
+                                       gid_t *directory_group);
-                                 mode_t *mode,
-                                 uid_t *directory_owner,
-                                 gid_t *directory_group);
   bool GetUserInfoFromName(const std::string &name,
                            uid_t *uid,
                            gid_t *gid);

crash_reporter/crash_collector_test.cc

@@ -83,54 +83,6 @@ TEST_F(CrashCollectorTest, Sanitize) {
   EXPECT_EQ("_", collector_.Sanitize(" "));
 }
 
-TEST_F(CrashCollectorTest, GetCrashDirectoryInfo) {
-  FilePath path;
-  const int kRootUid = 0;
-  const int kRootGid = 0;
-  const int kNtpUid = 5;
-  const int kChronosUid = 1000;
-  const int kChronosGid = 1001;
-  const mode_t kExpectedSystemMode = 01755;
-  const mode_t kExpectedUserMode = 0755;
-
-  mode_t directory_mode;
-  uid_t directory_owner;
-  gid_t directory_group;
-
-  path = collector_.GetCrashDirectoryInfo(kRootUid,
-                                          kChronosUid,
-                                          kChronosGid,
-                                          &directory_mode,
-                                          &directory_owner,
-                                          &directory_group);
-  EXPECT_EQ("/var/spool/crash", path.value());
-  EXPECT_EQ(kExpectedSystemMode, directory_mode);
-  EXPECT_EQ(kRootUid, directory_owner);
-  EXPECT_EQ(kRootGid, directory_group);
-
-  path = collector_.GetCrashDirectoryInfo(kNtpUid,
-                                          kChronosUid,
-                                          kChronosGid,
-                                          &directory_mode,
-                                          &directory_owner,
-                                          &directory_group);
-  EXPECT_EQ("/var/spool/crash", path.value());
-  EXPECT_EQ(kExpectedSystemMode, directory_mode);
-  EXPECT_EQ(kRootUid, directory_owner);
-  EXPECT_EQ(kRootGid, directory_group);
-
-  path = collector_.GetCrashDirectoryInfo(kChronosUid,
-                                          kChronosUid,
-                                          kChronosGid,
-                                          &directory_mode,
-                                          &directory_owner,
-                                          &directory_group);
-  EXPECT_EQ("/var/spool/crash", path.value());
-  EXPECT_EQ(kExpectedUserMode, directory_mode);
-  EXPECT_EQ(kChronosUid, directory_owner);
-  EXPECT_EQ(kChronosGid, directory_group);
-}
-
 TEST_F(CrashCollectorTest, FormatDumpBasename) {
   struct tm tm = {0};
   tm.tm_sec = 15;

crash_reporter/init.crash_reporter.rc

@@ -0,0 +1,19 @@
+on property:crash_reporter.coredump.enabled=1
+    write /proc/sys/kernel/core_pattern \
+          "|/system/bin/crash_reporter --user=%P:%s:%u:%e"
+
+on property:crash_reporter.coredump.enabled=0
+    write /proc/sys/kernel/core_pattern "core"
+
+on boot
+    # Allow catching multiple unrelated concurrent crashes, but use a finite
+    # number to prevent infinitely recursing on crash handling.
+    write /proc/sys/kernel/core_pipe_limit 4
+
+    # Create crash directories.
+    mkdir /data/misc/crash_reporter 0700 root root
+    mkdir /data/local/tmp/crash_reporter 0700 root root
+
+service crash_reporter /system/bin/crash_reporter --init
+    class late_start
+    oneshot

crash_reporter/user_collector.cc

@@ -24,21 +24,17 @@
 #include <base/strings/stringprintf.h>
 #include <chromeos/process.h>
 #include <chromeos/syslog_logging.h>
+#include <cutils/properties.h>
 
 static const char kCollectionErrorSignature[] =
     "crash_reporter-user-collection";
-// This procfs file is used to cause kernel core file writing to
+static const char kCorePatternProperty[] = "crash_reporter.coredump.enabled";
-// instead pipe the core file into a user space process.  See
+static const char kCoreToMinidumpConverterPath[] = "/system/bin/core2md";
-// core(5) man page.
-static const char kCorePatternFile[] = "/proc/sys/kernel/core_pattern";
-static const char kCorePipeLimitFile[] = "/proc/sys/kernel/core_pipe_limit";
-// Set core_pipe_limit to 4 so that we can catch a few unrelated concurrent
-// crashes, but finite to avoid infinitely recursing on crash handling.
-static const char kCorePipeLimit[] = "4";
-static const char kCoreToMinidumpConverterPath[] = "/usr/bin/core2md";
 
 static const char kStatePrefix[] = "State:\t";
 
+static const char kCoreTempFolder[] = "/data/local/tmp/crash_reporter";
+
 // Define an otherwise invalid value that represents an unknown UID.
 static const uid_t kUnknownUid = -1;
 
@@ -50,8 +46,6 @@ using base::StringPrintf;
 
 UserCollector::UserCollector()
     : generate_diagnostics_(false),
-      core_pattern_file_(kCorePatternFile),
-      core_pipe_limit_file_(kCorePipeLimitFile),
       initialized_(false) {
 }
 
@@ -115,18 +109,8 @@ bool UserCollector::SetUpInternal(bool enabled) {
   CHECK(initialized_);
   LOG(INFO) << (enabled ? "Enabling" : "Disabling") << " user crash handling";
 
-  if (base::WriteFile(FilePath(core_pipe_limit_file_), kCorePipeLimit,
+  property_set(kCorePatternProperty, enabled ? "1" : "0");
-                      strlen(kCorePipeLimit)) !=
+
-      static_cast<int>(strlen(kCorePipeLimit))) {
-    PLOG(ERROR) << "Unable to write " << core_pipe_limit_file_;
-    return false;
-  }
-  std::string pattern = GetPattern(enabled);
-  if (base::WriteFile(FilePath(core_pattern_file_), pattern.c_str(),
-                      pattern.length()) != static_cast<int>(pattern.length())) {
-    PLOG(ERROR) << "Unable to write " << core_pattern_file_;
-    return false;
-  }
   return true;
 }
 
@@ -342,7 +326,7 @@ bool UserCollector::GetCreatedCrashDirectory(pid_t pid, uid_t supplied_ruid,
 
 bool UserCollector::CopyStdinToCoreFile(const FilePath &core_path) {
   // Copy off all stdin to a core file.
-  FilePath stdin_path("/dev/fd/0");
+  FilePath stdin_path("/proc/self/fd/0");
   if (base::CopyFile(stdin_path, core_path)) {
     return true;
   }
@@ -438,7 +422,7 @@ UserCollector::ErrorType UserCollector::ConvertAndEnqueueCrash(
 
   // Directory like /tmp/crash_reporter/1234 which contains the
   // procfs entries and other temporary files used during conversion.
-  FilePath container_dir(StringPrintf("/tmp/crash_reporter/%d", pid));
+  FilePath container_dir(StringPrintf("%s/%d", kCoreTempFolder, pid));
   // Delete a pre-existing directory from crash reporter that may have
   // been left around for diagnostics from a failed conversion attempt.
   // If we don't, existing files can cause forking to fail.

crash_reporter/user_collector.h

@@ -47,16 +47,6 @@ class UserCollector : public CrashCollector {
   bool HandleCrash(const std::string &crash_attributes,
                    const char *force_exec);
 
-  // Set (override the default) core file pattern.
-  void set_core_pattern_file(const std::string &pattern) {
-    core_pattern_file_ = pattern;
-  }
-
-  // Set (override the default) core pipe limit file.
-  void set_core_pipe_limit_file(const std::string &path) {
-    core_pipe_limit_file_ = path;
-  }
-
  private:
   friend class UserCollectorTest;
   FRIEND_TEST(UserCollectorTest, CopyOffProcFilesBadPath);
@@ -172,8 +162,6 @@ class UserCollector : public CrashCollector {
                   std::string *reason);
 
   bool generate_diagnostics_;
-  std::string core_pattern_file_;
-  std::string core_pipe_limit_file_;
   std::string our_path_;
   bool initialized_;
 

crash_reporter/user_collector_test.cc

@@ -60,8 +60,6 @@ class UserCollectorTest : public ::testing::Test {
                           "");
     base::DeleteFile(FilePath("test"), true);
     mkdir("test", 0777);
-    collector_.set_core_pattern_file("test/core_pattern");
-    collector_.set_core_pipe_limit_file("test/core_pipe_limit");
     pid_ = getpid();
     chromeos::ClearLog();
   }
@@ -84,49 +82,6 @@ class UserCollectorTest : public ::testing::Test {
   pid_t pid_;
 };
 
-TEST_F(UserCollectorTest, EnableOK) {
-  ASSERT_TRUE(collector_.Enable());
-  ExpectFileEquals("|/my/path --user=%P:%s:%u:%e",
-                   FilePath("test/core_pattern"));
-  ExpectFileEquals("4", FilePath("test/core_pipe_limit"));
-  ASSERT_EQ(s_crashes, 0);
-  EXPECT_TRUE(FindLog("Enabling user crash handling"));
-}
-
-TEST_F(UserCollectorTest, EnableNoPatternFileAccess) {
-  collector_.set_core_pattern_file("/does_not_exist");
-  ASSERT_FALSE(collector_.Enable());
-  ASSERT_EQ(s_crashes, 0);
-  EXPECT_TRUE(FindLog("Enabling user crash handling"));
-  EXPECT_TRUE(FindLog("Unable to write /does_not_exist"));
-}
-
-TEST_F(UserCollectorTest, EnableNoPipeLimitFileAccess) {
-  collector_.set_core_pipe_limit_file("/does_not_exist");
-  ASSERT_FALSE(collector_.Enable());
-  ASSERT_EQ(s_crashes, 0);
-  // Core pattern should not be written if we cannot access the pipe limit
-  // or otherwise we may set a pattern that results in infinite recursion.
-  ASSERT_FALSE(base::PathExists(FilePath("test/core_pattern")));
-  EXPECT_TRUE(FindLog("Enabling user crash handling"));
-  EXPECT_TRUE(FindLog("Unable to write /does_not_exist"));
-}
-
-TEST_F(UserCollectorTest, DisableOK) {
-  ASSERT_TRUE(collector_.Disable());
-  ExpectFileEquals("core", FilePath("test/core_pattern"));
-  ASSERT_EQ(s_crashes, 0);
-  EXPECT_TRUE(FindLog("Disabling user crash handling"));
-}
-
-TEST_F(UserCollectorTest, DisableNoFileAccess) {
-  collector_.set_core_pattern_file("/does_not_exist");
-  ASSERT_FALSE(collector_.Disable());
-  ASSERT_EQ(s_crashes, 0);
-  EXPECT_TRUE(FindLog("Disabling user crash handling"));
-  EXPECT_TRUE(FindLog("Unable to write /does_not_exist"));
-}
-
 TEST_F(UserCollectorTest, ParseCrashAttributes) {
   pid_t pid;
   int signal;