Expand the MachOImageAnnotationsReader 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: I8ebabb4f5c77c5620b0d8e5036c3185eecfa4646
Reviewed-on: https://chromium-review.googlesource.com/717236
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

snapshot/mac/mach_o_image_annotations_reader.cc

@@ -25,6 +25,7 @@
 #include "client/simple_string_dictionary.h"
 #include "snapshot/mac/mach_o_image_reader.h"
 #include "snapshot/mac/process_reader.h"
+#include "snapshot/snapshot_constants.h"
 #include "util/mach/task_memory.h"
 #include "util/stdlib/strnlen.h"
 
@@ -57,6 +58,15 @@ std::map<std::string, std::string> MachOImageAnnotationsReader::SimpleMap()
   return simple_map_annotations;
 }
 
+std::vector<AnnotationSnapshot> MachOImageAnnotationsReader::AnnotationsList()
+    const {
+  std::vector<AnnotationSnapshot> annotations;
+
+  ReadCrashpadAnnotationsList(&annotations);
+
+  return annotations;
+}
+
 void MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations(
     std::vector<std::string>* vector_annotations) const {
   mach_vm_address_t crash_info_address;
@@ -173,4 +183,64 @@ void MachOImageAnnotationsReader::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 PEImageAnnotationsReader.
+void MachOImageAnnotationsReader::ReadCrashpadAnnotationsList(
+    std::vector<AnnotationSnapshot>* annotations) const {
+  process_types::CrashpadInfo crashpad_info;
+  if (!image_reader_->GetCrashpadInfo(&crashpad_info)) {
+    return;
+  }
+
+  if (!crashpad_info.annotations_list) {
+    return;
+  }
+
+  process_types::AnnotationList annotation_list_object;
+  if (!annotation_list_object.Read(process_reader_,
+                                   crashpad_info.annotations_list)) {
+    LOG(WARNING) << "could not read annotations list object in " << name_;
+    return;
+  }
+
+  process_types::Annotation current = annotation_list_object.head;
+  for (size_t index = 0;
+       current.link_node != annotation_list_object.tail_pointer &&
+       index < kMaxNumberOfAnnotations;
+       ++index) {
+    if (!current.Read(process_reader_, current.link_node)) {
+      LOG(WARNING) << "could not read annotation at index " << index << " in "
+                   << name_;
+      return;
+    }
+
+    if (current.size == 0) {
+      continue;
+    }
+
+    AnnotationSnapshot snapshot;
+    snapshot.type = current.type;
+    snapshot.value.resize(current.size);
+
+    if (!process_reader_->Memory()->ReadCStringSizeLimited(
+            current.name, Annotation::kNameMaxLength, &snapshot.name)) {
+      LOG(WARNING) << "could not read annotation name at index " << index
+                   << " in " << name_;
+      continue;
+    }
+
+    size_t size =
+        std::min(static_cast<size_t>(current.size), Annotation::kValueMaxSize);
+    if (!process_reader_->Memory()->Read(
+            current.value, size, snapshot.value.data())) {
+      LOG(WARNING) << "could not read annotation value at index " << index
+                   << " in " << name_;
+      continue;
+    }
+
+    annotations->push_back(std::move(snapshot));
+  }
+}
+
 }  // namespace crashpad

snapshot/mac/mach_o_image_annotations_reader.h

