From 64b25d6653ad62cdd05c837afb56839d4dc60f5b Mon Sep 17 00:00:00 2001 From: Ivan Penkov Date: Thu, 9 Dec 2021 04:58:57 +0000 Subject: [PATCH] Fixing issues in the Breakpad symbol file serialization code. - FastSourceLineResolver::Module::LoadMapFromMemory now rejects an older version of the serialization format. - Cleaned up several unneeded usages of scoped_ptr::get. - Fixed the serialization of bool. The serialization code was using 255 for 'true' while the deserialization code was expecting to see 1. - Serialization for PublicSymbol.is_multiple was missing. Deserialization was expecting it - Added some logging to processor/source_line_resolver_base.cc Change-Id: Iadc7d8ee23bf3a07e4ea280d5d4c3f25f6278b69 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3324395 Reviewed-by: Joshua Peraza --- src/processor/fast_source_line_resolver.cc | 32 ++++++++++++++----- .../fast_source_line_resolver_types.h | 13 ++++---- src/processor/simple_serializer-inl.h | 11 ++++--- src/processor/simple_serializer.h | 2 ++ src/processor/source_line_resolver_base.cc | 13 +++++--- 5 files changed, 49 insertions(+), 22 deletions(-) diff --git a/src/processor/fast_source_line_resolver.cc b/src/processor/fast_source_line_resolver.cc index 374b06cf..81a6b7ac 100644 --- a/src/processor/fast_source_line_resolver.cc +++ b/src/processor/fast_source_line_resolver.cc @@ -40,12 +40,14 @@ #include "google_breakpad/processor/fast_source_line_resolver.h" #include "processor/fast_source_line_resolver_types.h" +#include #include #include #include #include "common/scoped_ptr.h" #include "common/using_std_string.h" +#include "processor/logging.h" #include "processor/module_factory.h" #include "processor/simple_serializer-inl.h" @@ -83,7 +85,7 @@ void FastSourceLineResolver::Module::LookupAddress( if (functions_.RetrieveNearestRange(address, func_ptr, &function_base, &function_size) && address >= function_base && address - function_base < function_size) { - func.get()->CopyFrom(func_ptr); + func->CopyFrom(func_ptr); frame->function_name = func->name; frame->function_base = frame->module->base_address() + function_base; @@ -91,7 +93,7 @@ void FastSourceLineResolver::Module::LookupAddress( const Line* line_ptr = 0; MemAddr line_base; if (func->lines.RetrieveRange(address, line_ptr, &line_base, NULL)) { - line.get()->CopyFrom(line_ptr); + line->CopyFrom(line_ptr); FileMap::iterator it = files_.find(line->source_file_id); if (it != files_.end()) { frame->source_file_name = @@ -107,7 +109,7 @@ void FastSourceLineResolver::Module::LookupAddress( } else if (public_symbols_.Retrieve(address, public_symbol_ptr, &public_address) && (!func_ptr || public_address > function_base)) { - public_symbol.get()->CopyFrom(public_symbol_ptr); + public_symbol->CopyFrom(public_symbol_ptr); frame->function_name = public_symbol->name; frame->function_base = frame->module->base_address() + public_address; } @@ -125,13 +127,13 @@ void FastSourceLineResolver::Module::ConstructInlineFrames( for (const char* inline_ptr : inline_ptrs) { scoped_ptr in(new Inline); - in.get()->CopyFrom(inline_ptr); + in->CopyFrom(inline_ptr); unique_ptr new_frame = unique_ptr(new StackFrame(*frame)); auto origin_iter = inline_origins_.find(in->origin_id); if (origin_iter != inline_origins_.end()) { scoped_ptr origin(new InlineOrigin); - origin.get()->CopyFrom(origin_iter.GetValuePtr()); + origin->CopyFrom(origin_iter.GetValuePtr()); new_frame->function_name = origin->name; } else { new_frame->function_name = ""; @@ -234,6 +236,19 @@ bool FastSourceLineResolver::Module::LoadMapFromMemory( for (int i = 1; i < kNumberMaps_; ++i) { offsets[i] = offsets[i - 1] + map_sizes[i - 1]; } + unsigned int expected_size = sizeof(bool) + offsets[kNumberMaps_ - 1] + + map_sizes[kNumberMaps_ - 1] + 1; + if (expected_size != memory_buffer_size && + // Allow for having an extra null terminator. + expected_size != memory_buffer_size - 1) { + // This could either be a random corruption or the serialization format was + // changed without updating the version in kSerializedBreakpadFileExtension. + BPLOG(ERROR) << "Memory buffer is either corrupt or an unsupported version" + << ", expected size: " << expected_size + << ", actual size: " << memory_buffer_size; + return false; + } + BPLOG(INFO) << "Memory buffer size looks good, size: " << memory_buffer_size; // Use pointers to construct Static*Map data members in Module: int map_id = 0; @@ -242,9 +257,10 @@ bool FastSourceLineResolver::Module::LoadMapFromMemory( StaticRangeMap(mem_buffer + offsets[map_id++]); public_symbols_ = StaticAddressMap(mem_buffer + offsets[map_id++]); - for (int i = 0; i < WindowsFrameInfo::STACK_INFO_LAST; ++i) + for (int i = 0; i < WindowsFrameInfo::STACK_INFO_LAST; ++i) { windows_frame_info_[i] = StaticContainedRangeMap(mem_buffer + offsets[map_id++]); + } cfi_initial_rules_ = StaticRangeMap(mem_buffer + offsets[map_id++]); @@ -286,7 +302,7 @@ WindowsFrameInfo* FastSourceLineResolver::Module::FindWindowsFrameInfo( if (functions_.RetrieveNearestRange(address, function_ptr, &function_base, &function_size) && address >= function_base && address - function_base < function_size) { - function.get()->CopyFrom(function_ptr); + function->CopyFrom(function_ptr); result->parameter_size = function->parameter_size; result->valid |= WindowsFrameInfo::VALID_PARAMETER_SIZE; return result.release(); @@ -299,7 +315,7 @@ WindowsFrameInfo* FastSourceLineResolver::Module::FindWindowsFrameInfo( MemAddr public_address; if (public_symbols_.Retrieve(address, public_symbol_ptr, &public_address) && (!function_ptr || public_address > function_base)) { - public_symbol.get()->CopyFrom(public_symbol_ptr); + public_symbol->CopyFrom(public_symbol_ptr); result->parameter_size = public_symbol->parameter_size; } diff --git a/src/processor/fast_source_line_resolver_types.h b/src/processor/fast_source_line_resolver_types.h index 577b98e6..718905df 100644 --- a/src/processor/fast_source_line_resolver_types.h +++ b/src/processor/fast_source_line_resolver_types.h @@ -37,15 +37,16 @@ #ifndef PROCESSOR_FAST_SOURCE_LINE_RESOLVER_TYPES_H__ #define PROCESSOR_FAST_SOURCE_LINE_RESOLVER_TYPES_H__ -#include "contained_range_map.h" -#include "google_breakpad/processor/fast_source_line_resolver.h" -#include "processor/source_line_resolver_base_types.h" - +#include #include #include +#include "google_breakpad/processor/fast_source_line_resolver.h" #include "google_breakpad/processor/stack_frame.h" #include "processor/cfi_frame_info.h" +#include "processor/contained_range_map.h" +#include "processor/simple_serializer-inl.h" +#include "processor/source_line_resolver_base_types.h" #include "processor/static_address_map-inl.h" #include "processor/static_contained_range_map-inl.h" #include "processor/static_map.h" @@ -88,7 +89,7 @@ public SourceLineResolverBase::Function { DESERIALIZE(raw, address); DESERIALIZE(raw, size); DESERIALIZE(raw, parameter_size); - DESERIALIZE(raw, is_multiple); + raw = SimpleSerializer::Read(raw, &is_multiple); int32_t inline_size; DESERIALIZE(raw, inline_size); inlines = StaticContainedRangeMap(raw); @@ -152,7 +153,7 @@ public SourceLineResolverBase::PublicSymbol { raw += name_size; DESERIALIZE(raw, address); DESERIALIZE(raw, parameter_size); - DESERIALIZE(raw, is_multiple); + raw = SimpleSerializer::Read(raw, &is_multiple); } }; diff --git a/src/processor/simple_serializer-inl.h b/src/processor/simple_serializer-inl.h index 4b4200e4..c8d3e3c2 100644 --- a/src/processor/simple_serializer-inl.h +++ b/src/processor/simple_serializer-inl.h @@ -38,14 +38,15 @@ #ifndef PROCESSOR_SIMPLE_SERIALIZER_INL_H__ #define PROCESSOR_SIMPLE_SERIALIZER_INL_H__ -#include - #include "processor/simple_serializer.h" -#include "map_serializers-inl.h" + +#include +#include #include "google_breakpad/processor/basic_source_line_resolver.h" #include "processor/basic_source_line_resolver_types.h" #include "processor/linked_ptr.h" +#include "processor/map_serializers-inl.h" #include "processor/windows_frame_info.h" namespace google_breakpad { @@ -140,12 +141,14 @@ class SimpleSerializer { static size_t SizeOf(const PublicSymbol& pubsymbol) { return SimpleSerializer::SizeOf(pubsymbol.name) + SimpleSerializer::SizeOf(pubsymbol.address) - + SimpleSerializer::SizeOf(pubsymbol.parameter_size); + + SimpleSerializer::SizeOf(pubsymbol.parameter_size) + + SimpleSerializer::SizeOf(pubsymbol.is_multiple); } static char* Write(const PublicSymbol& pubsymbol, char* dest) { dest = SimpleSerializer::Write(pubsymbol.name, dest); dest = SimpleSerializer::Write(pubsymbol.address, dest); dest = SimpleSerializer::Write(pubsymbol.parameter_size, dest); + dest = SimpleSerializer::Write(pubsymbol.is_multiple, dest); return dest; } }; diff --git a/src/processor/simple_serializer.h b/src/processor/simple_serializer.h index 9e51d806..e1b1efd1 100644 --- a/src/processor/simple_serializer.h +++ b/src/processor/simple_serializer.h @@ -38,6 +38,8 @@ #ifndef PROCESSOR_SIMPLE_SERIALIZER_H__ #define PROCESSOR_SIMPLE_SERIALIZER_H__ +#include + #include "google_breakpad/common/breakpad_types.h" namespace google_breakpad { diff --git a/src/processor/source_line_resolver_base.cc b/src/processor/source_line_resolver_base.cc index cea1a46d..46303348 100644 --- a/src/processor/source_line_resolver_base.cc +++ b/src/processor/source_line_resolver_base.cc @@ -42,10 +42,10 @@ #include #include "google_breakpad/processor/source_line_resolver_base.h" -#include "processor/source_line_resolver_base_types.h" +#include "processor/logging.h" #include "processor/module_factory.h" +#include "processor/source_line_resolver_base_types.h" -using std::map; using std::make_pair; namespace google_breakpad { @@ -168,7 +168,9 @@ bool SourceLineResolverBase::LoadModule(const CodeModule* module, if (!ReadSymbolFile(map_file, &memory_buffer, &memory_buffer_size)) return false; - BPLOG(INFO) << "Read symbol file " << map_file << " succeeded"; + BPLOG(INFO) << "Read symbol file " << map_file << " succeeded. " + << "module = " << module->code_file() + << ", memory_buffer_size = " << memory_buffer_size; bool load_result = LoadModuleUsingMemoryBuffer(module, memory_buffer, memory_buffer_size); @@ -185,6 +187,9 @@ bool SourceLineResolverBase::LoadModule(const CodeModule* module, bool SourceLineResolverBase::LoadModuleUsingMapBuffer( const CodeModule* module, const string& map_buffer) { + BPLOG(INFO) << "SourceLineResolverBase::LoadModuleUsingMapBuffer(module = " + << module->code_file() + << ", map_buffer.size() = " << map_buffer.size() << ")"; if (module == NULL) return false; @@ -234,7 +239,7 @@ bool SourceLineResolverBase::LoadModuleUsingMemoryBuffer( } BPLOG(INFO) << "Loading symbols for module " << module->code_file() - << " from memory buffer"; + << " from memory buffer, size: " << memory_buffer_size; Module* basic_module = module_factory_->CreateModule(module->code_file());