crash-reporter: Avoid using system to invoke core2md
Change-Id: I1fc0b4dd6e4e84cb999ba61fedd43fc74a8fa1ba
BUG=6941
TEST=bvts
Review URL: http://codereview.chromium.org/3755011
diff --git a/crash_reporter/system_logging_mock.h b/crash_reporter/system_logging_mock.h
index 639fe42..f4fb133 100644
--- a/crash_reporter/system_logging_mock.h
+++ b/crash_reporter/system_logging_mock.h
@@ -18,6 +18,8 @@
const std::string &log() { return log_; }
+ void clear() { log_.clear(); }
+
private:
static std::string identity_;
std::string log_;
diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc
index 4047eb4..bc414ab 100644
--- a/crash_reporter/user_collector.cc
+++ b/crash_reporter/user_collector.cc
@@ -4,12 +4,17 @@
#include "crash-reporter/user_collector.h"
+#include <fcntl.h> // For creat.
#include <grp.h> // For struct group.
#include <pwd.h> // For struct passwd.
-#include <sys/types.h> // For getpwuid_r and getgrnam_r.
+#include <sys/types.h> // For getpwuid_r, getgrnam_r, WEXITSTATUS.
+#include <sys/wait.h> // For waitpid.
+#include <unistd.h> // For execv and fork.
#include <string>
+#include <vector>
+#include "base/eintr_wrapper.h"
#include "base/file_util.h"
#include "base/logging.h"
#include "base/string_util.h"
@@ -204,27 +209,81 @@
return false;
}
+int UserCollector::ForkExecAndPipe(std::vector<const char *> &arguments,
+ const char *output_file) {
+ // Copy off a writeable version of arguments.
+ scoped_array<char*> argv(new char *[arguments.size() + 1]);
+ int total_args_size = 0;
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ if (arguments[i] == NULL) {
+ logger_->LogError("Bad parameter");
+ return -1;
+ }
+ total_args_size += strlen(arguments[i]) + 1;
+ }
+ scoped_array<char> buffer(new char[total_args_size]);
+ char *buffer_pointer = &buffer[0];
+
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ argv[i] = buffer_pointer;
+ strcpy(buffer_pointer, arguments[i]);
+ buffer_pointer += strlen(arguments[i]);
+ *buffer_pointer = '\0';
+ ++buffer_pointer;
+ }
+ argv[arguments.size()] = NULL;
+
+ int pid = fork();
+ if (pid < 0) {
+ logger_->LogError("Fork failed: %d", errno);
+ return -1;
+ }
+
+ if (pid == 0) {
+ int output_handle = creat(output_file, 0700);
+ if (output_handle < 0) {
+ logger_->LogError("Could not create %s: %d", output_file, errno);
+ // Avoid exit() to avoid atexit handlers from parent.
+ _exit(127);
+ }
+ dup2(output_handle, 1);
+ dup2(output_handle, 2);
+ execv(argv[0], &argv[0]);
+ logger_->LogError("Exec failed: %d", errno);
+ _exit(127);
+ }
+
+ int status = 0;
+ if (HANDLE_EINTR(waitpid(pid, &status, 0)) < 0) {
+ logger_->LogError("Problem waiting for pid: %d", errno);
+ return -1;
+ }
+ if (!WIFEXITED(status)) {
+ logger_->LogError("Process did not exit normally: %x", status);
+ return -1;
+ }
+ return WEXITSTATUS(status);
+}
+
bool UserCollector::ConvertCoreToMinidump(const FilePath &core_path,
const FilePath &procfs_directory,
const FilePath &minidump_path,
const FilePath &temp_directory) {
- // TODO(kmixter): Rewrite to use process_util once it's included in
- // libchrome.
FilePath output_path = temp_directory.Append("output");
- std::string core2md_command =
- StringPrintf("\"%s\" \"%s\" \"%s\" \"%s\" > \"%s\" 2>&1",
- kCoreToMinidumpConverterPath,
- core_path.value().c_str(),
- procfs_directory.value().c_str(),
- minidump_path.value().c_str(),
- output_path.value().c_str());
- int errorlevel = system(core2md_command.c_str());
+ std::vector<const char *> core2md_arguments;
+ core2md_arguments.push_back(kCoreToMinidumpConverterPath);
+ core2md_arguments.push_back(core_path.value().c_str());
+ core2md_arguments.push_back(procfs_directory.value().c_str());
+ core2md_arguments.push_back(minidump_path.value().c_str());
+
+ int errorlevel = ForkExecAndPipe(core2md_arguments,
+ output_path.value().c_str());
std::string output;
file_util::ReadFileToString(output_path, &output);
if (errorlevel != 0) {
logger_->LogInfo("Problem during %s [result=%d]: %s",
- core2md_command.c_str(),
+ kCoreToMinidumpConverterPath,
errorlevel,
output.c_str());
return false;
diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h
index 484445f..7d6c711 100644
--- a/crash_reporter/user_collector.h
+++ b/crash_reporter/user_collector.h
@@ -6,6 +6,7 @@
#define _CRASH_REPORTER_USER_COLLECTOR_H_
#include <string>
+#include <vector>
#include "crash-reporter/crash_collector.h"
#include "gtest/gtest_prod.h" // for FRIEND_TEST
@@ -51,6 +52,7 @@
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesBadPath);
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesBadPid);
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesOK);
+ FRIEND_TEST(UserCollectorTest, ForkExecAndPipe);
FRIEND_TEST(UserCollectorTest, GetIdFromStatus);
FRIEND_TEST(UserCollectorTest, GetProcessPath);
FRIEND_TEST(UserCollectorTest, GetSymlinkTarget);
@@ -66,6 +68,8 @@
kIdMax
};
+ static const int kForkProblem = 255;
+
std::string GetPattern(bool enabled) const;
bool SetUpInternal(bool enabled);
@@ -86,6 +90,8 @@
bool GetCreatedCrashDirectory(pid_t pid,
FilePath *crash_file_path);
bool CopyStdinToCoreFile(const FilePath &core_path);
+ int ForkExecAndPipe(std::vector<const char *> &arguments,
+ const char *output_file);
bool ConvertCoreToMinidump(const FilePath &core_path,
const FilePath &procfs_directory,
const FilePath &minidump_path,
diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc
index 3390caa..9dc0d60 100644
--- a/crash_reporter/user_collector_test.cc
+++ b/crash_reporter/user_collector_test.cc
@@ -15,6 +15,12 @@
static const char kFilePath[] = "/my/path";
+// This test assumes the following standard binaries are installed.
+static const char kBinBash[] = "/bin/bash";
+static const char kBinCp[] = "/bin/cp";
+static const char kBinEcho[] = "/bin/echo";
+static const char kBinFalse[] = "/bin/false";
+
void CountCrash() {
++s_crashes;
}
@@ -38,17 +44,22 @@
protected:
void TestEnableOK(bool generate_diagnostics);
+ void ExpectFileEquals(const char *golden,
+ const char *file_path) {
+ std::string contents;
+ EXPECT_TRUE(file_util::ReadFileToString(FilePath(file_path),
+ &contents));
+ EXPECT_EQ(golden, contents);
+ }
+
SystemLoggingMock logging_;
UserCollector collector_;
pid_t pid_;
};
TEST_F(UserCollectorTest, EnableOK) {
- std::string contents;
ASSERT_TRUE(collector_.Enable());
- ASSERT_TRUE(file_util::ReadFileToString(FilePath("test/core_pattern"),
- &contents));
- ASSERT_EQ("|/my/path --signal=%s --pid=%p", contents);
+ ExpectFileEquals("|/my/path --signal=%s --pid=%p", "test/core_pattern");
ASSERT_EQ(s_crashes, 0);
ASSERT_NE(logging_.log().find("Enabling user crash handling"),
std::string::npos);
@@ -65,11 +76,8 @@
}
TEST_F(UserCollectorTest, DisableOK) {
- std::string contents;
ASSERT_TRUE(collector_.Disable());
- ASSERT_TRUE(file_util::ReadFileToString(FilePath("test/core_pattern"),
- &contents));
- ASSERT_EQ("core", contents);
+ ExpectFileEquals("core", "test/core_pattern");
ASSERT_EQ(s_crashes, 0);
ASSERT_NE(logging_.log().find("Disabling user crash handling"),
std::string::npos);
@@ -85,6 +93,69 @@
std::string::npos);
}
+TEST_F(UserCollectorTest, 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(UserCollectorTest, HandleCrashWithoutMetrics) {
s_metrics = false;
collector_.HandleCrash(10, 20, "foobar");