@@ -20,6 +20,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "snapshot/annotation_snapshot.h"
 #include "snapshot/mac/process_types.h"
 
 namespace crashpad {
@@ -67,6 +68,10 @@ class MachOImageAnnotationsReader {
   //!     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:
   // Reades crashreporter_annotations_t::message and
   // crashreporter_annotations_t::message2 on behalf of Vector().
@@ -81,6 +86,10 @@ class MachOImageAnnotationsReader {
   void ReadCrashpadSimpleAnnotations(
       std::map<std::string, std::string>* simple_map_annotations) const;
 
+  // Reads CrashpadInfo::annotations_list_ on behalf of AnnotationsList().
+  void ReadCrashpadAnnotationsList(
+      std::vector<AnnotationSnapshot>* vector_annotations) const;
+
   std::string name_;
   ProcessReader* process_reader_;  // weak
   const MachOImageReader* image_reader_;  // weak

snapshot/mac/mach_o_image_annotations_reader_test.cc

@@ -28,6 +28,8 @@
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "client/annotation.h"
+#include "client/annotation_list.h"
 #include "client/crashpad_info.h"
 #include "client/simple_string_dictionary.h"
 #include "gtest/gtest.h"
@@ -248,10 +250,12 @@ class TestMachOImageAnnotationsReader final
     char c;
     CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c));
 
-    // Verify the “simple map” annotations set via the CrashpadInfo interface.
+    // Verify the “simple map” and object-based annotations set via the
+    // CrashpadInfo interface.
     const std::vector<ProcessReader::Module>& modules =
         process_reader.Modules();
     std::map<std::string, std::string> all_annotations_simple_map;
+    std::vector<AnnotationSnapshot> all_annotations;
     for (const ProcessReader::Module& module : modules) {
       MachOImageAnnotationsReader module_annotations_reader(
           &process_reader, module.reader, module.name);
@@ -259,6 +263,11 @@ class TestMachOImageAnnotationsReader final
           module_annotations_reader.SimpleMap();
       all_annotations_simple_map.insert(module_annotations_simple_map.begin(),
                                         module_annotations_simple_map.end());
+
+      std::vector<AnnotationSnapshot> annotations =
+          module_annotations_reader.AnnotationsList();
+      all_annotations.insert(
+          all_annotations.end(), annotations.begin(), annotations.end());
     }
 
     EXPECT_GE(all_annotations_simple_map.size(), 5u);
@@ -268,6 +277,31 @@ class TestMachOImageAnnotationsReader final
     EXPECT_EQ(all_annotations_simple_map["#TEST# longer"], "shorter");
     EXPECT_EQ(all_annotations_simple_map["#TEST# empty_value"], "");
 
+    EXPECT_EQ(all_annotations.size(), 3u);
+    bool saw_same_name_3 = false, saw_same_name_4 = false;
+    for (const auto& annotation : all_annotations) {
+      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 that it’s permitted to crash.
     CheckedWriteFile(WritePipeHandle(), &c, sizeof(c));
 
@@ -329,6 +363,19 @@ class TestMachOImageAnnotationsReader final
 
     crashpad_info->set_simple_annotations(simple_annotations);
 
+    AnnotationList::Register();  // This is “leaked” to crashpad_info.
+
+    static StringAnnotation<32> test_annotation_one{"#TEST# one"};
+    static StringAnnotation<32> test_annotation_two{"#TEST# two"};
+    static StringAnnotation<32> test_annotation_three{"#TEST# same-name"};
+    static StringAnnotation<32> test_annotation_four{"#TEST# same-name"};
+
+    test_annotation_one.Set("moocow");
+    test_annotation_two.Set("this will be cleared");
+    test_annotation_three.Set("same-name 3");
+    test_annotation_four.Set("same-name 4");
+    test_annotation_two.Clear();
+
     // Tell the parent that the environment has been set up.
     char c = '\0';
     CheckedWriteFile(WritePipeHandle(), &c, sizeof(c));

snapshot/mac/process_types/all.proctype

@@ -19,6 +19,7 @@
 // snapshot/mac/process_types.cc to produce process type struct definitions and
 // accessors.
 
+#include "snapshot/mac/process_types/annotation.proctype"
 #include "snapshot/mac/process_types/crashpad_info.proctype"
 #include "snapshot/mac/process_types/crashreporterclient.proctype"
 #include "snapshot/mac/process_types/dyld_images.proctype"

snapshot/mac/process_types/annotation.proctype

@@ -0,0 +1,31 @@
+// Copyright 2017 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+PROCESS_TYPE_STRUCT_BEGIN(Annotation)
+  PROCESS_TYPE_STRUCT_MEMBER(Pointer, link_node)
+  PROCESS_TYPE_STRUCT_MEMBER(Pointer, name)
+  PROCESS_TYPE_STRUCT_MEMBER(Pointer, value)
+  PROCESS_TYPE_STRUCT_MEMBER(uint32_t, size)
+  PROCESS_TYPE_STRUCT_MEMBER(uint16_t, type)
+PROCESS_TYPE_STRUCT_END(Annotation)
+
+#if !defined(PROCESS_TYPE_STRUCT_IMPLEMENT_ARRAY)
+
+PROCESS_TYPE_STRUCT_BEGIN(AnnotationList)
+  PROCESS_TYPE_STRUCT_MEMBER(Pointer, tail_pointer)
+  PROCESS_TYPE_STRUCT_MEMBER(crashpad::process_types::Annotation, head)
+  PROCESS_TYPE_STRUCT_MEMBER(crashpad::process_types::Annotation, tail)
+PROCESS_TYPE_STRUCT_END(AnnotationList)
+
+#endif  // !defined(PROCESS_TYPE_STRUCT_IMPLEMENT_ARRAY)

snapshot/snapshot.gyp

@@ -86,6 +86,7 @@
         'mac/process_types.cc',
         'mac/process_types.h',
         'mac/process_types/all.proctype',
+        'mac/process_types/annotation.proctype',
         'mac/process_types/crashpad_info.proctype',
         'mac/process_types/crashreporterclient.proctype',
         'mac/process_types/custom.cc',
@@ -114,6 +115,7 @@
         'posix/timezone.cc',
         'posix/timezone.h',
         'process_snapshot.h',
+        'snapshot_constants.h',
         'system_snapshot.h',
         'thread_snapshot.h',
         'unloaded_module_snapshot.cc',

snapshot/snapshot_constants.h

@@ -0,0 +1,28 @@
+// Copyright 2017 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef SNAPSHOT_SNAPSHOT_CONSTANTS_H_
+#define SNAPSHOT_SNAPSHOT_CONSTANTS_H_
+
+namespace crashpad {
+
+//! \brief The maximum number of crashpad::Annotations that will be read from
+//!     a client process.
+//!
+//! \note This maximum was chosen arbitrarily and may change in the future.
+constexpr size_t kMaxNumberOfAnnotations = 200;
+
+}  // namespace crashpad
+
+#endif  // SNAPSHOT_SNAPSHOT_CONSTANTS_H_