From 1c5533d4cd99f19ec8a37aaa841caeba76f7005c Mon Sep 17 00:00:00 2001 From: Ben Zhang Date: Tue, 20 Jan 2015 17:26:31 -0800 Subject: [PATCH] crash-reporter: add a sanity check for kernel dmesg records On some devices, after a cold boot, a junk pstore record /dev/pstore/dmesg-ramoops-0 is created which is just a chunk of uninitialized memory containing random bits, and it's not the result of a kernel crash. The sanity check scans for the dmesg log level pattern to avoid creating junk .kcrash files. BUG=chromium:443764 TEST=platform_KernelErrorPaths with 3.8 and 3.14 kernel; check no kcrash file is created for random binary ramoops dump on stumpy. Change-Id: I83041436cd8e5e0c7c0015c529f462032ce82f30 Signed-off-by: Ben Zhang Reviewed-on: https://chromium-review.googlesource.com/242147 Reviewed-by: Olof Johansson Reviewed-by: Grant Grundler Reviewed-by: Mike Frysinger --- crash_reporter/kernel_collector.cc | 21 ++++++++++++++++----- crash_reporter/kernel_collector_test.cc | 10 ++++++++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/crash_reporter/kernel_collector.cc b/crash_reporter/kernel_collector.cc index 98b3e8f26..d86efbdcc 100644 --- a/crash_reporter/kernel_collector.cc +++ b/crash_reporter/kernel_collector.cc @@ -89,6 +89,8 @@ bool KernelCollector::ReadRecordToString(std::string *contents, "====\\d+\\.\\d+\n(.*)", pcrecpp::RE_Options().set_multiline(true).set_dotall(true)); + pcrecpp::RE sanity_check_re("\n<\\d+>\\[\\s*(\\d+\\.\\d+)\\]"); + FilePath ramoops_record; GetRamoopsRecordPath(&ramoops_record, current_record); if (!base::ReadFileToString(ramoops_record, &record)) { @@ -96,18 +98,27 @@ bool KernelCollector::ReadRecordToString(std::string *contents, return false; } - *record_found = true; + *record_found = false; if (record_re.FullMatch(record, &captured)) { - // Found a match, append it to the content, and remove from pstore. + // Found a ramoops header, so strip the header and append the rest. contents->append(captured); - } else { + *record_found = true; + } else if (sanity_check_re.PartialMatch(record.substr(0, 1024))) { // pstore compression has been added since kernel 3.12. In order to // decompress dmesg correctly, ramoops driver has to strip the header // before handing over the record to the pstore driver, so we don't - // need to do it here anymore. + // need to do it here anymore. However, the sanity check is needed because + // sometimes a pstore record is just a chunk of uninitialized memory which + // is not the result of a kernel crash. See crbug.com/443764 contents->append(record); + *record_found = true; + } else { + LOG(WARNING) << "Found invalid record at " << ramoops_record.value(); } - base::DeleteFile(ramoops_record, false); + + // Remove the record from pstore after it's found. + if (*record_found) + base::DeleteFile(ramoops_record, false); return true; } diff --git a/crash_reporter/kernel_collector_test.cc b/crash_reporter/kernel_collector_test.cc index 6284f49ef..08bb8b0ac 100644 --- a/crash_reporter/kernel_collector_test.cc +++ b/crash_reporter/kernel_collector_test.cc @@ -78,15 +78,21 @@ TEST_F(KernelCollectorTest, LoadPreservedDump) { std::string dump; dump.clear(); - WriteStringToFile(kcrash_file(), "CrashRecordWithoutRamoopsHeader"); + WriteStringToFile(kcrash_file(), + "CrashRecordWithoutRamoopsHeader\n<6>[ 0.078852]"); ASSERT_TRUE(collector_.LoadParameters()); ASSERT_TRUE(collector_.LoadPreservedDump(&dump)); - ASSERT_EQ("CrashRecordWithoutRamoopsHeader", dump); + ASSERT_EQ("CrashRecordWithoutRamoopsHeader\n<6>[ 0.078852]", dump); WriteStringToFile(kcrash_file(), "====1.1\nsomething"); ASSERT_TRUE(collector_.LoadParameters()); ASSERT_TRUE(collector_.LoadPreservedDump(&dump)); ASSERT_EQ("something", dump); + + WriteStringToFile(kcrash_file(), "\x01\x02\xfe\xff random blob"); + ASSERT_TRUE(collector_.LoadParameters()); + ASSERT_FALSE(collector_.LoadPreservedDump(&dump)); + ASSERT_EQ("", dump); } TEST_F(KernelCollectorTest, EnableMissingKernel) {