Expand the PEImageAnnotationsReader to read Annotation objects.

Nothing currently directs the handler to read these Annotation objects
from the target process, so they will not be read by Crashpad nor appear
in the minidump.

Bug: crashpad:192
Change-Id: I1eb1e9f42282c07e37d335631f0cc6083ef28a89
Reviewed-on: https://chromium-review.googlesource.com/726501
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

snapshot/snapshot_test.gyp

@@ -247,7 +247,7 @@
           },
         },
         {
-          'target_name': 'crashpad_snapshot_test_simple_annotations',
+          'target_name': 'crashpad_snapshot_test_annotations',
           'type': 'executable',
           'dependencies': [
             '../client/client.gyp:crashpad_client',
@@ -255,7 +255,7 @@
             '../third_party/mini_chromium/mini_chromium.gyp:base',
           ],
           'sources': [
-            'win/crashpad_snapshot_test_simple_annotations.cc',
+            'win/crashpad_snapshot_test_annotations.cc',
           ],
         },
       ],

snapshot/win/crashpad_snapshot_test_annotations.cc

@@ -15,6 +15,8 @@
 #include <windows.h>
 
 #include "base/logging.h"
+#include "client/annotation.h"
+#include "client/annotation_list.h"
 #include "client/crashpad_info.h"
 #include "util/file/file_io.h"
 
