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 <benzh@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/242401
Reviewed-by: Grant Grundler <grundler@chromium.org>
diff --git a/crash-reporter/kernel_collector.cc b/crash-reporter/kernel_collector.cc
index 98b3e8f..d86efbd 100644
--- a/crash-reporter/kernel_collector.cc
+++ b/crash-reporter/kernel_collector.cc
@@ -89,6 +89,8 @@
"====\\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 @@
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 6284f49..08bb8b0 100644
--- a/crash-reporter/kernel_collector_test.cc
+++ b/crash-reporter/kernel_collector_test.cc
@@ -78,15 +78,21 @@
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) {
