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 <jperaza@chromium.org>

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 <cassert>
 #include <map>
 #include <string>
 #include <utility>
 
 #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<Inline> in(new Inline);
-    in.get()->CopyFrom(inline_ptr);
+    in->CopyFrom(inline_ptr);
     unique_ptr<StackFrame> new_frame =
         unique_ptr<StackFrame>(new StackFrame(*frame));
     auto origin_iter = inline_origins_.find(in->origin_id);
     if (origin_iter != inline_origins_.end()) {
       scoped_ptr<InlineOrigin> 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 = "<name omitted>";
@@ -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<MemAddr, Function>(mem_buffer + offsets[map_id++]);
   public_symbols_ =
       StaticAddressMap<MemAddr, PublicSymbol>(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<MemAddr, char>(mem_buffer + offsets[map_id++]);
+  }
 
   cfi_initial_rules_ =
       StaticRangeMap<MemAddr, char>(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;
   }
 

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 <cstdint>
 #include <map>
 #include <string>
 
+#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<bool>::Read(raw, &is_multiple);
     int32_t inline_size;
     DESERIALIZE(raw, inline_size);
     inlines = StaticContainedRangeMap<MemAddr, char>(raw);
@@ -152,7 +153,7 @@ public SourceLineResolverBase::PublicSymbol {
     raw += name_size;
     DESERIALIZE(raw, address);
     DESERIALIZE(raw, parameter_size);
-    DESERIALIZE(raw, is_multiple);
+    raw = SimpleSerializer<bool>::Read(raw, &is_multiple);
   }
 };
 

src/processor/simple_serializer-inl.h

@@ -38,14 +38,15 @@
 #ifndef PROCESSOR_SIMPLE_SERIALIZER_INL_H__
 #define PROCESSOR_SIMPLE_SERIALIZER_INL_H__
 
-#include <string>
-
 #include "processor/simple_serializer.h"
-#include "map_serializers-inl.h"
+
+#include <cstdint>
+#include <string>
 
 #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<BasicSourceLineResolver::PublicSymbol> {
   static size_t SizeOf(const PublicSymbol& pubsymbol) {
     return SimpleSerializer<string>::SizeOf(pubsymbol.name)
          + SimpleSerializer<MemAddr>::SizeOf(pubsymbol.address)
-         + SimpleSerializer<int32_t>::SizeOf(pubsymbol.parameter_size);
+         + SimpleSerializer<int32_t>::SizeOf(pubsymbol.parameter_size)
+         + SimpleSerializer<bool>::SizeOf(pubsymbol.is_multiple);
   }
   static char* Write(const PublicSymbol& pubsymbol, char* dest) {
     dest = SimpleSerializer<string>::Write(pubsymbol.name, dest);
     dest = SimpleSerializer<MemAddr>::Write(pubsymbol.address, dest);
     dest = SimpleSerializer<int32_t>::Write(pubsymbol.parameter_size, dest);
+    dest = SimpleSerializer<bool>::Write(pubsymbol.is_multiple, dest);
     return dest;
   }
 };

src/processor/simple_serializer.h

@@ -38,6 +38,8 @@
 #ifndef PROCESSOR_SIMPLE_SERIALIZER_H__
 #define PROCESSOR_SIMPLE_SERIALIZER_H__
 
+#include <stddef.h>
+
 #include "google_breakpad/common/breakpad_types.h"
 
 namespace google_breakpad {

src/processor/source_line_resolver_base.cc

@@ -42,10 +42,10 @@
 #include <utility>
 
 #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());