@@ -34,6 +36,20 @@ int wmain(int argc, wchar_t* argv[]) {
 
   crashpad_info->set_simple_annotations(simple_annotations);
 
+  // Set the annotation objects.
+  crashpad::AnnotationList::Register();
+
+  static crashpad::StringAnnotation<32> annotation_one("#TEST# one");
+  static crashpad::StringAnnotation<32> annotation_two("#TEST# two");
+  static crashpad::StringAnnotation<32> annotation_three("#TEST# same-name");
+  static crashpad::StringAnnotation<32> annotation_four("#TEST# same-name");
+
+  annotation_one.Set("moocow");
+  annotation_two.Set("this will be cleared");
+  annotation_three.Set("same-name 3");
+  annotation_four.Set("same-name 4");
+  annotation_two.Clear();
+
   // Tell the parent that the environment has been set up.
   HANDLE out = GetStdHandle(STD_OUTPUT_HANDLE);
   PCHECK(out != INVALID_HANDLE_VALUE) << "GetStdHandle";

snapshot/win/pe_image_annotations_reader.cc

@@ -18,13 +18,35 @@
 #include <sys/types.h>
 
 #include "base/strings/utf_string_conversions.h"
+#include "client/annotation.h"
 #include "client/simple_string_dictionary.h"
+#include "snapshot/snapshot_constants.h"
 #include "snapshot/win/pe_image_reader.h"
 #include "snapshot/win/process_reader_win.h"
 #include "util/win/process_structs.h"
 
 namespace crashpad {
 
+namespace process_types {
+
+template <class Traits>
+struct Annotation {
+  typename Traits::Pointer link_node;
+  typename Traits::Pointer name;
+  typename Traits::Pointer value;
+  uint32_t size;
+  uint16_t type;
+};
+
+template <class Traits>
+struct AnnotationList {
+  typename Traits::Pointer tail_pointer;
+  Annotation<Traits> head;
+  Annotation<Traits> tail;
+};
+
+}  // namespace process_types
+
 PEImageAnnotationsReader::PEImageAnnotationsReader(
     ProcessReaderWin* process_reader,
     const PEImageReader* pe_image_reader,
@@ -46,6 +68,19 @@ std::map<std::string, std::string> PEImageAnnotationsReader::SimpleMap() const {
   return simple_map_annotations;
 }
 
+std::vector<AnnotationSnapshot> PEImageAnnotationsReader::AnnotationsList()
+    const {
+  std::vector<AnnotationSnapshot> annotations;
+  if (process_reader_->Is64Bit()) {
+    ReadCrashpadAnnotationsList<process_types::internal::Traits64>(
+        &annotations);
+  } else {
+    ReadCrashpadAnnotationsList<process_types::internal::Traits32>(
+        &annotations);
+  }
+  return annotations;
+}
+
 template <class Traits>
 void PEImageAnnotationsReader::ReadCrashpadSimpleAnnotations(
     std::map<std::string, std::string>* simple_map_annotations) const {
@@ -80,4 +115,71 @@ void PEImageAnnotationsReader::ReadCrashpadSimpleAnnotations(
   }
 }
 
+// TODO(rsesek): When there is a platform-agnostic remote memory reader
+// interface available, use it so that the implementation is not duplicated
+// in the MachOImageAnnotationsReader.
+template <class Traits>
+void PEImageAnnotationsReader::ReadCrashpadAnnotationsList(
+    std::vector<AnnotationSnapshot>* vector_annotations) const {
+  process_types::CrashpadInfo<Traits> crashpad_info;
+  if (!pe_image_reader_->GetCrashpadInfo(&crashpad_info)) {
+    return;
+  }
+
+  if (!crashpad_info.annotations_list) {
+    return;
+  }
+
+  process_types::AnnotationList<Traits> annotation_list_object;
+  if (!process_reader_->ReadMemory(crashpad_info.annotations_list,
+                                   sizeof(annotation_list_object),
+                                   &annotation_list_object)) {
+    LOG(WARNING) << "could not read annotations list object in "
+                 << base::UTF16ToUTF8(name_);
+    return;
+  }
+
+  process_types::Annotation<Traits> current = annotation_list_object.head;
+  for (size_t index = 0;
+       current.link_node != annotation_list_object.tail_pointer &&
+       index < kMaxNumberOfAnnotations;
+       ++index) {
+    if (!process_reader_->ReadMemory(
+            current.link_node, sizeof(current), &current)) {
+      LOG(WARNING) << "could not read annotation at index " << index << " in "
+                   << base::UTF16ToUTF8(name_);
+      return;
+    }
+
+    if (current.size == 0) {
+      continue;
+    }
+
+    AnnotationSnapshot snapshot;
+    snapshot.type = current.type;
+
+    char name[Annotation::kNameMaxLength];
+    if (!process_reader_->ReadMemory(current.name, arraysize(name), name)) {
+      LOG(WARNING) << "could not read annotation name at index " << index
+                   << " in " << base::UTF16ToUTF8(name_);
+      continue;
+    }
+
+    size_t name_length = strnlen(name, Annotation::kNameMaxLength);
+    snapshot.name = std::string(name, name_length);
+
+    size_t value_length =
+        std::min(static_cast<size_t>(current.size), Annotation::kValueMaxSize);
+    snapshot.value.resize(value_length);
+    if (!process_reader_->ReadMemory(
+            current.value, value_length, snapshot.value.data())) {
+      LOG(WARNING) << "could not read annotation value at index " << index
+                   << " in " << base::UTF16ToUTF8(name_);
+      continue;
+    }
+
+    vector_annotations->push_back(std::move(snapshot));
+  }
+}
+
 }  // namespace crashpad

snapshot/win/pe_image_annotations_reader.h

@@ -20,6 +20,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "snapshot/annotation_snapshot.h"
 
 namespace crashpad {
 
@@ -54,12 +55,21 @@ class PEImageAnnotationsReader {
   //!     pairs, where all keys and values are strings.
   std::map<std::string, std::string> SimpleMap() const;
 
+  //! \brief Returns the module's annotations that are organized as a list of
+  //!     typed annotation objects.
+  std::vector<AnnotationSnapshot> AnnotationsList() const;
+
  private:
   // Reads CrashpadInfo::simple_annotations_ on behalf of SimpleMap().
   template <class Traits>
   void ReadCrashpadSimpleAnnotations(
       std::map<std::string, std::string>* simple_map_annotations) const;
 
+  // Reads CrashpadInfo::annotations_list_ on behalf of AnnotationsList().
+  template <class Traits>
+  void ReadCrashpadAnnotationsList(
+      std::vector<AnnotationSnapshot>* vector_annotations) const;
+
   std::wstring name_;
   ProcessReaderWin* process_reader_;  // weak
   const PEImageReader* pe_image_reader_;  // weak

snapshot/win/pe_image_annotations_reader_test.cc

@@ -27,6 +27,7 @@
 #include "client/crashpad_info.h"
 #include "client/simple_string_dictionary.h"
 #include "gtest/gtest.h"
+#include "snapshot/annotation_snapshot.h"
 #include "snapshot/win/pe_image_reader.h"
 #include "snapshot/win/process_reader_win.h"
 #include "test/gtest_disabled.h"
@@ -54,7 +55,7 @@ void TestAnnotationsOnCrash(TestType type, const base::FilePath& directory) {
                                                        .BaseName()
                                                        .RemoveFinalExtension()
                                                        .value() +
-                                                   L"_simple_annotations.exe")
+                                                   L"_annotations.exe")
                                            .value();
   ChildLauncher child(child_test_executable, L"");
   ASSERT_NO_FATAL_FAILURE(child.Start());
@@ -68,9 +69,11 @@ void TestAnnotationsOnCrash(TestType type, const base::FilePath& directory) {
   ASSERT_TRUE(process_reader.Initialize(child.process_handle(),
                                         ProcessSuspensionState::kRunning));
 
-  // Verify the "simple map" annotations set via the CrashpadInfo interface.
+  // Read all the kinds of annotations referenced from the CrashpadInfo
+  // structure.
   const std::vector<ProcessInfo::Module>& modules = process_reader.Modules();
   std::map<std::string, std::string> all_annotations_simple_map;
+  std::vector<AnnotationSnapshot> all_annotation_objects;
   for (const ProcessInfo::Module& module : modules) {
     PEImageReader pe_image_reader;
     pe_image_reader.Initialize(&process_reader,
@@ -79,12 +82,19 @@ void TestAnnotationsOnCrash(TestType type, const base::FilePath& directory) {
                                base::UTF16ToUTF8(module.name));
     PEImageAnnotationsReader module_annotations_reader(
         &process_reader, &pe_image_reader, module.name);
+
     std::map<std::string, std::string> module_annotations_simple_map =
         module_annotations_reader.SimpleMap();
     all_annotations_simple_map.insert(module_annotations_simple_map.begin(),
                                       module_annotations_simple_map.end());
+
+    auto module_annotations_list = module_annotations_reader.AnnotationsList();
+    all_annotation_objects.insert(all_annotation_objects.end(),
+                                  module_annotations_list.begin(),
+                                  module_annotations_list.end());
   }
 
+  // Verify the "simple map" annotations.
   EXPECT_GE(all_annotations_simple_map.size(), 5u);
   EXPECT_EQ(all_annotations_simple_map["#TEST# pad"], "crash");
   EXPECT_EQ(all_annotations_simple_map["#TEST# key"], "value");
@@ -92,6 +102,32 @@ void TestAnnotationsOnCrash(TestType type, const base::FilePath& directory) {
   EXPECT_EQ(all_annotations_simple_map["#TEST# longer"], "shorter");
   EXPECT_EQ(all_annotations_simple_map["#TEST# empty_value"], "");
 
+  // Verify the typed annotation objects.
+  EXPECT_EQ(all_annotation_objects.size(), 3);
+  bool saw_same_name_3 = false, saw_same_name_4 = false;
+  for (const auto& annotation : all_annotation_objects) {
+    EXPECT_EQ(annotation.type,
+              static_cast<uint16_t>(Annotation::Type::kString));
+    std::string value(reinterpret_cast<const char*>(annotation.value.data()),
+                      annotation.value.size());
+
+    if (annotation.name == "#TEST# one") {
+      EXPECT_EQ(value, "moocow");
+    } else if (annotation.name == "#TEST# same-name") {
+      if (value == "same-name 3") {
+        EXPECT_FALSE(saw_same_name_3);
+        saw_same_name_3 = true;
+      } else if (value == "same-name 4") {
+        EXPECT_FALSE(saw_same_name_4);
+        saw_same_name_4 = true;
+      } else {
+        ADD_FAILURE() << "unexpected annotation value " << value;
+      }
+    } else {
+      ADD_FAILURE() << "unexpected annotation " << annotation.name;
+    }
+  }
+
   // Tell the child process to continue.
   DWORD expected_exit_code;
   switch (type) {