Add error logging and fix uploading for chrome crash reports
Adds some logging of parse errors in case there's a format mismatch in future.
Fixes detection of the minidump, was checking the wrong variable.
Modifies crash_sender to properly upload a Chrome dump, including all of the
extra options that get included, and identifies it as a Chrome and not ChromeOS
dump so that it shows up in the right lists server-side.
BUG=chromium:216523
TEST=loaded build of chrome with matching changes, verified about:crash causes
a dump to be created and placed in the right spot, and that it uploads
properly by crash_sender
Change-Id: I8a114ad6798f09f33b78df1680153c0412eabf45
Reviewed-on: https://gerrit.chromium.org/gerrit/59572
Reviewed-by: Albert Chaulk <achaulk@chromium.org>
Tested-by: Albert Chaulk <achaulk@chromium.org>
Commit-Queue: Albert Chaulk <achaulk@chromium.org>
diff --git a/crash_reporter/chrome_collector.cc b/crash_reporter/chrome_collector.cc
index d8250f7..2c9ae6e 100644
--- a/crash_reporter/chrome_collector.cc
+++ b/crash_reporter/chrome_collector.cc
@@ -45,6 +45,10 @@
if (!is_feedback_allowed_function_())
return true;
+ if (exe_name.find('/') != std::string::npos) {
+ LOG(ERROR) << "exe_name contains illegal characters: " << exe_name;
+ return false;
+ }
FilePath dir;
uid_t uid = atoi(uid_string.c_str());
@@ -65,7 +69,7 @@
return false;
}
- if (!ParseCrashLog(data, dir, minidump_path)) {
+ if (!ParseCrashLog(data, dir, minidump_path, dump_basename)) {
LOG(ERROR) << "Failed to parse Chrome's crash log";
return false;
}
@@ -81,27 +85,37 @@
bool ChromeCollector::ParseCrashLog(const std::string &data,
const FilePath &dir,
- const FilePath &minidump) {
+ const FilePath &minidump,
+ const std::string &basename) {
size_t at = 0;
while (at < data.size()) {
// Look for a : followed by a decimal number, followed by another :
// followed by N bytes of data.
std::string name, size_string;
- if (!GetDelimitedString(data, ':', at, &name))
+ if (!GetDelimitedString(data, ':', at, &name)) {
+ LOG(ERROR) << "Can't find : after name @ offset " << at;
break;
+ }
at += name.size() + 1; // Skip the name & : delimiter.
- if (!GetDelimitedString(data, ':', at, &size_string))
+ if (!GetDelimitedString(data, ':', at, &size_string)) {
+ LOG(ERROR) << "Can't find : after size @ offset " << at;
break;
+ }
at += size_string.size() + 1; // Skip the size & : delimiter.
size_t size;
- if (!base::StringToSizeT(size_string, &size))
+ if (!base::StringToSizeT(size_string, &size)) {
+ LOG(ERROR) << "String not convertible to integer: " << size_string;
break;
+ }
// Data would run past the end, did we get a truncated file?
- if (at + size > data.size())
+ if (at + size > data.size()) {
+ LOG(ERROR) << "Overrun, expected " << size << " bytes of data, got "
+ << (data.size() - at);
break;
+ }
if (name.find("filename") != std::string::npos) {
// File.
@@ -110,19 +124,19 @@
// Descriptive name will be upload_file_minidump for the dump.
std::string desc, filename;
pcrecpp::RE re("(.*)\" *; *filename=\"(.*)\"");
- if (!re.FullMatch(name.c_str(), &desc, &filename))
+ if (!re.FullMatch(name.c_str(), &desc, &filename)) {
+ LOG(ERROR) << "Filename was not in expected format: " << name;
break;
+ }
- if (filename.compare(kDefaultMinidumpName) == 0) {
+ if (desc.compare(kDefaultMinidumpName) == 0) {
// The minidump.
WriteNewFile(minidump, data.c_str() + at, size);
} else {
// Some other file.
- FilePath path = GetCrashPath(dir, filename, "other");
+ FilePath path = GetCrashPath(dir, basename + "-" + filename, "other");
if (WriteNewFile(path, data.c_str() + at, size) >= 0) {
- std::string value = "@";
- value.append(path.value());
- AddCrashMetaData(desc, value);
+ AddCrashMetaUploadFile(desc, path.value());
}
}
} else {
@@ -160,7 +174,7 @@
break;
}
}
- AddCrashMetaData(name, value_str);
+ AddCrashMetaUploadData(name, value_str);
}
at += size;
diff --git a/crash_reporter/chrome_collector.h b/crash_reporter/chrome_collector.h
index e25cd86..0a8a956 100644
--- a/crash_reporter/chrome_collector.h
+++ b/crash_reporter/chrome_collector.h
@@ -37,7 +37,8 @@
// For file values, name actually contains both a description and a filename,
// in a fixed format of: <description>"; filename="<filename>"
bool ParseCrashLog(const std::string &data, const base::FilePath &dir,
- const base::FilePath &minidump);
+ const base::FilePath &minidump,
+ const std::string &basename);
};
#endif
diff --git a/crash_reporter/chrome_collector_test.cc b/crash_reporter/chrome_collector_test.cc
index b24a194..9a5ee9b 100644
--- a/crash_reporter/chrome_collector_test.cc
+++ b/crash_reporter/chrome_collector_test.cc
@@ -68,7 +68,8 @@
TEST_F(ChromeCollectorTest, GoodValues) {
FilePath dir(".");
EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatGood,
- dir, dir.Append("minidump.dmp")));
+ dir, dir.Append("minidump.dmp"),
+ "base"));
// Check to see if the values made it in properly.
std::string meta = collector_.extra_metadata_;
@@ -79,7 +80,8 @@
TEST_F(ChromeCollectorTest, Newlines) {
FilePath dir(".");
EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatEmbeddedNewline,
- dir, dir.Append("minidump.dmp")));
+ dir, dir.Append("minidump.dmp"),
+ "base"));
// Check to see if the values were escaped.
std::string meta = collector_.extra_metadata_;
@@ -101,14 +103,16 @@
for (size_t i = 0; i < sizeof(list) / sizeof(list[0]); i++) {
chromeos::ClearLog();
EXPECT_FALSE(collector_.ParseCrashLog(list[i].data,
- dir, dir.Append("minidump.dmp")));
+ dir, dir.Append("minidump.dmp"),
+ "base"));
}
}
TEST_F(ChromeCollectorTest, File) {
FilePath dir(".");
EXPECT_TRUE(collector_.ParseCrashLog(kCrashFormatWithFile,
- dir, dir.Append("minidump.dmp")));
+ dir, dir.Append("minidump.dmp"),
+ "base"));
// Check to see if the values are still correct and that the file was
// written with the right data.
@@ -116,8 +120,9 @@
EXPECT_TRUE(meta.find("value1=abcdefghij") != std::string::npos);
EXPECT_TRUE(meta.find("value2=12345") != std::string::npos);
EXPECT_TRUE(meta.find("value3=ok") != std::string::npos);
- ExpectFileEquals("12345\n789\n12345", "foo.txt.other");
- file_util::Delete(dir.Append("foo.txt.other"), false);
+ ExpectFileEquals("12345\n789\n12345",
+ dir.Append("base-foo.txt.other").value().c_str());
+ file_util::Delete(dir.Append("base-foo.txt.other"), false);
}
int main(int argc, char **argv) {
diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc
index b433bec..d39d6f4 100644
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@@ -39,6 +39,8 @@
static const char kLsbRelease[] = "/etc/lsb-release";
static const char kShellPath[] = "/bin/sh";
static const char kSystemCrashPath[] = "/var/spool/crash";
+static const char kUploadVarPrefix[] = "upload_var_";
+static const char kUploadFilePrefix[] = "upload_file_";
// Normally this path is not used. Unfortunately, there are a few edge cases
// where we need this. Any process that runs as kDefaultUserName that crashes
// is consider a "user crash". That includes the initial Chrome browser that
@@ -479,6 +481,18 @@
extra_metadata_.append(StringPrintf("%s=%s\n", key.c_str(), value.c_str()));
}
+void CrashCollector::AddCrashMetaUploadFile(const std::string &key,
+ const std::string &path) {
+ if (!path.empty())
+ AddCrashMetaData(kUploadFilePrefix + key, path);
+}
+
+void CrashCollector::AddCrashMetaUploadData(const std::string &key,
+ const std::string &value) {
+ if (!value.empty())
+ AddCrashMetaData(kUploadVarPrefix + key, value);
+}
+
void CrashCollector::WriteCrashMetaData(const FilePath &meta_path,
const std::string &exec_name,
const std::string &payload_path) {
diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h
index 4cbe1b9..19ec97b 100644
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@@ -137,6 +137,16 @@
// "\n" characters. Value must not contain "\n" characters.
void AddCrashMetaData(const std::string &key, const std::string &value);
+ // Add a file to be uploaded to the crash reporter server. The file must
+ // persist until the crash report is sent; ideally it should live in the same
+ // place as the .meta file, so it can be cleaned up automatically.
+ void AddCrashMetaUploadFile(const std::string &key, const std::string &path);
+
+ // Add non-standard meta data to the crash metadata file.
+ // Data added though this call will be uploaded to the crash reporter server,
+ // appearing as a form field.
+ void AddCrashMetaUploadData(const std::string &key, const std::string &value);
+
// Write a file of metadata about crash.
void WriteCrashMetaData(const base::FilePath &meta_path,
const std::string &exec_name,
diff --git a/crash_reporter/crash_sender b/crash_reporter/crash_sender
index 6bebb6e..72205cf 100755
--- a/crash_reporter/crash_sender
+++ b/crash_reporter/crash_sender
@@ -240,16 +240,25 @@
}
get_key_value() {
- local file=$1 key=$2 value
+ local file="$1" key="$2" value
if [ -f "${file}" ]; then
# Return the first entry. There shouldn't be more than one anyways.
- value=$(awk -F= '/^'"${key}"'[[:space:]]*=/ { print $NF }' "${file}")
+ # Substr at length($1) + 2 skips past the key and following = sign (awk
+ # uses 1-based indexes), but preserves embedded = characters.
+ value=$(sed -n "/^${key}[[:space:]]*=/{s:^[^=]*=::p;q}" "${file}")
fi
echo "${value:-undefined}"
}
+get_keys() {
+ local file="$1" regex="$2"
+
+ awk -F'[[:space:]=]' -vregex="${regex}" \
+ 'match($1, regex) { print $1 }' "${file}"
+}
+
# Return the board name.
get_board() {
get_key_value "/etc/lsb-release" "CHROMEOS_RELEASE_BOARD"
@@ -286,6 +295,46 @@
local log="$(get_key_value "${meta_path}" "log")"
local sig="$(get_key_value "${meta_path}" "sig")"
local send_payload_size="$(stat --printf=%s "${report_payload}" 2>/dev/null)"
+ local product="$(get_key_value "${meta_path}" "upload_var_prod")"
+ local version="$(get_key_value "${meta_path}" "upload_var_ver")"
+ local upload_prefix="$(get_key_value "${meta_path}" "upload_prefix")"
+
+ set -- \
+ -F "write_payload_size=${write_payload_size}" \
+ -F "send_payload_size=${send_payload_size}"
+ if [ "${sig}" != "undefined" ]; then
+ set -- "$@" \
+ -F "sig=${sig}" \
+ -F "sig2=${sig}"
+ fi
+ if [ "${log}" != "undefined" ]; then
+ set -- "$@" \
+ -F "log=@${log}"
+ fi
+
+ if [ "${upload_prefix}" = "undefined" ]; then
+ upload_prefix=""
+ fi
+
+ # Grab any variable that begins with upload_.
+ local v
+ for k in $(get_keys "${meta_path}" "^upload_"); do
+ v="$(get_key_value "${meta_path}" "${k}")"
+ case ${k} in
+ # Product & version are handled separately.
+ upload_var_prod) ;;
+ upload_var_ver) ;;
+ upload_var_*) set -- "$@" -F "${upload_prefix}${k#upload_var_}=${v}" ;;
+ upload_file_*) set -- "$@" -F "${upload_prefix}${k#upload_file_}=@${v}" ;;
+ esac
+ done
+
+ # When uploading Chrome reports we need to report the right product and
+ # version, so allow the meta file to override these values.
+ if [ "${product}" = "undefined" ]; then
+ product=${CHROMEOS_PRODUCT}
+ version=${chromeos_version}
+ fi
local image_type
if is_test_image; then
@@ -307,38 +356,32 @@
boot_mode="dev"
fi
- local extra_key1="write_payload_size"
- local extra_value1="${write_payload_size}"
- local extra_key2="send_payload_size"
- local extra_value2="${send_payload_size}"
- if [ "${sig}" != "undefined" ]; then
- extra_key1="sig"
- extra_value1="${sig}"
- extra_key2="sig2"
- extra_value2="${sig}"
- elif [ "${log}" != "undefined" ]; then
- # Upload a log file if it was specified.
- extra_key1="log"
- extra_value1="@${log}"
- fi
-
local error_type="$(get_key_value "${meta_path}" "error_type")"
[ "${error_type}" = "undefined" ] && error_type=
lecho "Sending crash:"
+ if [ "${product}" != "${CHROMEOS_PRODUCT}" ]; then
+ lecho " Sending crash report on behalf of ${product}"
+ fi
lecho " Scheduled to send in ${sleep_time}s"
lecho " Metadata: ${meta_path} (${kind})"
lecho " Payload: ${report_payload}"
- lecho " Version: ${chromeos_version}"
+ lecho " Version: ${version}"
[ -n "${image_type}" ] && lecho " Image type: ${image_type}"
[ -n "${boot_mode}" ] && lecho " Boot mode: ${boot_mode}"
if is_mock; then
- lecho " Product: ${CHROMEOS_PRODUCT}"
+ lecho " Product: ${product}"
lecho " URL: ${url}"
lecho " Board: ${board}"
lecho " HWClass: ${hwclass}"
- lecho " ${extra_key1}: ${extra_value1}"
- lecho " ${extra_key2}: ${extra_value2}"
+ lecho " write_payload_size: ${write_payload_size}"
+ lecho " send_payload_size: ${send_payload_size}"
+ if [ "${log}" != "undefined" ]; then
+ lecho " log: @${log}"
+ fi
+ if [ "${sig}" != "undefined" ]; then
+ lecho " sig: ${sig}"
+ fi
fi
lecho " Exec name: ${exec_name}"
[ -n "${error_type}" ] && lecho " Error type: ${error_type}"
@@ -369,8 +412,8 @@
set +e
curl "${url}" ${proxy:+--proxy "$proxy"} \
--capath "${RESTRICTED_CERTIFICATES_PATH}" --ciphers HIGH \
- -F "prod=${CHROMEOS_PRODUCT}" \
- -F "ver=${chromeos_version}" \
+ -F "prod=${product}" \
+ -F "ver=${version}" \
-F "upload_file_${kind}=@${report_payload}" \
-F "board=${board}" \
-F "hwclass=${hwclass}" \
@@ -378,9 +421,10 @@
${image_type:+-F "image_type=${image_type}"} \
${boot_mode:+-F "boot_mode=${boot_mode}"} \
${error_type:+-F "error_type=${error_type}"} \
- -F "${extra_key1}=${extra_value1}" \
- -F "${extra_key2}=${extra_value2}" \
- -F "guid=<${CONSENT_ID}" -o "${report_id}" 2>"${curl_stderr}"
+ -F "guid=<${CONSENT_ID}" \
+ -o "${report_id}" \
+ "$@" \
+ 2>"${curl_stderr}"
curl_result=$?
set -e