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>
2013-06-19 15:34:13 -07:00 · 2013-06-19 15:34:13 -07:00 · 33dfd47146
commit 33dfd47146
parent 32f827f9dd
6 changed files with 134 additions and 46 deletions
--- a/crash_reporter/chrome_collector.cc
+++ b/crash_reporter/chrome_collector.cc
@ -45,6 +45,10 @@ bool ChromeCollector::HandleCrash(const std::string &file_path,
  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 @@ bool ChromeCollector::HandleCrash(const std::string &file_path,
    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::HandleCrash(const std::string &file_path,

 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 @@ bool ChromeCollector::ParseCrashLog(const std::string &data,
      // 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 @@ bool ChromeCollector::ParseCrashLog(const std::string &data,
           break;
        }
      }
-      AddCrashMetaData(name, value_str);
+      AddCrashMetaUploadData(name, value_str);
    }

    at += size;
--- a/crash_reporter/chrome_collector.h
+++ b/crash_reporter/chrome_collector.h
@ -37,7 +37,8 @@ class ChromeCollector : public CrashCollector {
  // 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
--- a/crash_reporter/chrome_collector_test.cc
+++ b/crash_reporter/chrome_collector_test.cc
@ -68,7 +68,8 @@ class ChromeCollectorTest : public ::testing::Test {
 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, GoodValues) {
 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 @@ TEST_F(ChromeCollectorTest, BadValues) {
  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 @@ TEST_F(ChromeCollectorTest, File) {
  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) {
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@ -39,6 +39,8 @@ static const char kLeaveCoreFile[] = "/root/.leave_core";
 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 @@ void CrashCollector::AddCrashMetaData(const std::string &key,
  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) {
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@ -137,6 +137,16 @@ class CrashCollector {
  // "\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,
--- a/crash_reporter/crash_sender
+++ b/crash_reporter/crash_sender
@ -240,16 +240,25 @@ get_kind() {
 }

 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 @@ send_crash() {
  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 @@ send_crash() {
    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 @@ send_crash() {
  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 @@ send_crash() {
    ${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