From e4723d524f196826ba8275402da0c117c767da38 Mon Sep 17 00:00:00 2001 From: Robert Sesek Date: Tue, 31 Oct 2017 17:14:27 -0400 Subject: [PATCH] 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 Reviewed-by: Mark Mentovai --- .../mac/mach_o_image_annotations_reader.cc | 70 +++++++++++++++++++ .../mac/mach_o_image_annotations_reader.h | 9 +++ .../mach_o_image_annotations_reader_test.cc | 49 ++++++++++++- snapshot/mac/process_types/all.proctype | 1 + .../mac/process_types/annotation.proctype | 31 ++++++++ snapshot/snapshot.gyp | 2 + snapshot/snapshot_constants.h | 28 ++++++++ 7 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 snapshot/mac/process_types/annotation.proctype create mode 100644 snapshot/snapshot_constants.h diff --git a/snapshot/mac/mach_o_image_annotations_reader.cc b/snapshot/mac/mach_o_image_annotations_reader.cc index df8bebad..1a74d812 100644 --- a/snapshot/mac/mach_o_image_annotations_reader.cc +++ b/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 MachOImageAnnotationsReader::SimpleMap() return simple_map_annotations; } +std::vector MachOImageAnnotationsReader::AnnotationsList() + const { + std::vector annotations; + + ReadCrashpadAnnotationsList(&annotations); + + return annotations; +} + void MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations( std::vector* 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* 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(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 diff --git a/snapshot/mac/mach_o_image_annotations_reader.h b/snapshot/mac/mach_o_image_annotations_reader.h index 2ff6e0e4..06d2bea9 100644 --- a/snapshot/mac/mach_o_image_annotations_reader.h +++ b/snapshot/mac/mach_o_image_annotations_reader.h @@ -20,6 +20,7 @@ #include #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 SimpleMap() const; + //! \brief Returns the module’s annotations that are organized as a list of + // typed annotation objects. + std::vector 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* simple_map_annotations) const; + // Reads CrashpadInfo::annotations_list_ on behalf of AnnotationsList(). + void ReadCrashpadAnnotationsList( + std::vector* vector_annotations) const; + std::string name_; ProcessReader* process_reader_; // weak const MachOImageReader* image_reader_; // weak diff --git a/snapshot/mac/mach_o_image_annotations_reader_test.cc b/snapshot/mac/mach_o_image_annotations_reader_test.cc index 68d6c4e0..cda76116 100644 --- a/snapshot/mac/mach_o_image_annotations_reader_test.cc +++ b/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& modules = process_reader.Modules(); std::map all_annotations_simple_map; + std::vector 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 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(Annotation::Type::kString)); + std::string value(reinterpret_cast(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)); diff --git a/snapshot/mac/process_types/all.proctype b/snapshot/mac/process_types/all.proctype index 7ef54d5c..d84b41d1 100644 --- a/snapshot/mac/process_types/all.proctype +++ b/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" diff --git a/snapshot/mac/process_types/annotation.proctype b/snapshot/mac/process_types/annotation.proctype new file mode 100644 index 00000000..5d34fdab --- /dev/null +++ b/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) diff --git a/snapshot/snapshot.gyp b/snapshot/snapshot.gyp index e6d83892..73a5da04 100644 --- a/snapshot/snapshot.gyp +++ b/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', diff --git a/snapshot/snapshot_constants.h b/snapshot/snapshot_constants.h new file mode 100644 index 00000000..2bbddda3 --- /dev/null +++ b/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_