From 2bb6f068a8fcd08f6eeb725d7b00b06ac9c279b6 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Thu, 24 Feb 2022 16:38:28 -0500 Subject: [PATCH] Fix off-by-one error in ModuleSnapshotMinidump DebugFileName. Strings in minidumps are typically NUL-terminated (https://docs.microsoft.com/en-us/windows/win32/api/minidumpapiset/ns-minidumpapiset-minidump_string). But the CodeViewRecord types do not use MINDIUMP_STRINGs, and do not have a separate length field for the pdb_name. Instead, the strings are always NUL-terminated, with the length derived from the MINIDUMP_LOCATION_DESCRIPTOR::DataSize field. The writer is correctly NUL-terminating the debug filename, but ModuleSnapshotMinidump is off-by-one and including the NUL-terminator. Change-Id: I8d813b5ef9e9e167dca73a6a938fbbf8dd1580c2 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3482876 Reviewed-by: Mark Mentovai Commit-Queue: Justin Cohen --- snapshot/minidump/module_snapshot_minidump.cc | 10 ++++++++-- snapshot/minidump/process_snapshot_minidump_test.cc | 11 ++++++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/snapshot/minidump/module_snapshot_minidump.cc b/snapshot/minidump/module_snapshot_minidump.cc index 698f5357..7dad87a6 100644 --- a/snapshot/minidump/module_snapshot_minidump.cc +++ b/snapshot/minidump/module_snapshot_minidump.cc @@ -100,7 +100,7 @@ bool ModuleSnapshotMinidump::InitializeModuleCodeView( signature = *reinterpret_cast(cv_record.data()); if (signature == CodeViewRecordPDB70::kSignature) { - if (cv_record.size() < offsetof(CodeViewRecordPDB70, pdb_name)) { + if (cv_record.size() < offsetof(CodeViewRecordPDB70, pdb_name) + 1) { LOG(ERROR) << "CodeView record in module marked as PDB70 but too small"; return false; } @@ -111,8 +111,14 @@ bool ModuleSnapshotMinidump::InitializeModuleCodeView( age_ = cv_record_pdb70->age; uuid_ = cv_record_pdb70->uuid; + if (cv_record.back() != '\0') { + LOG(ERROR) << "CodeView record marked as PDB70 missing NUL-terminator in " + "pdb_name"; + return false; + } + std::copy(cv_record.begin() + offsetof(CodeViewRecordPDB70, pdb_name), - cv_record.end(), + cv_record.end() - 1, std::back_inserter(debug_file_name_)); return true; } diff --git a/snapshot/minidump/process_snapshot_minidump_test.cc b/snapshot/minidump/process_snapshot_minidump_test.cc index b9fa23d7..3c5dcf6c 100644 --- a/snapshot/minidump/process_snapshot_minidump_test.cc +++ b/snapshot/minidump/process_snapshot_minidump_test.cc @@ -350,6 +350,7 @@ TEST(ProcessSnapshotMinidump, Modules) { "libgeorgism", "librealistutopia", }; + constexpr char debug_name[] = "debugme.pdb"; minidump_module.BaseOfImage = 0xbadf00d; minidump_module.SizeOfImage = 9001; @@ -373,12 +374,15 @@ TEST(ProcessSnapshotMinidump, Modules) { pdb70_cv.signature = CodeViewRecordPDB70::kSignature; pdb70_cv.age = 7; pdb70_cv.uuid.InitializeFromString("00112233-4455-6677-8899-aabbccddeeff"); - pdb70_cv.pdb_name[0] = '\0'; auto pdb70_loc = static_cast(string_file.SeekGet()); - auto pdb70_size = sizeof(pdb70_cv); + auto pdb70_size = offsetof(CodeViewRecordPDB70, pdb_name); - EXPECT_TRUE(string_file.Write(&pdb70_cv, sizeof(pdb70_cv))); + EXPECT_TRUE(string_file.Write(&pdb70_cv, pdb70_size)); + + size_t nul_terminated_length = strlen(debug_name) + 1; + EXPECT_TRUE(string_file.Write(debug_name, nul_terminated_length)); + pdb70_size += nul_terminated_length; CodeViewRecordBuildID build_id_cv; build_id_cv.signature = CodeViewRecordBuildID::kSignature; @@ -545,6 +549,7 @@ TEST(ProcessSnapshotMinidump, Modules) { EXPECT_EQ(uuid.ToString(), "00112233-4455-6677-8899-aabbccddeeff"); EXPECT_EQ(age, 7U); + EXPECT_EQ(modules[i]->DebugFileName(), debug_name); } else { auto build_id = modules[i]->BuildID(); std::string build_id_text(build_id.data(),