Don't restrict ImageAnnotationReader to a module's address range

Annotations data structures may be dynamically allocated so could
appear outside a modules's address range. Let ImageAnnotationReader
use a ProcessMemoryRange for the process, rather than the module.

Also add a test for linux.

Bug: crashpad:30
Change-Id: Ibbf1d2fcb2e44b1b70c8a02e86c6f2fbd784535f
Reviewed-on: https://chromium-review.googlesource.com/1054705
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
This commit is contained in:
Joshua Peraza 2018-05-10 17:15:59 -07:00 committed by Commit Bot
parent e2b934cc40
commit 19e6087bb2
9 changed files with 78 additions and 26 deletions

View File

@ -127,6 +127,7 @@ source_set("client_test") {
deps = [
":client",
"../compat",
"../snapshot",
"../test",
"../third_party/gtest:gmock",
"../third_party/gtest:gtest",

View File

@ -24,6 +24,7 @@
'client.gyp:crashpad_client',
'../compat/compat.gyp:crashpad_compat',
'../handler/handler.gyp:crashpad_handler',
'../snapshot/snapshot.gyp:crashpad_snapshot',
'../test/test.gyp:crashpad_gmock_main',
'../test/test.gyp:crashpad_test',
'../third_party/gtest/gmock.gyp:gmock',

View File

@ -21,14 +21,19 @@
#include <unistd.h>
#include "base/logging.h"
#include "client/annotation.h"
#include "client/annotation_list.h"
#include "client/crash_report_database.h"
#include "client/simulate_crash.h"
#include "gtest/gtest.h"
#include "test/multiprocess_exec.h"
#include "snapshot/annotation_snapshot.h"
#include "snapshot/minidump/process_snapshot_minidump.h"
#include "test/multiprocess.h"
#include "test/multiprocess_exec.h"
#include "test/scoped_temp_dir.h"
#include "test/test_paths.h"
#include "util/file/file_io.h"
#include "util/file/filesystem.h"
#include "util/linux/exception_handler_client.h"
#include "util/linux/exception_information.h"
#include "util/misc/address_types.h"
@ -94,6 +99,32 @@ TEST(CrashpadClient, SimulateCrash) {
}
}
constexpr char kTestAnnotationName[] = "name_of_annotation";
constexpr char kTestAnnotationValue[] = "value_of_annotation";
void ValidateDump(const CrashReportDatabase::UploadReport* report) {
ProcessSnapshotMinidump minidump_snapshot;
ASSERT_TRUE(minidump_snapshot.Initialize(report->Reader()));
for (const ModuleSnapshot* module : minidump_snapshot.Modules()) {
for (const AnnotationSnapshot& annotation : module->AnnotationObjects()) {
if (static_cast<Annotation::Type>(annotation.type) !=
Annotation::Type::kString) {
continue;
}
if (annotation.name == kTestAnnotationName) {
std::string value(
reinterpret_cast<const char*>(annotation.value.data()),
annotation.value.size());
EXPECT_EQ(value, kTestAnnotationValue);
return;
}
}
}
ADD_FAILURE();
}
CRASHPAD_CHILD_TEST_MAIN(StartHandlerAtCrashChild) {
FileHandle in = StdioFileHandle(StdioStream::kStandardInput);
@ -106,6 +137,11 @@ CRASHPAD_CHILD_TEST_MAIN(StartHandlerAtCrashChild) {
base::FilePath handler_path = TestPaths::Executable().DirName().Append(
FILE_PATH_LITERAL("crashpad_handler"));
crashpad::AnnotationList::Register();
static StringAnnotation<32> test_annotation(kTestAnnotationName);
test_annotation.Set(kTestAnnotationValue);
crashpad::CrashpadClient client;
if (!client.StartHandlerAtCrash(handler_path,
base::FilePath(temp_dir),
@ -145,14 +181,19 @@ class StartHandlerAtCrashTest : public MultiprocessExec {
ASSERT_TRUE(database);
std::vector<CrashReportDatabase::Report> reports;
ASSERT_EQ(database->GetCompletedReports(&reports),
CrashReportDatabase::kNoError);
EXPECT_EQ(reports.size(), 0u);
reports.clear();
ASSERT_EQ(database->GetPendingReports(&reports),
CrashReportDatabase::kNoError);
EXPECT_EQ(reports.size(), 1u);
reports.clear();
ASSERT_EQ(database->GetCompletedReports(&reports),
std::unique_ptr<const CrashReportDatabase::UploadReport> report;
ASSERT_EQ(database->GetReportForUploading(reports[0].uuid, &report),
CrashReportDatabase::kNoError);
EXPECT_EQ(reports.size(), 0u);
ValidateDump(report.get());
}
DISALLOW_COPY_AND_ASSIGN(StartHandlerAtCrashTest);

View File

@ -27,10 +27,12 @@ namespace internal {
ModuleSnapshotElf::ModuleSnapshotElf(const std::string& name,
ElfImageReader* elf_reader,
ModuleSnapshot::ModuleType type)
ModuleSnapshot::ModuleType type,
ProcessMemoryRange* process_memory_range)
: ModuleSnapshot(),
name_(name),
elf_reader_(elf_reader),
process_memory_range_(process_memory_range),
crashpad_info_(),
type_(type),
initialized_() {}
@ -170,7 +172,7 @@ std::map<std::string, std::string> ModuleSnapshotElf::AnnotationsSimpleMap()
INITIALIZATION_STATE_DCHECK_VALID(initialized_);
std::map<std::string, std::string> annotations;
if (crashpad_info_ && crashpad_info_->SimpleAnnotations()) {
ImageAnnotationReader reader(elf_reader_->Memory());
ImageAnnotationReader reader(process_memory_range_);
reader.SimpleMap(crashpad_info_->SimpleAnnotations(), &annotations);
}
return annotations;
@ -180,7 +182,7 @@ std::vector<AnnotationSnapshot> ModuleSnapshotElf::AnnotationObjects() const {
INITIALIZATION_STATE_DCHECK_VALID(initialized_);
std::vector<AnnotationSnapshot> annotations;
if (crashpad_info_ && crashpad_info_->AnnotationsList()) {
ImageAnnotationReader reader(elf_reader_->Memory());
ImageAnnotationReader reader(process_memory_range_);
reader.AnnotationsList(crashpad_info_->AnnotationsList(), &annotations);
}
return annotations;

View File

@ -41,9 +41,11 @@ class ModuleSnapshotElf final : public ModuleSnapshot {
//! \param[in] name The pathname used to load the module from disk.
//! \param[in] elf_reader An image reader for the module.
//! \param[in] type The module's type.
//! \param[in] process_memory_range A memory reader for the target process.
ModuleSnapshotElf(const std::string& name,
ElfImageReader* elf_reader,
ModuleSnapshot::ModuleType type);
ModuleSnapshot::ModuleType type,
ProcessMemoryRange* process_memory_range);
~ModuleSnapshotElf() override;
//! \brief Initializes the object.
@ -84,6 +86,7 @@ class ModuleSnapshotElf final : public ModuleSnapshot {
private:
std::string name_;
ElfImageReader* elf_reader_;
ProcessMemoryRange* process_memory_range_;
std::unique_ptr<CrashpadInfoReader> crashpad_info_;
ModuleType type_;
InitializationStateDcheck initialized_;

View File

@ -33,7 +33,8 @@ bool ProcessSnapshotFuchsia::Initialize(zx_handle_t process) {
return false;
}
if (!process_reader_.Initialize(process)) {
if (!process_reader_.Initialize(process) ||
!memory_range_.Initialize(process_reader_->Memory(), true)) {
return false;
}
@ -204,8 +205,11 @@ void ProcessSnapshotFuchsia::InitializeThreads() {
void ProcessSnapshotFuchsia::InitializeModules() {
for (const ProcessReaderFuchsia::Module& reader_module :
process_reader_.Modules()) {
auto module = std::make_unique<internal::ModuleSnapshotElf>(
reader_module.name, reader_module.reader, reader_module.type);
auto module =
std::make_unique<internal::ModuleSnapshotElf>(reader_module.name,
reader_module.reader,
reader_module.type,
&memory_range_);
if (module->Initialize()) {
modules_.push_back(std::move(module));
}

View File

@ -33,6 +33,7 @@
#include "snapshot/process_snapshot.h"
#include "snapshot/unloaded_module_snapshot.h"
#include "util/misc/initialization_state_dcheck.h"
#include "util/process/process_memory_range.h"
namespace crashpad {
@ -132,6 +133,7 @@ class ProcessSnapshotFuchsia : public ProcessSnapshot {
std::vector<std::unique_ptr<internal::ModuleSnapshotElf>> modules_;
std::unique_ptr<internal::ExceptionSnapshotFuchsia> exception_;
ProcessReaderFuchsia process_reader_;
ProcessMemoryRange memory_range_;
std::map<std::string, std::string> annotations_simple_map_;
UUID report_id_;
UUID client_id_;

View File

@ -21,19 +21,9 @@
namespace crashpad {
ProcessSnapshotLinux::ProcessSnapshotLinux()
: ProcessSnapshot(),
annotations_simple_map_(),
snapshot_time_(),
report_id_(),
client_id_(),
threads_(),
exception_(),
system_(),
process_reader_(),
initialized_() {}
ProcessSnapshotLinux::ProcessSnapshotLinux() = default;
ProcessSnapshotLinux::~ProcessSnapshotLinux() {}
ProcessSnapshotLinux::~ProcessSnapshotLinux() = default;
bool ProcessSnapshotLinux::Initialize(PtraceConnection* connection) {
INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
@ -43,11 +33,14 @@ bool ProcessSnapshotLinux::Initialize(PtraceConnection* connection) {
return false;
}
if (!process_reader_.Initialize(connection)) {
if (!process_reader_.Initialize(connection) ||
!memory_range_.Initialize(process_reader_.Memory(),
process_reader_.Is64Bit())) {
return false;
}
system_.Initialize(&process_reader_, &snapshot_time_);
InitializeThreads();
InitializeModules();
@ -227,8 +220,11 @@ void ProcessSnapshotLinux::InitializeThreads() {
void ProcessSnapshotLinux::InitializeModules() {
for (const ProcessReaderLinux::Module& reader_module :
process_reader_.Modules()) {
auto module = std::make_unique<internal::ModuleSnapshotElf>(
reader_module.name, reader_module.elf_reader, reader_module.type);
auto module =
std::make_unique<internal::ModuleSnapshotElf>(reader_module.name,
reader_module.elf_reader,
reader_module.type,
&memory_range_);
if (module->Initialize()) {
modules_.push_back(std::move(module));
}

View File

@ -39,6 +39,7 @@
#include "util/linux/ptrace_connection.h"
#include "util/misc/initialization_state_dcheck.h"
#include "util/misc/uuid.h"
#include "util/process/process_memory_range.h"
namespace crashpad {
@ -128,6 +129,7 @@ class ProcessSnapshotLinux final : public ProcessSnapshot {
std::unique_ptr<internal::ExceptionSnapshotLinux> exception_;
internal::SystemSnapshotLinux system_;
ProcessReaderLinux process_reader_;
ProcessMemoryRange memory_range_;
InitializationStateDcheck initialized_;
DISALLOW_COPY_AND_ASSIGN(ProcessSnapshotLinux);