crash-reporter: Add diagnostics to help diagnose failures in the wild
We add logging messages that are generated during invocation of core2md, but we also enable sending arbitrary system info based on the configuration file. In this case we add the list of running processes, meminfo, and dmesg.
Change-Id: Ifdf29b89dd60d56349fa39095d2aa07f6b5e2de2
BUG=chromium-os:6299,chromium-os:9345
TEST=UserCrash, unit tests
Review URL: http://codereview.chromium.org/6297004
diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc
index 89fe41d..afd72d1 100644
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@@ -110,7 +110,8 @@
}
if (pid == 0) {
- int output_handle = HANDLE_EINTR(creat(output_file, 0600));
+ int output_handle = HANDLE_EINTR(
+ open(output_file, O_CREAT | O_WRONLY | O_TRUNC | O_EXCL, 0666));
if (output_handle < 0) {
logger_->LogError("Could not create %s: %d", output_file, errno);
// Avoid exit() to avoid atexit handlers from parent.
diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h
index 772de24..f3fcbe5 100644
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@@ -48,6 +48,15 @@
FRIEND_TEST(CrashCollectorTest, ReadKeyValueFile);
FRIEND_TEST(CrashCollectorTest, Sanitize);
FRIEND_TEST(CrashCollectorTest, WriteNewFile);
+ FRIEND_TEST(ForkExecAndPipeTest, Basic);
+ FRIEND_TEST(ForkExecAndPipeTest, NonZeroReturnValue);
+ FRIEND_TEST(ForkExecAndPipeTest, BadOutputFile);
+ FRIEND_TEST(ForkExecAndPipeTest, ExistingOutputFile);
+ FRIEND_TEST(ForkExecAndPipeTest, BadExecutable);
+ FRIEND_TEST(ForkExecAndPipeTest, StderrCaptured);
+ FRIEND_TEST(ForkExecAndPipeTest, NULLParam);
+ FRIEND_TEST(ForkExecAndPipeTest, NoParams);
+ FRIEND_TEST(ForkExecAndPipeTest, SegFaultHandling);
// Set maximum enqueued crashes in a crash directory.
static const int kMaxCrashDirectorySize;
diff --git a/crash_reporter/crash_collector_test.cc b/crash_reporter/crash_collector_test.cc
index 7f1d4c2..8100a5f 100644
--- a/crash_reporter/crash_collector_test.cc
+++ b/crash_reporter/crash_collector_test.cc
@@ -67,69 +67,6 @@
strlen(kBuffer)), 0);
}
-TEST_F(CrashCollectorTest, ForkExecAndPipe) {
- std::vector<const char *> args;
- char output_file[] = "test/fork_out";
-
- // Test basic call with stdout.
- args.clear();
- args.push_back(kBinEcho);
- args.push_back("hello world");
- EXPECT_EQ(0, collector_.ForkExecAndPipe(args, output_file));
- ExpectFileEquals("hello world\n", output_file);
- EXPECT_EQ("", logging_.log());
-
- // Test non-zero return value
- logging_.clear();
- args.clear();
- args.push_back(kBinFalse);
- EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
- ExpectFileEquals("", output_file);
- EXPECT_EQ("", logging_.log());
-
- // Test bad output_file.
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, "/bad/path"));
-
- // Test bad executable.
- logging_.clear();
- args.clear();
- args.push_back("false");
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
-
- // Test stderr captured.
- std::string contents;
- logging_.clear();
- args.clear();
- args.push_back(kBinCp);
- EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_TRUE(file_util::ReadFileToString(FilePath(output_file),
- &contents));
- EXPECT_NE(std::string::npos, contents.find("missing file operand"));
- EXPECT_EQ("", logging_.log());
-
- // NULL parameter.
- logging_.clear();
- args.clear();
- args.push_back(NULL);
- EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_NE(std::string::npos,
- logging_.log().find("Bad parameter"));
-
- // No parameters.
- args.clear();
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
-
- // Segmentation faulting process.
- logging_.clear();
- args.clear();
- args.push_back(kBinBash);
- args.push_back("-c");
- args.push_back("kill -SEGV $$");
- EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_NE(std::string::npos,
- logging_.log().find("Process did not exit normally"));
-}
-
TEST_F(CrashCollectorTest, Sanitize) {
EXPECT_EQ("chrome", collector_.Sanitize("chrome"));
EXPECT_EQ("CHROME", collector_.Sanitize("CHROME"));
@@ -389,10 +326,12 @@
ASSERT_TRUE(
file_util::WriteFile(config_file,
kConfigContents, strlen(kConfigContents)));
+ file_util::Delete(FilePath(output_file), false);
EXPECT_FALSE(collector_.GetLogContents(config_file,
"barfoo",
output_file));
EXPECT_FALSE(file_util::PathExists(output_file));
+ file_util::Delete(FilePath(output_file), false);
EXPECT_TRUE(collector_.GetLogContents(config_file,
"foobar",
output_file));
@@ -402,6 +341,86 @@
EXPECT_EQ("hello world\n", contents);
}
+class ForkExecAndPipeTest : public CrashCollectorTest {
+ public:
+ void SetUp() {
+ CrashCollectorTest::SetUp();
+ output_file_ = "test/fork_out";
+ file_util::Delete(FilePath(output_file_), false);
+ }
+
+ void TearDown() {
+ CrashCollectorTest::TearDown();
+ }
+
+ protected:
+ std::vector<const char *> args_;
+ const char *output_file_;
+};
+
+TEST_F(ForkExecAndPipeTest, Basic) {
+ args_.push_back(kBinEcho);
+ args_.push_back("hello world");
+ EXPECT_EQ(0, collector_.ForkExecAndPipe(args_, output_file_));
+ ExpectFileEquals("hello world\n", output_file_);
+ EXPECT_EQ("", logging_.log());
+}
+
+TEST_F(ForkExecAndPipeTest, NonZeroReturnValue) {
+ args_.push_back(kBinFalse);
+ EXPECT_EQ(1, collector_.ForkExecAndPipe(args_, output_file_));
+ ExpectFileEquals("", output_file_);
+ EXPECT_EQ("", logging_.log());
+}
+
+TEST_F(ForkExecAndPipeTest, BadOutputFile) {
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, "/bad/path"));
+}
+
+TEST_F(ForkExecAndPipeTest, ExistingOutputFile) {
+ args_.push_back(kBinEcho);
+ args_.push_back("hello world");
+ EXPECT_FALSE(file_util::PathExists(FilePath(output_file_)));
+ EXPECT_EQ(0, collector_.ForkExecAndPipe(args_, output_file_));
+ EXPECT_TRUE(file_util::PathExists(FilePath(output_file_)));
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_));
+}
+
+TEST_F(ForkExecAndPipeTest, BadExecutable) {
+ args_.push_back("false");
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_));
+}
+
+TEST_F(ForkExecAndPipeTest, StderrCaptured) {
+ std::string contents;
+ args_.push_back(kBinCp);
+ EXPECT_EQ(1, collector_.ForkExecAndPipe(args_, output_file_));
+ EXPECT_TRUE(file_util::ReadFileToString(FilePath(output_file_),
+ &contents));
+ EXPECT_NE(std::string::npos, contents.find("missing file operand"));
+ EXPECT_EQ("", logging_.log());
+}
+
+TEST_F(ForkExecAndPipeTest, NULLParam) {
+ args_.push_back(NULL);
+ EXPECT_EQ(-1, collector_.ForkExecAndPipe(args_, output_file_));
+ EXPECT_NE(std::string::npos,
+ logging_.log().find("Bad parameter"));
+}
+
+TEST_F(ForkExecAndPipeTest, NoParams) {
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_));
+}
+
+TEST_F(ForkExecAndPipeTest, SegFaultHandling) {
+ args_.push_back(kBinBash);
+ args_.push_back("-c");
+ args_.push_back("kill -SEGV $$");
+ EXPECT_EQ(-1, collector_.ForkExecAndPipe(args_, output_file_));
+ EXPECT_NE(std::string::npos,
+ logging_.log().find("Process did not exit normally"));
+}
+
int main(int argc, char **argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
diff --git a/crash_reporter/crash_reporter.cc b/crash_reporter/crash_reporter.cc
index c583dec..ffe6cdf 100644
--- a/crash_reporter/crash_reporter.cc
+++ b/crash_reporter/crash_reporter.cc
@@ -21,8 +21,7 @@
DEFINE_string(generate_kernel_signature, "",
"Generate signature from given kcrash file");
DEFINE_bool(crash_test, false, "Crash test");
-DEFINE_int32(pid, -1, "Crashing PID");
-DEFINE_int32(signal, -1, "Signal causing crash");
+DEFINE_string(user, "", "User crash info (pid:signal:exec_name)");
DEFINE_bool(unclean_check, true, "Check for unclean shutdown");
#pragma GCC diagnostic error "-Wstrict-aliasing"
@@ -140,8 +139,7 @@
static int HandleUserCrash(UserCollector *user_collector) {
// Handle a specific user space crash.
- CHECK(FLAGS_signal != -1) << "Signal must be set";
- CHECK(FLAGS_pid != -1) << "PID must be set";
+ CHECK(!FLAGS_user.empty()) << "--user= must be set";
// Make it possible to test what happens when we crash while
// handling a crash.
@@ -151,7 +149,7 @@
}
// Handle the crash, get the name of the process from procfs.
- if (!user_collector->HandleCrash(FLAGS_signal, FLAGS_pid, NULL)) {
+ if (!user_collector->HandleCrash(FLAGS_user, NULL)) {
return 1;
}
return 0;
diff --git a/crash_reporter/crash_reporter_logs.conf b/crash_reporter/crash_reporter_logs.conf
index 172b9c3..1921983 100644
--- a/crash_reporter/crash_reporter_logs.conf
+++ b/crash_reporter/crash_reporter_logs.conf
@@ -17,6 +17,11 @@
#
update_engine:cat $(ls -1tr /var/log/update_engine | tail -5 | sed s.^./var/log/update_engine/.) | tail -c 50000
+# The following rule is used for generating additional diagnostics when
+# collection of user crashes fails. This output should not be too large
+# as it is stored in memory.
+crash_reporter-user-collection:echo "===ps output==="; ps auxw | tail -c 25000; echo "===dmesg output==="; dmesg | tail -c 25000; echo "===meminfo==="; cat /proc/meminfo
+
# The following rules are only for testing purposes.
crash_log_test:echo hello world
crash_log_recursion_test:sleep 1 && /usr/local/autotest/tests/crash_log_recursion_test
diff --git a/crash_reporter/system_logging.cc b/crash_reporter/system_logging.cc
index 5789514..04bb356 100644
--- a/crash_reporter/system_logging.cc
+++ b/crash_reporter/system_logging.cc
@@ -2,14 +2,15 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <stdarg.h>
+#include "crash-reporter/system_logging.h"
+
#include <syslog.h>
-#include "crash-reporter/system_logging.h"
+#include "base/stringprintf.h"
std::string SystemLoggingImpl::identity_;
-SystemLoggingImpl::SystemLoggingImpl() {
+SystemLoggingImpl::SystemLoggingImpl() : is_accumulating_(false) {
}
SystemLoggingImpl::~SystemLoggingImpl() {
@@ -22,23 +23,33 @@
openlog(identity_.c_str(), LOG_PID, LOG_USER);
}
+void SystemLoggingImpl::LogWithLevel(int level, const char *format,
+ va_list arg_list) {
+ std::string message = StringPrintV(format, arg_list);
+ syslog(level, "%s", message.c_str());
+ if (is_accumulating_) {
+ accumulator_.append(message);
+ accumulator_.push_back('\n');
+ }
+}
+
void SystemLoggingImpl::LogInfo(const char *format, ...) {
va_list vl;
va_start(vl, format);
- vsyslog(LOG_INFO, format, vl);
+ LogWithLevel(LOG_INFO, format, vl);
va_end(vl);
}
void SystemLoggingImpl::LogWarning(const char *format, ...) {
va_list vl;
va_start(vl, format);
- vsyslog(LOG_WARNING, format, vl);
+ LogWithLevel(LOG_WARNING, format, vl);
va_end(vl);
}
void SystemLoggingImpl::LogError(const char *format, ...) {
va_list vl;
va_start(vl, format);
- vsyslog(LOG_ERR, format, vl);
+ LogWithLevel(LOG_ERR, format, vl);
va_end(vl);
}
diff --git a/crash_reporter/system_logging.h b/crash_reporter/system_logging.h
index 7ebfc5e..d1f72a5 100644
--- a/crash_reporter/system_logging.h
+++ b/crash_reporter/system_logging.h
@@ -5,6 +5,7 @@
#ifndef CRASH_REPORTER_SYSTEM_LOGGING_H_
#define CRASH_REPORTER_SYSTEM_LOGGING_H_
+#include <stdarg.h>
#include <string>
class SystemLogging {
@@ -13,8 +14,12 @@
virtual void LogInfo(const char *format, ...) = 0;
virtual void LogWarning(const char *format, ...) = 0;
virtual void LogError(const char *format, ...) = 0;
+ virtual void set_accumulating(bool value) = 0;
+ virtual std::string get_accumulator() = 0;
};
+// SystemLoggingImpl implements SystemLogging but adds the
+// capability of accumulating the log to a STL string.
class SystemLoggingImpl : public SystemLogging {
public:
SystemLoggingImpl();
@@ -23,8 +28,18 @@
virtual void LogInfo(const char *format, ...);
virtual void LogWarning(const char *format, ...);
virtual void LogError(const char *format, ...);
+ virtual void set_accumulating(bool value) {
+ is_accumulating_ = value;
+ }
+ virtual std::string get_accumulator() {
+ return accumulator_;
+ }
private:
static std::string identity_;
+ std::string accumulator_;
+ bool is_accumulating_;
+ void LogWithLevel(int level, const char *format,
+ va_list arg_list);
};
#endif // CRASH_REPORTER_SYSTEM_LOGGING_H_
diff --git a/crash_reporter/system_logging_mock.cc b/crash_reporter/system_logging_mock.cc
index a2760e9..04ce853 100644
--- a/crash_reporter/system_logging_mock.cc
+++ b/crash_reporter/system_logging_mock.cc
@@ -33,3 +33,10 @@
log_ += "\n";
va_end(vl);
}
+
+void SystemLoggingMock::set_accumulating(bool value) {
+}
+
+std::string SystemLoggingMock::get_accumulator() {
+ return "";
+}
diff --git a/crash_reporter/system_logging_mock.h b/crash_reporter/system_logging_mock.h
index f4fb133..56c6e58 100644
--- a/crash_reporter/system_logging_mock.h
+++ b/crash_reporter/system_logging_mock.h
@@ -15,6 +15,8 @@
virtual void LogInfo(const char *format, ...);
virtual void LogWarning(const char *format, ...);
virtual void LogError(const char *format, ...);
+ virtual void set_accumulating(bool value);
+ virtual std::string get_accumulator();
const std::string &log() { return log_; }
diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc
index 59b460b..a6dd091 100644
--- a/crash_reporter/user_collector.cc
+++ b/crash_reporter/user_collector.cc
@@ -5,6 +5,8 @@
#include "crash-reporter/user_collector.h"
#include <grp.h> // For struct group.
+#include <pcrecpp.h>
+#include <pcrecpp.h>
#include <pwd.h> // For struct passwd.
#include <sys/types.h> // For getpwuid_r, getgrnam_r, WEXITSTATUS.
@@ -18,8 +20,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 +70,11 @@
std::string UserCollector::GetPattern(bool enabled) const {
if (enabled) {
- return StringPrintf("|%s --signal=%%s --pid=%%p", our_path_.c_str());
+ // Combine the three crash attributes into one parameter to try to reduce
+ // the size of the invocation line for crash_reporter since the kernel
+ // has a fixed-sized (128B) buffer that it will truncate into. Note that
+ // the kernel does not support quoted arguments in core_pattern.
+ return StringPrintf("|%s --user=%%p:%%s:%%e", our_path_.c_str());
} else {
return "core";
}
@@ -169,12 +175,6 @@
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 +184,24 @@
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 +209,13 @@
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 +228,7 @@
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 +240,27 @@
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 +273,7 @@
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 +290,7 @@
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 +302,16 @@
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 +347,7 @@
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;
}
@@ -342,6 +355,10 @@
// procfs entries and other temporary files used during conversion.
FilePath container_dir = FilePath("/tmp").Append(
StringPrintf("crash_reporter.%d", 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.
+ file_util::Delete(container_dir, true);
std::string dump_basename = FormatDumpBasename(exec, time(NULL), pid);
FilePath core_path = GetCrashPath(crash_path, dump_basename, "core");
FilePath meta_path = GetCrashPath(crash_path, dump_basename, "meta");
@@ -376,15 +393,34 @@
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 +430,7 @@
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 +457,11 @@
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;
diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h
index 90bc578..851d587 100644
--- a/crash_reporter/user_collector.h
+++ b/crash_reporter/user_collector.h
@@ -40,7 +40,8 @@
bool Disable() { return SetUpInternal(false); }
// Handle a specific user crash. Returns true on success.
- bool HandleCrash(int signal, int pid, const char *force_exec);
+ 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) {
@@ -61,6 +62,7 @@
FRIEND_TEST(UserCollectorTest, GetProcessPath);
FRIEND_TEST(UserCollectorTest, GetSymlinkTarget);
FRIEND_TEST(UserCollectorTest, GetUserInfoFromName);
+ FRIEND_TEST(UserCollectorTest, ParseCrashAttributes);
// Enumeration to pass to GetIdFromStatus. Must match the order
// that the kernel lists IDs in the status file.
@@ -109,6 +111,9 @@
const FilePath &minidump_path);
bool ConvertAndEnqueueCrash(int pid, const std::string &exec_name,
bool *out_of_capacity);
+ bool ParseCrashAttributes(const std::string &crash_attributes,
+ pid_t *pid, int *signal,
+ std::string *kernel_supplied_name);
bool generate_diagnostics_;
std::string core_pattern_file_;
@@ -116,9 +121,6 @@
std::string our_path_;
bool initialized_;
- // String containing the current log of crash handling errors.
- std::string error_log_;
-
static const char *kUserId;
static const char *kGroupId;
};
diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc
index 942123f..53e227c 100644
--- a/crash_reporter/user_collector_test.cc
+++ b/crash_reporter/user_collector_test.cc
@@ -54,7 +54,7 @@
TEST_F(UserCollectorTest, EnableOK) {
ASSERT_TRUE(collector_.Enable());
- ExpectFileEquals("|/my/path --signal=%s --pid=%p", "test/core_pattern");
+ ExpectFileEquals("|/my/path --user=%p:%s:%e", "test/core_pattern");
ExpectFileEquals("4", "test/core_pipe_limit");
ASSERT_EQ(s_crashes, 0);
ASSERT_NE(logging_.log().find("Enabling user crash handling"),
@@ -102,9 +102,36 @@
std::string::npos);
}
+TEST_F(UserCollectorTest, ParseCrashAttributes) {
+ pid_t pid;
+ int signal;
+ std::string exec_name;
+ EXPECT_TRUE(collector_.ParseCrashAttributes("123456:11:foobar",
+ &pid, &signal, &exec_name));
+ EXPECT_EQ(123456, pid);
+ EXPECT_EQ(11, signal);
+ EXPECT_EQ("foobar", exec_name);
+
+ EXPECT_FALSE(collector_.ParseCrashAttributes("123456:11",
+ &pid, &signal, &exec_name));
+
+ EXPECT_TRUE(collector_.ParseCrashAttributes("123456:11:exec:extra",
+ &pid, &signal, &exec_name));
+ EXPECT_EQ("exec:extra", exec_name);
+
+ EXPECT_FALSE(collector_.ParseCrashAttributes("12345p:11:foobar",
+ &pid, &signal, &exec_name));
+
+ EXPECT_FALSE(collector_.ParseCrashAttributes("123456:1 :foobar",
+ &pid, &signal, &exec_name));
+
+ EXPECT_FALSE(collector_.ParseCrashAttributes("123456::foobar",
+ &pid, &signal, &exec_name));
+}
+
TEST_F(UserCollectorTest, HandleCrashWithoutMetrics) {
s_metrics = false;
- collector_.HandleCrash(10, 20, "foobar");
+ collector_.HandleCrash("20:10:ignored", "foobar");
ASSERT_NE(std::string::npos,
logging_.log().find(
"Received crash notification for foobar[20] sig 10"));
@@ -113,7 +140,7 @@
TEST_F(UserCollectorTest, HandleNonChromeCrashWithMetrics) {
s_metrics = true;
- collector_.HandleCrash(2, 5, "chromeos-wm");
+ collector_.HandleCrash("5:2:ignored", "chromeos-wm");
ASSERT_NE(std::string::npos,
logging_.log().find(
"Received crash notification for chromeos-wm[5] sig 2"));
@@ -122,7 +149,7 @@
TEST_F(UserCollectorTest, HandleChromeCrashWithMetrics) {
s_metrics = true;
- collector_.HandleCrash(2, 5, "chrome");
+ collector_.HandleCrash("5:2:ignored", "chrome");
ASSERT_NE(std::string::npos,
logging_.log().find(
"Received crash notification for chrome[5] sig 2"));
