diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc index 69fe9392f..540616043 100644 --- a/crash_reporter/crash_collector.cc +++ b/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 = kUserCrashPathMode; - *directory_owner = default_user_id; - *directory_group = default_user_group; - return FilePath(kFallbackUserCrashPath); - } else { - *mode = kSystemCrashPathMode; - *directory_owner = kRootOwner; - *directory_group = kRootGroup; - return FilePath(kSystemCrashPath); - } + *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, - default_user_id, - default_user_group, - &directory_mode, + GetCrashDirectoryInfo(&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; diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h index 0d335cdda..0c28048fd 100644 --- a/crash_reporter/crash_collector.h +++ b/crash_reporter/crash_collector.h @@ -72,12 +72,9 @@ class CrashCollector { forced_crash_directory_ = forced_directory; } - base::FilePath 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); + base::FilePath GetCrashDirectoryInfo(mode_t *mode, + uid_t *directory_owner, + gid_t *directory_group); bool GetUserInfoFromName(const std::string &name, uid_t *uid, gid_t *gid); diff --git a/crash_reporter/crash_collector_test.cc b/crash_reporter/crash_collector_test.cc index 13fb76a6d..28c4462f2 100644 --- a/crash_reporter/crash_collector_test.cc +++ b/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; diff --git a/crash_reporter/init.crash_reporter.rc b/crash_reporter/init.crash_reporter.rc new file mode 100644 index 000000000..f65371a2f --- /dev/null +++ b/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 diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc index 069b581b7..6bf912099 100644 --- a/crash_reporter/user_collector.cc +++ b/crash_reporter/user_collector.cc @@ -24,21 +24,17 @@ #include #include #include +#include static const char kCollectionErrorSignature[] = "crash_reporter-user-collection"; -// This procfs file is used to cause kernel core file writing to -// instead pipe the core file into a user space process. See -// 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 kCorePatternProperty[] = "crash_reporter.coredump.enabled"; +static const char kCoreToMinidumpConverterPath[] = "/system/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, - strlen(kCorePipeLimit)) != - static_cast(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(pattern.length())) { - PLOG(ERROR) << "Unable to write " << core_pattern_file_; - return false; - } + property_set(kCorePatternProperty, enabled ? "1" : "0"); + 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. diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h index 7b356eed7..dd4245713 100644 --- a/crash_reporter/user_collector.h +++ b/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_; diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc index ee3ca128b..3c595212d 100644 --- a/crash_reporter/user_collector_test.cc +++ b/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;