From 3af81d7012e492e89a807f23701fb00376e66d6a Mon Sep 17 00:00:00 2001 From: Ryan Tseng Date: Mon, 23 Jul 2018 11:38:06 -0700 Subject: [PATCH 01/11] DEPS: Use gclient's native cipd instead of runhooks Bug: 865729 Change-Id: Ie5cbd3edb394b82b86509576fb421cf2f1807675 Reviewed-on: https://chromium-review.googlesource.com/1145830 Commit-Queue: Ryan Tseng Reviewed-by: Mark Mentovai --- DEPS | 205 ++++++++++++++++++++++++----------------------------------- 1 file changed, 84 insertions(+), 121 deletions(-) diff --git a/DEPS b/DEPS index dbdc3a49..34f1ef68 100644 --- a/DEPS +++ b/DEPS @@ -37,6 +37,90 @@ deps = { 'crashpad/third_party/zlib/zlib': Var('chromium_git') + '/chromium/src/third_party/zlib@' + '13dc246a58e4b72104d35f9b1809af95221ebda7', + + # CIPD packages below. + 'crashpad/third_party/linux/clang/linux-amd64': { + 'packages': [ + { + 'package': 'fuchsia/clang/linux-amd64', + 'version': 'goma', + }, + ], + 'condition': 'checkout_linux and pull_linux_clang', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/linux/clang/mac-amd64': { + 'packages': [ + { + 'package': 'fuchsia/clang/mac-amd64', + 'version': 'goma', + }, + ], + 'condition': 'checkout_fuchsia and host_os == "mac"', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/fuchsia/clang/linux-amd64': { + 'packages': [ + { + 'package': 'fuchsia/clang/linux-amd64', + 'version': 'goma', + }, + ], + 'condition': 'checkout_fuchsia and host_os == "linux"', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/fuchsia/qemu/mac-amd64': { + 'packages': [ + { + 'package': 'fuchsia/qemu/mac-amd64', + 'version': 'latest' + }, + ], + 'condition': 'checkout_fuchsia and host_os == "mac"', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/fuchsia/qemu/linux-amd64': { + 'packages': [ + { + 'package': 'fuchsia/qemu/linux-amd64', + 'version': 'latest' + }, + ], + 'condition': 'checkout_fuchsia and host_os == "linux"', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/fuchsia/sdk/linux-amd64': { + # The SDK is keyed to the host system because it contains build tools. + # Currently, linux-amd64 is the only SDK published (see + # https://chrome-infra-packages.appspot.com/#/?path=fuchsia/sdk). + # As long as this is the case, use that SDK package + # even on other build hosts. + # The sysroot (containing headers and libraries) and other components are + # related to the target and should be functional with an appropriate + # toolchain that runs on the build host (fuchsia_clang, above). + 'packages': [ + { + 'package': 'fuchsia/sdk/linux-amd64', + 'version': 'latest' + }, + ], + 'condition': 'checkout_fuchsia', + 'dep_type': 'cipd' + }, + 'crashpad/third_party/win/toolchain': { + # This package is only updated when the solution in .gclient includes an + # entry like: + # "custom_vars": { "pull_win_toolchain": True } + # This is because the contained bits are not redistributable. + 'packages': [ + { + 'package': 'chrome_internal/third_party/sdk/windows', + 'version': 'uploaded:2018-06-13' + }, + ], + 'condition': 'checkout_win and pull_win_toolchain', + 'dep_type': 'cipd' + }, } hooks = [ @@ -118,28 +202,6 @@ hooks = [ 'buildtools/win/gn.exe.sha1', ], }, - { - # This uses “cipd install” so that mac-amd64 and linux-amd64 can coexist - # peacefully. “cipd ensure” would remove the macOS package when running on a - # Linux build host and vice-versa. https://crbug.com/789364. This package is - # only updated when the solution in .gclient includes an entry like: - # "custom_vars": { "pull_linux_clang": True } - # The ref used is "goma". This is like "latest", but is considered a more - # stable latest by the Fuchsia toolchain team. - 'name': 'clang_linux', - 'pattern': '.', - 'condition': 'checkout_linux and pull_linux_clang', - 'action': [ - 'cipd', - 'install', - # sic, using Fuchsia team's generic build of clang for linux-amd64 to - # build for linux-amd64 target too. - 'fuchsia/clang/linux-amd64', - 'goma', - '-root', 'crashpad/third_party/linux/clang/linux-amd64', - '-log-level', 'info', - ], - }, { # If using a local clang ("pull_linux_clang" above), also pull down a # sysroot. @@ -150,105 +212,6 @@ hooks = [ 'crashpad/build/install_linux_sysroot.py', ], }, - { - # Same rationale for using "install" rather than "ensure" as for first clang - # package. https://crbug.com/789364. - # Same rationale for using "goma" instead of "latest" as clang_linux above. - 'name': 'fuchsia_clang_mac', - 'pattern': '.', - 'condition': 'checkout_fuchsia and host_os == "mac"', - 'action': [ - 'cipd', - 'install', - 'fuchsia/clang/mac-amd64', - 'goma', - '-root', 'crashpad/third_party/fuchsia/clang/mac-amd64', - '-log-level', 'info', - ], - }, - { - # Same rationale for using "install" rather than "ensure" as for first clang - # package. https://crbug.com/789364. - # Same rationale for using "goma" instead of "latest" as clang_linux above. - 'name': 'fuchsia_clang_linux', - 'pattern': '.', - 'condition': 'checkout_fuchsia and host_os == "linux"', - 'action': [ - 'cipd', - 'install', - 'fuchsia/clang/linux-amd64', - 'goma', - '-root', 'crashpad/third_party/fuchsia/clang/linux-amd64', - '-log-level', 'info', - ], - }, - { - # Same rationale for using "install" rather than "ensure" as for clang - # packages. https://crbug.com/789364. - 'name': 'fuchsia_qemu_mac', - 'pattern': '.', - 'condition': 'checkout_fuchsia and host_os == "mac"', - 'action': [ - 'cipd', - 'install', - 'fuchsia/qemu/mac-amd64', - 'latest', - '-root', 'crashpad/third_party/fuchsia/qemu/mac-amd64', - '-log-level', 'info', - ], - }, - { - # Same rationale for using "install" rather than "ensure" as for clang - # packages. https://crbug.com/789364. - 'name': 'fuchsia_qemu_linux', - 'pattern': '.', - 'condition': 'checkout_fuchsia and host_os == "linux"', - 'action': [ - 'cipd', - 'install', - 'fuchsia/qemu/linux-amd64', - 'latest', - '-root', 'crashpad/third_party/fuchsia/qemu/linux-amd64', - '-log-level', 'info', - ], - }, - { - # The SDK is keyed to the host system because it contains build tools. - # Currently, linux-amd64 is the only SDK published (see - # https://chrome-infra-packages.appspot.com/#/?path=fuchsia/sdk). As long as - # this is the case, use that SDK package even on other build hosts. The - # sysroot (containing headers and libraries) and other components are - # related to the target and should be functional with an appropriate - # toolchain that runs on the build host (fuchsia_clang, above). - 'name': 'fuchsia_sdk', - 'pattern': '.', - 'condition': 'checkout_fuchsia', - 'action': [ - 'cipd', - 'install', - 'fuchsia/sdk/linux-amd64', - 'latest', - '-root', 'crashpad/third_party/fuchsia/sdk/linux-amd64', - '-log-level', 'info', - ], - }, - { - 'name': 'toolchain_win', - 'pattern': '.', - # This package is only updated when the solution in .gclient includes an - # entry like: - # "custom_vars": { "pull_win_toolchain": True } - # This is because the contained bits are not redistributable. - 'condition': 'checkout_win and pull_win_toolchain', - 'action': [ - 'cipd', - 'install', - 'chrome_internal/third_party/sdk/windows', - 'uploaded:2018-06-13', - '-root', 'crashpad/third_party/win/toolchain', - '-log-level', 'info', - ], - }, { 'name': 'gyp', 'pattern': '\.gypi?$', From c11c8833f78ff14b18ed5c8daa12e2a43059bf93 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 24 Jul 2018 09:21:51 -0700 Subject: [PATCH 02/11] Add ProcessSnapshotMinidump::ProcessID() Bug: crashpad:30, crashpad:10 Change-Id: I7013debfc9b68ef218c48f859ffdcf7051ea43d9 Reviewed-on: https://chromium-review.googlesource.com/1148540 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- .../minidump/process_snapshot_minidump.cc | 52 ++++++++++++++++--- snapshot/minidump/process_snapshot_minidump.h | 5 ++ .../process_snapshot_minidump_test.cc | 32 ++++++++++++ 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index 90bfac54..652e0e4c 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -31,6 +31,7 @@ ProcessSnapshotMinidump::ProcessSnapshotMinidump() crashpad_info_(), annotations_simple_map_(), file_reader_(nullptr), + process_id_(static_cast(-1)), initialized_() { } @@ -83,19 +84,19 @@ bool ProcessSnapshotMinidump::Initialize(FileReaderInterface* file_reader) { stream_map_[stream_type] = &directory.Location; } - INITIALIZATION_STATE_SET_VALID(initialized_); - - if (!InitializeCrashpadInfo()) { + if (!InitializeCrashpadInfo() || + !InitializeMiscInfo() || + !InitializeModules()) { return false; } - return InitializeModules(); + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; } pid_t ProcessSnapshotMinidump::ProcessID() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://crashpad.chromium.org/bug/10 - return 0; + return process_id_; } pid_t ProcessSnapshotMinidump::ParentProcessID() const { @@ -234,6 +235,45 @@ bool ProcessSnapshotMinidump::InitializeCrashpadInfo() { &annotations_simple_map_); } +bool ProcessSnapshotMinidump::InitializeMiscInfo() { + const auto& stream_it = stream_map_.find(kMinidumpStreamTypeMiscInfo); + if (stream_it == stream_map_.end()) { + return true; + } + + if (!file_reader_->SeekSet(stream_it->second->Rva)) { + return false; + } + + const size_t size = stream_it->second->DataSize; + if (size != sizeof(MINIDUMP_MISC_INFO_5) && + size != sizeof(MINIDUMP_MISC_INFO_4) && + size != sizeof(MINIDUMP_MISC_INFO_3) && + size != sizeof(MINIDUMP_MISC_INFO_2) && + size != sizeof(MINIDUMP_MISC_INFO)) { + LOG(ERROR) << "misc_info size mismatch"; + return false; + } + + MINIDUMP_MISC_INFO_5 info; + if (!file_reader_->ReadExactly(&info, size)) { + return false; + } + + switch (stream_it->second->DataSize) { + case sizeof(MINIDUMP_MISC_INFO_5): + case sizeof(MINIDUMP_MISC_INFO_4): + case sizeof(MINIDUMP_MISC_INFO_3): + case sizeof(MINIDUMP_MISC_INFO_2): + case sizeof(MINIDUMP_MISC_INFO): + // TODO(jperaza): Save the remaining misc info. + // https://crashpad.chromium.org/bug/10 + process_id_ = info.ProcessId; + } + + return true; +} + bool ProcessSnapshotMinidump::InitializeModules() { const auto& stream_it = stream_map_.find(kMinidumpStreamTypeModuleList); if (stream_it == stream_map_.end()) { diff --git a/snapshot/minidump/process_snapshot_minidump.h b/snapshot/minidump/process_snapshot_minidump.h index 211805e8..9ba5d9c6 100644 --- a/snapshot/minidump/process_snapshot_minidump.h +++ b/snapshot/minidump/process_snapshot_minidump.h @@ -92,6 +92,10 @@ class ProcessSnapshotMinidump final : public ProcessSnapshot { std::map* module_crashpad_info_links); + // Initializes data carried in a MINIDUMP_MISC_INFO structure on behalf of + // Initialize(). + bool InitializeMiscInfo(); + MINIDUMP_HEADER header_; std::vector stream_directory_; std::map stream_map_; @@ -100,6 +104,7 @@ class ProcessSnapshotMinidump final : public ProcessSnapshot { MinidumpCrashpadInfo crashpad_info_; std::map annotations_simple_map_; FileReaderInterface* file_reader_; // weak + pid_t process_id_; InitializationStateDcheck initialized_; DISALLOW_COPY_AND_ASSIGN(ProcessSnapshotMinidump); diff --git a/snapshot/minidump/process_snapshot_minidump_test.cc b/snapshot/minidump/process_snapshot_minidump_test.cc index 82e14a7a..ed179394 100644 --- a/snapshot/minidump/process_snapshot_minidump_test.cc +++ b/snapshot/minidump/process_snapshot_minidump_test.cc @@ -420,6 +420,38 @@ TEST(ProcessSnapshotMinidump, Modules) { EXPECT_EQ(annotation_objects, annotations_4); } +TEST(ProcessSnapshotMinidump, ProcessID) { + StringFile string_file; + + MINIDUMP_HEADER header = {}; + ASSERT_TRUE(string_file.Write(&header, sizeof(header))); + + static const pid_t kTestProcessId = 42; + MINIDUMP_MISC_INFO misc_info = {}; + misc_info.SizeOfInfo = sizeof(misc_info); + misc_info.Flags1 = MINIDUMP_MISC1_PROCESS_ID; + misc_info.ProcessId = kTestProcessId; + + MINIDUMP_DIRECTORY misc_directory = {}; + misc_directory.StreamType = kMinidumpStreamTypeMiscInfo; + misc_directory.Location.DataSize = sizeof(misc_info); + misc_directory.Location.Rva = static_cast(string_file.SeekGet()); + ASSERT_TRUE(string_file.Write(&misc_info, sizeof(misc_info))); + + header.StreamDirectoryRva = static_cast(string_file.SeekGet()); + ASSERT_TRUE(string_file.Write(&misc_directory, sizeof(misc_directory))); + + header.Signature = MINIDUMP_SIGNATURE; + header.Version = MINIDUMP_VERSION; + header.NumberOfStreams = 1; + ASSERT_TRUE(string_file.SeekSet(0)); + ASSERT_TRUE(string_file.Write(&header, sizeof(header))); + + ProcessSnapshotMinidump process_snapshot; + ASSERT_TRUE(process_snapshot.Initialize(&string_file)); + EXPECT_EQ(process_snapshot.ProcessID(), kTestProcessId); +} + } // namespace } // namespace test } // namespace crashpad From 2418cb8fbef8f2862286bb4abdcd4f8778aef5a8 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 24 Jul 2018 09:13:49 -0700 Subject: [PATCH 03/11] Make upload report metrics optional Bug: crashpad:30 Change-Id: I202e4571ee8dc8006550173c1cf0c735fae29103 Reviewed-on: https://chromium-review.googlesource.com/1148580 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- client/crash_report_database.cc | 3 ++- client/crash_report_database.h | 8 +++++++- client/crash_report_database_generic.cc | 11 ++++++++--- client/crash_report_database_mac.mm | 11 ++++++++--- client/crash_report_database_win.cc | 11 ++++++++--- client/prune_crash_reports_test.cc | 5 +++-- 6 files changed, 36 insertions(+), 13 deletions(-) diff --git a/client/crash_report_database.cc b/client/crash_report_database.cc index aa365beb..d300a8f9 100644 --- a/client/crash_report_database.cc +++ b/client/crash_report_database.cc @@ -69,7 +69,8 @@ CrashReportDatabase::UploadReport::UploadReport() reader_(std::make_unique()), database_(nullptr), attachment_readers_(), - attachment_map_() {} + attachment_map_(), + report_metrics_(false) {} CrashReportDatabase::UploadReport::~UploadReport() { if (database_) { diff --git a/client/crash_report_database.h b/client/crash_report_database.h index 9ceeddc3..d115c74b 100644 --- a/client/crash_report_database.h +++ b/client/crash_report_database.h @@ -178,6 +178,7 @@ class CrashReportDatabase { CrashReportDatabase* database_; std::vector> attachment_readers_; std::map attachment_map_; + bool report_metrics_; DISALLOW_COPY_AND_ASSIGN(UploadReport); }; @@ -326,11 +327,16 @@ class CrashReportDatabase { //! \param[in] uuid The unique identifier for the crash report record. //! \param[out] report A crash report record for the report to be uploaded. //! Only valid if this returns #kNoError. + //! \param[in] report_metrics If `false`, metrics will not be recorded for + //! this upload attempt when RecordUploadComplete() is called or \a report + //! is destroyed. Metadata for the upload attempt will still be recorded + //! in the database. //! //! \return The operation status code. virtual OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) = 0; + std::unique_ptr* report, + bool report_metrics = true) = 0; //! \brief Records a successful upload for a report and updates the last //! upload attempt time as returned by diff --git a/client/crash_report_database_generic.cc b/client/crash_report_database_generic.cc index 6dc8e9e6..8e932374 100644 --- a/client/crash_report_database_generic.cc +++ b/client/crash_report_database_generic.cc @@ -180,7 +180,8 @@ class CrashReportDatabaseGeneric : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -455,7 +456,8 @@ OperationStatus CrashReportDatabaseGeneric::GetCompletedReports( OperationStatus CrashReportDatabaseGeneric::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); auto upload_report = std::make_unique(); @@ -470,6 +472,7 @@ OperationStatus CrashReportDatabaseGeneric::GetReportForUploading( if (!upload_report->Initialize(path, this)) { return kFileSystemError; } + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); return kNoError; @@ -609,7 +612,9 @@ OperationStatus CrashReportDatabaseGeneric::RecordUploadAttempt( const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } time_t now = time(nullptr); report->id = id; diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 79b876d2..3106dc2c 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -141,7 +141,8 @@ class CrashReportDatabaseMac : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -422,7 +423,8 @@ CrashReportDatabaseMac::GetCompletedReports( CrashReportDatabase::OperationStatus CrashReportDatabaseMac::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); auto upload_report = std::make_unique(); @@ -444,6 +446,7 @@ CrashReportDatabaseMac::GetReportForUploading( upload_report->database_ = this; upload_report->lock_fd.reset(lock.release()); + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); return kNoError; } @@ -454,7 +457,9 @@ CrashReportDatabaseMac::RecordUploadAttempt(UploadReport* report, const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } DCHECK(report); DCHECK(successful || id.empty()); diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index e19e6058..89677706 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -594,7 +594,8 @@ class CrashReportDatabaseWin : public CrashReportDatabase { OperationStatus GetCompletedReports(std::vector* reports) override; OperationStatus GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) override; + std::unique_ptr* report, + bool report_metrics) override; OperationStatus SkipReportUpload(const UUID& uuid, Metrics::CrashSkippedReason reason) override; OperationStatus DeleteReport(const UUID& uuid) override; @@ -731,7 +732,8 @@ OperationStatus CrashReportDatabaseWin::GetCompletedReports( OperationStatus CrashReportDatabaseWin::GetReportForUploading( const UUID& uuid, - std::unique_ptr* report) { + std::unique_ptr* report, + bool report_metrics) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); std::unique_ptr metadata(AcquireMetadata()); @@ -749,6 +751,7 @@ OperationStatus CrashReportDatabaseWin::GetReportForUploading( if (!upload_report->Initialize(upload_report->file_path, this)) { return kFileSystemError; } + upload_report->report_metrics_ = report_metrics; report->reset(upload_report.release()); } @@ -761,7 +764,9 @@ OperationStatus CrashReportDatabaseWin::RecordUploadAttempt( const std::string& id) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - Metrics::CrashUploadAttempted(successful); + if (report->report_metrics_) { + Metrics::CrashUploadAttempted(successful); + } std::unique_ptr metadata(AcquireMetadata()); if (!metadata) diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 2648dee2..01990d27 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -41,9 +41,10 @@ class MockDatabase : public CrashReportDatabase { MOCK_METHOD2(LookUpCrashReport, OperationStatus(const UUID&, Report*)); MOCK_METHOD1(GetPendingReports, OperationStatus(std::vector*)); MOCK_METHOD1(GetCompletedReports, OperationStatus(std::vector*)); - MOCK_METHOD2(GetReportForUploading, + MOCK_METHOD3(GetReportForUploading, OperationStatus(const UUID&, - std::unique_ptr*)); + std::unique_ptr*, + bool report_metrics)); MOCK_METHOD3(RecordUploadAttempt, OperationStatus(UploadReport*, bool, const std::string&)); MOCK_METHOD2(SkipReportUpload, From 0909bee2e26c59d4532c08dfb5738615ace92d90 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 24 Jul 2018 12:47:14 -0700 Subject: [PATCH 04/11] linux: Fix broken tests with address sanitizer These fixes are mostly related to address sanitizer causing stack variables to not be stored on the call-stack. Attempting to disable safe-stack has no effect. Change-Id: Ib5718bfb74ce91dee560b397ccdbf68d78e4ec6a Reviewed-on: https://chromium-review.googlesource.com/1140507 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- client/simple_string_dictionary_test.cc | 23 +++++++++---------- snapshot/linux/process_reader_linux_test.cc | 10 ++++++-- .../process_snapshot_sanitized_test.cc | 19 +++++++++++++++ util/misc/capture_context_test.cc | 14 +++++++---- 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/client/simple_string_dictionary_test.cc b/client/simple_string_dictionary_test.cc index 5fdeb5bf..7af51a27 100644 --- a/client/simple_string_dictionary_test.cc +++ b/client/simple_string_dictionary_test.cc @@ -115,8 +115,7 @@ TEST(SimpleStringDictionary, CopyAndAssign) { // Add a bunch of values to the dictionary, remove some entries in the middle, // and then add more. TEST(SimpleStringDictionary, Iterator) { - SimpleStringDictionary* dict = new SimpleStringDictionary; - ASSERT_TRUE(dict); + SimpleStringDictionary dict; char key[SimpleStringDictionary::key_size]; char value[SimpleStringDictionary::value_size]; @@ -135,32 +134,32 @@ TEST(SimpleStringDictionary, Iterator) { for (int i = 0; i < kPartitionIndex; ++i) { sprintf(key, "key%d", i); sprintf(value, "value%d", i); - dict->SetKeyValue(key, value); + dict.SetKeyValue(key, value); } expected_dictionary_size = kPartitionIndex; // set a couple of the keys twice (with the same value) - should be nop - dict->SetKeyValue("key2", "value2"); - dict->SetKeyValue("key4", "value4"); - dict->SetKeyValue("key15", "value15"); + dict.SetKeyValue("key2", "value2"); + dict.SetKeyValue("key4", "value4"); + dict.SetKeyValue("key15", "value15"); // Remove some random elements in the middle - dict->RemoveKey("key7"); - dict->RemoveKey("key18"); - dict->RemoveKey("key23"); - dict->RemoveKey("key31"); + dict.RemoveKey("key7"); + dict.RemoveKey("key18"); + dict.RemoveKey("key23"); + dict.RemoveKey("key31"); expected_dictionary_size -= 4; // we just removed four key/value pairs // Set some more key/value pairs like key59/value59, key60/value60, ... for (int i = kPartitionIndex; i < kDictionaryCapacity; ++i) { sprintf(key, "key%d", i); sprintf(value, "value%d", i); - dict->SetKeyValue(key, value); + dict.SetKeyValue(key, value); } expected_dictionary_size += kDictionaryCapacity - kPartitionIndex; // Now create an iterator on the dictionary - SimpleStringDictionary::Iterator iter(*dict); + SimpleStringDictionary::Iterator iter(dict); // We then verify that it iterates through exactly the number of key/value // pairs we expect, and that they match one-for-one with what we would expect. diff --git a/snapshot/linux/process_reader_linux_test.cc b/snapshot/linux/process_reader_linux_test.cc index 6c0b98af..940114c6 100644 --- a/snapshot/linux/process_reader_linux_test.cc +++ b/snapshot/linux/process_reader_linux_test.cc @@ -40,6 +40,7 @@ #include "test/multiprocess.h" #include "util/file/file_io.h" #include "util/linux/direct_ptrace_connection.h" +#include "util/misc/address_sanitizer.h" #include "util/misc/from_pointer_cast.h" #include "util/synchronization/semaphore.h" @@ -264,12 +265,17 @@ void ExpectThreads(const ThreadMap& thread_map, iterator->second.tls); ASSERT_TRUE(memory_map.FindMapping(thread.stack_region_address)); - EXPECT_LE(thread.stack_region_address, iterator->second.stack_address); - ASSERT_TRUE(memory_map.FindMapping(thread.stack_region_address + thread.stack_region_size - 1)); + +#if !defined(ADDRESS_SANITIZER) + // AddressSanitizer causes stack variables to be stored separately from the + // call stack. + EXPECT_LE(thread.stack_region_address, iterator->second.stack_address); EXPECT_GE(thread.stack_region_address + thread.stack_region_size, iterator->second.stack_address); +#endif // !defined(ADDRESS_SANITIZER) + if (iterator->second.max_stack_size) { EXPECT_LT(thread.stack_region_size, iterator->second.max_stack_size); } diff --git a/snapshot/sanitized/process_snapshot_sanitized_test.cc b/snapshot/sanitized/process_snapshot_sanitized_test.cc index 485ef87f..cb413e3b 100644 --- a/snapshot/sanitized/process_snapshot_sanitized_test.cc +++ b/snapshot/sanitized/process_snapshot_sanitized_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "test/multiprocess_exec.h" #include "util/file/file_io.h" +#include "util/misc/address_sanitizer.h" #include "util/numeric/safe_assignment.h" #if defined(OS_LINUX) || defined(OS_ANDROID) @@ -162,6 +163,23 @@ class StackSanitizationChecker : public MemorySnapshot::Delegate { // MemorySnapshot::Delegate bool MemorySnapshotDelegateRead(void* data, size_t size) override { +#if defined(ADDRESS_SANITIZER) && (defined(OS_LINUX) || defined(OS_ANDROID)) + // AddressSanitizer causes stack variables to be stored separately from the + // call stack. + auto addr_not_in_stack_range = + [](VMAddress addr, VMAddress stack_addr, VMSize stack_size) { + return addr < stack_addr || addr >= stack_addr + stack_size; + }; + EXPECT_PRED3(addr_not_in_stack_range, + addrs_.code_pointer_address, + stack_->Address(), + size); + EXPECT_PRED3(addr_not_in_stack_range, + addrs_.string_address, + stack_->Address(), + size); + return true; +#else size_t pointer_offset; if (!AssignIfInRange(&pointer_offset, addrs_.code_pointer_address - stack_->Address())) { @@ -192,6 +210,7 @@ class StackSanitizationChecker : public MemorySnapshot::Delegate { EXPECT_STREQ(string, kSensitiveStackData); } return true; +#endif // ADDRESS_SANITIZER && (OS_LINUX || OS_ANDROID) } private: diff --git a/util/misc/capture_context_test.cc b/util/misc/capture_context_test.cc index 1117789d..7f3890f1 100644 --- a/util/misc/capture_context_test.cc +++ b/util/misc/capture_context_test.cc @@ -60,12 +60,16 @@ void TestCaptureContext() { kReferencePC); #endif // !defined(ADDRESS_SANITIZER) - // Declare sp and context_2 here because all local variables need to be - // declared before computing the stack pointer reference value, so that the - // reference value can be the lowest value possible. - uintptr_t sp; + const uintptr_t sp = StackPointerFromContext(context_1); + + // Declare context_2 here because all local variables need to be declared + // before computing the stack pointer reference value, so that the reference + // value can be the lowest value possible. NativeCPUContext context_2; +// AddressSanitizer on Linux causes stack variables to be stored separately from +// the call stack. +#if !defined(ADDRESS_SANITIZER) || (!defined(OS_LINUX) && !defined(OS_ANDROID)) // The stack pointer reference value is the lowest address of a local variable // in this function. The captured program counter will be slightly less than // or equal to the reference stack pointer. @@ -74,11 +78,11 @@ void TestCaptureContext() { reinterpret_cast(&context_2)), std::min(reinterpret_cast(&pc), reinterpret_cast(&sp))); - sp = StackPointerFromContext(context_1); EXPECT_PRED2([](uintptr_t actual, uintptr_t reference) { return reference - actual < 768u; }, sp, kReferenceSP); +#endif // !ADDRESS_SANITIZER || (!OS_LINUX && !OS_ANDROID) // Capture the context again, expecting that the stack pointer stays the same // and the program counter increases. Strictly speaking, there’s no guarantee From 2f3a8b8f72dccc7c1738eb496e46e86ee60ec4ed Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 24 Jul 2018 13:09:38 -0700 Subject: [PATCH 05/11] Add CrashSkippedReason::kPrepareForUploadFailed Bug: crashpad:30 Change-Id: I763c30e261c315b45860c8672d9cffbba4714f32 Reviewed-on: https://chromium-review.googlesource.com/1148895 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- handler/crash_report_upload_thread.cc | 4 ++++ util/misc/metrics.h | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 4783ecb2..205d860f 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -227,6 +227,10 @@ void CrashReportUploadThread::ProcessPendingReport( database_->RecordUploadComplete(std::move(upload_report), response_body); break; case UploadResult::kPermanentFailure: + upload_report.reset(); + database_->SkipReportUpload( + report.uuid, Metrics::CrashSkippedReason::kPrepareForUploadFailed); + break; case UploadResult::kRetry: upload_report.reset(); diff --git a/util/misc/metrics.h b/util/misc/metrics.h index 3e9337a9..8046497e 100644 --- a/util/misc/metrics.h +++ b/util/misc/metrics.h @@ -77,6 +77,10 @@ class Metrics { //! server. kUploadFailed = 4, + //! \brief There was an error between accessing the report from the database + //! and uploading it to the crash server. + kPrepareForUploadFailed = 5, + //! \brief The number of values in this enumeration; not a valid value. kMaxValue }; From 93b1271e1b8ddca0c7b052b0248046e37db2ab70 Mon Sep 17 00:00:00 2001 From: Ryan Tseng Date: Tue, 24 Jul 2018 16:32:52 -0700 Subject: [PATCH 06/11] package.h: Update PACKAGE_COPYRIGHT_YEAR to 2018 Bug: crashpad: Change-Id: I64d76604c683b53524b496a5929382c490fe2dc9 Reviewed-on: https://chromium-review.googlesource.com/1149157 Reviewed-by: Mark Mentovai Commit-Queue: Ryan Tseng --- package.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.h b/package.h index ddf98c14..e170dec9 100644 --- a/package.h +++ b/package.h @@ -19,7 +19,7 @@ #define PACKAGE_COPYRIGHT \ "Copyright " PACKAGE_COPYRIGHT_YEAR " " PACKAGE_COPYRIGHT_OWNER #define PACKAGE_COPYRIGHT_OWNER "The Crashpad Authors" -#define PACKAGE_COPYRIGHT_YEAR "2017" +#define PACKAGE_COPYRIGHT_YEAR "2018" #define PACKAGE_NAME "Crashpad" #define PACKAGE_STRING PACKAGE_NAME " " PACKAGE_VERSION #define PACKAGE_TARNAME "crashpad" From 20294e79cc0926b8e3c5413d01d4d1c514874e91 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Wed, 25 Jul 2018 16:31:30 -0700 Subject: [PATCH 07/11] android: Rename crashpad_handler_module target Targets suffixed with "_module" are now treated specially in chromium as dynamic feature modules. Bug: crashpad:30 Change-Id: I9682a76a0e0fae993bbe7454c49a44ada6c4165b Reviewed-on: https://chromium-review.googlesource.com/1150851 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- handler/BUILD.gn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/handler/BUILD.gn b/handler/BUILD.gn index 7928461b..2eae30b5 100644 --- a/handler/BUILD.gn +++ b/handler/BUILD.gn @@ -165,7 +165,7 @@ crashpad_executable("crashpad_handler") { # installer will ignore files not named like a shared object, so give the # handler executable an acceptable name. if (crashpad_is_android) { - copy("crashpad_handler_module") { + copy("crashpad_handler_named_as_so") { deps = [ ":crashpad_handler", ] From 52ff1accbbe81ab3631fdb410f0dd4afd1c91c0d Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 26 Jul 2018 08:27:31 -0700 Subject: [PATCH 08/11] linux: Fix locating modules with multiple mappings from offset 0 The general strategy used by Crashpad to determine loaded modules is to read the link_map to get the addresses of the dynamic arrays for all loaded modules. Those addresses can then be used to query the MemoryMap to locate the module's mappings, and in particular the base mapping from which Crashpad can parse the entire loaded ELF file. ELF modules are typically loaded in several mappings with varying permissions for different segments. The previous strategy used to find the base mapping for a module was to search backwards from the mapping for the dynamic array until a mapping from file offset 0 was found for the same file. This fails when the file is mapped multiple times from file offset 0, which can happen if the first page of the file contains a GNU_RELRO segment. This new strategy queries the MemoryMap for ALL mappings associated with the dynamic array's mapping, mapped from offset 0. The consumer (process_reader_linux.cc) can then determine which mapping is the correct base by attempting to parse a module at that address and corroborating the PT_DYNAMIC or program header table address from the parsed module with the values Crashpad gets from the link_map or auxiliary vector. Bug: crashpad:30 Change-Id: Ibfcbba512e8fccc8c65afef734ea5640b71e9f70 Reviewed-on: https://chromium-review.googlesource.com/1139396 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- snapshot/elf/elf_image_reader.cc | 105 +++++---- snapshot/elf/elf_image_reader.h | 20 +- snapshot/elf/elf_image_reader_test.cc | 8 +- snapshot/linux/debug_rendezvous_test.cc | 35 ++- snapshot/linux/process_reader_linux.cc | 84 +++++-- snapshot/linux/process_reader_linux_test.cc | 238 +++++++++++++++++++- test/scoped_module_handle.cc | 5 + test/scoped_module_handle.h | 1 + util/linux/memory_map.cc | 43 ++-- util/linux/memory_map.h | 25 +- util/linux/memory_map_test.cc | 151 +++++++++++-- 11 files changed, 592 insertions(+), 123 deletions(-) diff --git a/snapshot/elf/elf_image_reader.cc b/snapshot/elf/elf_image_reader.cc index c6816605..bbf9b54e 100644 --- a/snapshot/elf/elf_image_reader.cc +++ b/snapshot/elf/elf_image_reader.cc @@ -31,12 +31,14 @@ class ElfImageReader::ProgramHeaderTable { public: virtual ~ProgramHeaderTable() {} - virtual bool VerifyLoadSegments() const = 0; + virtual bool VerifyLoadSegments(bool verbose) const = 0; virtual size_t Size() const = 0; virtual bool GetDynamicSegment(VMAddress* address, VMSize* size) const = 0; - virtual bool GetPreferredElfHeaderAddress(VMAddress* address) const = 0; + virtual bool GetPreferredElfHeaderAddress(VMAddress* address, + bool verbose) const = 0; virtual bool GetPreferredLoadedMemoryRange(VMAddress* address, - VMSize* size) const = 0; + VMSize* size, + bool verbose) const = 0; // Locate the next PT_NOTE segment starting at segment index start_index. If a // PT_NOTE segment is found, start_index is set to the next index after the @@ -58,14 +60,15 @@ class ElfImageReader::ProgramHeaderTableSpecific bool Initialize(const ProcessMemoryRange& memory, VMAddress address, - VMSize num_segments) { + VMSize num_segments, + bool verbose) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); table_.resize(num_segments); if (!memory.Read(address, sizeof(PhdrType) * num_segments, table_.data())) { return false; } - if (!VerifyLoadSegments()) { + if (!VerifyLoadSegments(verbose)) { return false; } @@ -73,7 +76,7 @@ class ElfImageReader::ProgramHeaderTableSpecific return true; } - bool VerifyLoadSegments() const override { + bool VerifyLoadSegments(bool verbose) const override { constexpr bool is_64_bit = std::is_same::value; VMAddress last_vaddr; bool load_found = false; @@ -83,12 +86,12 @@ class ElfImageReader::ProgramHeaderTableSpecific is_64_bit, header.p_vaddr, header.p_memsz); if (!load_range.IsValid()) { - LOG(ERROR) << "bad load range"; + LOG_IF(ERROR, verbose) << "bad load range"; return false; } if (load_found && header.p_vaddr <= last_vaddr) { - LOG(ERROR) << "out of order load segments"; + LOG_IF(ERROR, verbose) << "out of order load segments"; return false; } load_found = true; @@ -100,7 +103,8 @@ class ElfImageReader::ProgramHeaderTableSpecific size_t Size() const override { return sizeof(PhdrType) * table_.size(); } - bool GetPreferredElfHeaderAddress(VMAddress* address) const override { + bool GetPreferredElfHeaderAddress(VMAddress* address, + bool verbose) const override { INITIALIZATION_STATE_DCHECK_VALID(initialized_); for (const auto& header : table_) { if (header.p_type == PT_LOAD && header.p_offset == 0) { @@ -108,12 +112,13 @@ class ElfImageReader::ProgramHeaderTableSpecific return true; } } - LOG(ERROR) << "no preferred header address"; + LOG_IF(ERROR, verbose) << "no preferred header address"; return false; } bool GetPreferredLoadedMemoryRange(VMAddress* base, - VMSize* size) const override { + VMSize* size, + bool verbose) const override { INITIALIZATION_STATE_DCHECK_VALID(initialized_); VMAddress preferred_base = 0; @@ -133,7 +138,7 @@ class ElfImageReader::ProgramHeaderTableSpecific *size = preferred_end - preferred_base; return true; } - LOG(ERROR) << "no load segments"; + LOG_IF(ERROR, verbose) << "no load segments"; return false; } @@ -340,7 +345,8 @@ ElfImageReader::ElfImageReader() ElfImageReader::~ElfImageReader() {} bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, - VMAddress address) { + VMAddress address, + bool verbose) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); ehdr_address_ = address; if (!memory_.Initialize(memory)) { @@ -354,13 +360,13 @@ bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, if (e_ident[EI_MAG0] != ELFMAG0 || e_ident[EI_MAG1] != ELFMAG1 || e_ident[EI_MAG2] != ELFMAG2 || e_ident[EI_MAG3] != ELFMAG3) { - LOG(ERROR) << "Incorrect ELF magic number"; + LOG_IF(ERROR, verbose) << "Incorrect ELF magic number"; return false; } if (!(memory_.Is64Bit() && e_ident[EI_CLASS] == ELFCLASS64) && !(!memory_.Is64Bit() && e_ident[EI_CLASS] == ELFCLASS32)) { - LOG(ERROR) << "unexpected bitness"; + LOG_IF(ERROR, verbose) << "unexpected bitness"; return false; } @@ -370,12 +376,12 @@ bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, constexpr uint8_t expected_encoding = ELFDATA2MSB; #endif if (e_ident[EI_DATA] != expected_encoding) { - LOG(ERROR) << "unexpected encoding"; + LOG_IF(ERROR, verbose) << "unexpected encoding"; return false; } if (e_ident[EI_VERSION] != EV_CURRENT) { - LOG(ERROR) << "unexpected version"; + LOG_IF(ERROR, verbose) << "unexpected version"; return false; } @@ -388,15 +394,15 @@ bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, #define VERIFY_HEADER(header) \ do { \ if (header.e_type != ET_EXEC && header.e_type != ET_DYN) { \ - LOG(ERROR) << "unexpected image type"; \ + LOG_IF(ERROR, verbose) << "unexpected image type"; \ return false; \ } \ if (header.e_version != EV_CURRENT) { \ - LOG(ERROR) << "unexpected version"; \ + LOG_IF(ERROR, verbose) << "unexpected version"; \ return false; \ } \ if (header.e_ehsize != sizeof(header)) { \ - LOG(ERROR) << "unexpected header size"; \ + LOG_IF(ERROR, verbose) << "unexpected header size"; \ return false; \ } \ } while (false); @@ -407,21 +413,21 @@ bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, VERIFY_HEADER(header_32_); } - if (!InitializeProgramHeaders()) { + if (!InitializeProgramHeaders(verbose)) { return false; } VMAddress preferred_ehdr_address; if (!program_headers_.get()->GetPreferredElfHeaderAddress( - &preferred_ehdr_address)) { + &preferred_ehdr_address, verbose)) { return false; } load_bias_ = ehdr_address_ - preferred_ehdr_address; VMAddress base_address; VMSize loaded_size; - if (!program_headers_.get()->GetPreferredLoadedMemoryRange(&base_address, - &loaded_size)) { + if (!program_headers_.get()->GetPreferredLoadedMemoryRange( + &base_address, &loaded_size, verbose)) { return false; } base_address += load_bias_; @@ -443,12 +449,12 @@ bool ElfImageReader::Initialize(const ProcessMemoryRange& memory, CheckedVMAddressRange range(memory_.Is64Bit(), base_address, loaded_size); if (!range.ContainsRange( CheckedVMAddressRange(memory_.Is64Bit(), ehdr_address_, ehdr_size))) { - LOG(ERROR) << "ehdr out of range"; + LOG_IF(ERROR, verbose) << "ehdr out of range"; return false; } if (!range.ContainsRange(CheckedVMAddressRange( memory.Is64Bit(), phdr_address, program_headers_->Size()))) { - LOG(ERROR) << "phdrs out of range"; + LOG_IF(ERROR, verbose) << "phdrs out of range"; return false; } @@ -545,19 +551,40 @@ bool ElfImageReader::GetDebugAddress(VMAddress* debug) { return GetAddressFromDynamicArray(DT_DEBUG, true, debug); } -bool ElfImageReader::InitializeProgramHeaders() { -#define INITIALIZE_PROGRAM_HEADERS(PhdrType, header) \ - do { \ - if (header.e_phentsize != sizeof(PhdrType)) { \ - LOG(ERROR) << "unexpected phdr size"; \ - return false; \ - } \ - auto phdrs = new ProgramHeaderTableSpecific(); \ - program_headers_.reset(phdrs); \ - if (!phdrs->Initialize( \ - memory_, ehdr_address_ + header.e_phoff, header.e_phnum)) { \ - return false; \ - } \ +bool ElfImageReader::GetDynamicArrayAddress(VMAddress* address) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + VMAddress dyn_segment_address; + VMSize dyn_segment_size; + if (!program_headers_.get()->GetDynamicSegment(&dyn_segment_address, + &dyn_segment_size)) { + LOG(ERROR) << "no dynamic segment"; + return false; + } + *address = dyn_segment_address + GetLoadBias(); + return true; +} + +VMAddress ElfImageReader::GetProgramHeaderTableAddress() { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return ehdr_address_ + + (memory_.Is64Bit() ? header_64_.e_phoff : header_32_.e_phoff); +} + +bool ElfImageReader::InitializeProgramHeaders(bool verbose) { +#define INITIALIZE_PROGRAM_HEADERS(PhdrType, header) \ + do { \ + if (header.e_phentsize != sizeof(PhdrType)) { \ + LOG_IF(ERROR, verbose) << "unexpected phdr size"; \ + return false; \ + } \ + auto phdrs = new ProgramHeaderTableSpecific(); \ + program_headers_.reset(phdrs); \ + if (!phdrs->Initialize(memory_, \ + ehdr_address_ + header.e_phoff, \ + header.e_phnum, \ + verbose)) { \ + return false; \ + } \ } while (false); if (memory_.Is64Bit()) { diff --git a/snapshot/elf/elf_image_reader.h b/snapshot/elf/elf_image_reader.h index 7549f346..29f8b1eb 100644 --- a/snapshot/elf/elf_image_reader.h +++ b/snapshot/elf/elf_image_reader.h @@ -118,7 +118,13 @@ class ElfImageReader { //! \param[in] memory A memory reader for the remote process. //! \param[in] address The address in the remote process' address space where //! the ELF image is loaded. - bool Initialize(const ProcessMemoryRange& memory, VMAddress address); + //! \param[in] verbose `true` if this method should log error messages during + //! initialization. Setting this value to `false` will reduce the error + //! messages relating to verifying the ELF image, but may not suppress + //! logging entirely. + bool Initialize(const ProcessMemoryRange& memory, + VMAddress address, + bool verbose = true); //! \brief Returns the base address of the image's memory range. //! @@ -172,6 +178,16 @@ class ElfImageReader { //! \return `true` if the debug address was found. bool GetDebugAddress(VMAddress* debug); + //! \brief Determine the address of `PT_DYNAMIC` segment. + //! + //! \param[out] address The address of the array, valid if this method returns + //! `true`. + //! \return `true` on success. Otherwise `false` with a message logged. + bool GetDynamicArrayAddress(VMAddress* address); + + //! \brief Return the address of the program header table. + VMAddress GetProgramHeaderTableAddress(); + //! \brief Return a NoteReader for this image, which scans all PT_NOTE //! segments in the image. //! @@ -238,7 +254,7 @@ class ElfImageReader { template class ProgramHeaderTableSpecific; - bool InitializeProgramHeaders(); + bool InitializeProgramHeaders(bool verbose); bool InitializeDynamicArray(); bool InitializeDynamicSymbolTable(); bool GetAddressFromDynamicArray(uint64_t tag, bool log, VMAddress* address); diff --git a/snapshot/elf/elf_image_reader_test.cc b/snapshot/elf/elf_image_reader_test.cc index d64fd5c4..11fb1feb 100644 --- a/snapshot/elf/elf_image_reader_test.cc +++ b/snapshot/elf/elf_image_reader_test.cc @@ -103,10 +103,10 @@ void LocateExecutable(PtraceConnection* connection, ASSERT_TRUE(memory_map.Initialize(connection)); const MemoryMap::Mapping* phdr_mapping = memory_map.FindMapping(phdrs); ASSERT_TRUE(phdr_mapping); - const MemoryMap::Mapping* exe_mapping = - memory_map.FindFileMmapStart(*phdr_mapping); - ASSERT_TRUE(exe_mapping); - *elf_address = exe_mapping->range.Base(); + std::vector possible_mappings = + memory_map.FindFilePossibleMmapStarts(*phdr_mapping); + ASSERT_EQ(possible_mappings.size(), 1u); + *elf_address = possible_mappings[0]->range.Base(); } #endif // OS_FUCHSIA diff --git a/snapshot/linux/debug_rendezvous_test.cc b/snapshot/linux/debug_rendezvous_test.cc index f9920ab4..431e2bbe 100644 --- a/snapshot/linux/debug_rendezvous_test.cc +++ b/snapshot/linux/debug_rendezvous_test.cc @@ -74,9 +74,10 @@ void TestAgainstTarget(PtraceConnection* connection) { const MemoryMap::Mapping* phdr_mapping = mappings.FindMapping(phdrs); ASSERT_TRUE(phdr_mapping); - const MemoryMap::Mapping* exe_mapping = - mappings.FindFileMmapStart(*phdr_mapping); - LinuxVMAddress elf_address = exe_mapping->range.Base(); + std::vector exe_mappings = + mappings.FindFilePossibleMmapStarts(*phdr_mapping); + ASSERT_EQ(exe_mappings.size(), 1u); + LinuxVMAddress elf_address = exe_mappings[0]->range.Base(); ProcessMemoryLinux memory; ASSERT_TRUE(memory.Initialize(connection->GetProcessID())); @@ -142,9 +143,24 @@ void TestAgainstTarget(PtraceConnection* connection) { mappings.FindMapping(module.dynamic_array); ASSERT_TRUE(dyn_mapping); - const MemoryMap::Mapping* module_mapping = - mappings.FindFileMmapStart(*dyn_mapping); - ASSERT_TRUE(module_mapping); + std::vector possible_mappings = + mappings.FindFilePossibleMmapStarts(*dyn_mapping); + ASSERT_GE(possible_mappings.size(), 1u); + + std::unique_ptr module_reader; + const MemoryMap::Mapping* module_mapping = nullptr; + for (const auto mapping : possible_mappings) { + auto parsed_module = std::make_unique(); + VMAddress dynamic_address; + if (parsed_module->Initialize(range, mapping->range.Base()) && + parsed_module->GetDynamicArrayAddress(&dynamic_address) && + dynamic_address == module.dynamic_array) { + module_reader = std::move(parsed_module); + module_mapping = mapping; + break; + } + } + ASSERT_TRUE(module_reader.get()); #if defined(OS_ANDROID) EXPECT_FALSE(module.name.empty()); @@ -168,20 +184,17 @@ void TestAgainstTarget(PtraceConnection* connection) { module.name); #endif // OS_ANDROID - ElfImageReader module_reader; - ASSERT_TRUE(module_reader.Initialize(range, module_mapping->range.Base())); - // Android's loader stops setting its own load bias after Android 4.4.4 // (API 20) until Android 6.0 (API 23). if (is_android_loader && android_runtime_api > 20 && android_runtime_api < 23) { EXPECT_EQ(module.load_bias, 0); } else { - EXPECT_EQ(module.load_bias, module_reader.GetLoadBias()); + EXPECT_EQ(module.load_bias, module_reader->GetLoadBias()); } CheckedLinuxAddressRange module_range( - connection->Is64Bit(), module_reader.Address(), module_reader.Size()); + connection->Is64Bit(), module_reader->Address(), module_reader->Size()); EXPECT_TRUE(module_range.ContainsValue(module.dynamic_array)); } } diff --git a/snapshot/linux/process_reader_linux.cc b/snapshot/linux/process_reader_linux.cc index 038d5cef..33d4f92b 100644 --- a/snapshot/linux/process_reader_linux.cc +++ b/snapshot/linux/process_reader_linux.cc @@ -347,20 +347,45 @@ void ProcessReaderLinux::InitializeModules() { return; } - const MemoryMap::Mapping* exe_mapping; - if (!(exe_mapping = GetMemoryMap()->FindMapping(phdrs)) || - !(exe_mapping = GetMemoryMap()->FindFileMmapStart(*exe_mapping))) { - return; - } - ProcessMemoryRange range; if (!range.Initialize(Memory(), is_64_bit_)) { return; } - auto exe_reader = std::make_unique(); - if (!exe_reader->Initialize(range, exe_mapping->range.Base())) { - return; + // The strategy used for identifying loaded modules depends on ELF files + // conventionally loading their header and program headers into memory. + // Locating the correct module could fail if the headers aren't mapped, are + // mapped at an unexpected location, or if there are other mappings + // constructed to look like the ELF module being searched for. + const MemoryMap::Mapping* exe_mapping = nullptr; + std::unique_ptr exe_reader; + { + const MemoryMap::Mapping* phdr_mapping = memory_map_.FindMapping(phdrs); + if (!phdr_mapping) { + return; + } + + std::vector possible_mappings = + memory_map_.FindFilePossibleMmapStarts(*phdr_mapping); + for (auto riter = possible_mappings.rbegin(); + riter != possible_mappings.rend(); + ++riter) { + auto mapping = *riter; + auto parsed_exe = std::make_unique(); + if (parsed_exe->Initialize( + range, + mapping->range.Base(), + /* verbose= */ possible_mappings.size() == 1) && + parsed_exe->GetProgramHeaderTableAddress() == phdrs) { + exe_mapping = mapping; + exe_reader = std::move(parsed_exe); + break; + } + } + if (!exe_mapping) { + LOG(ERROR) << "no exe mappings " << possible_mappings.size(); + return; + } } LinuxVMAddress debug_address; @@ -386,19 +411,42 @@ void ProcessReaderLinux::InitializeModules() { aux.GetValue(AT_BASE, &loader_base); for (const DebugRendezvous::LinkEntry& entry : debug.Modules()) { - const MemoryMap::Mapping* mapping; - if (!(mapping = memory_map_.FindMapping(entry.dynamic_array)) || - !(mapping = memory_map_.FindFileMmapStart(*mapping))) { - continue; - } + const MemoryMap::Mapping* module_mapping = nullptr; + std::unique_ptr elf_reader; + { + const MemoryMap::Mapping* dyn_mapping = + memory_map_.FindMapping(entry.dynamic_array); + if (!dyn_mapping) { + continue; + } - auto elf_reader = std::make_unique(); - if (!elf_reader->Initialize(range, mapping->range.Base())) { - continue; + std::vector possible_mappings = + memory_map_.FindFilePossibleMmapStarts(*dyn_mapping); + for (auto riter = possible_mappings.rbegin(); + riter != possible_mappings.rend(); + ++riter) { + auto mapping = *riter; + auto parsed_module = std::make_unique(); + VMAddress dynamic_address; + if (parsed_module->Initialize( + range, + mapping->range.Base(), + /* verbose= */ possible_mappings.size() == 1) && + parsed_module->GetDynamicArrayAddress(&dynamic_address) && + dynamic_address == entry.dynamic_array) { + module_mapping = mapping; + elf_reader = std::move(parsed_module); + break; + } + } + if (!module_mapping) { + LOG(ERROR) << "no module mappings " << possible_mappings.size(); + continue; + } } Module module = {}; - module.name = !entry.name.empty() ? entry.name : mapping->name; + module.name = !entry.name.empty() ? entry.name : module_mapping->name; module.elf_reader = elf_reader.get(); module.type = loader_base && elf_reader->Address() == loader_base ? ModuleSnapshot::kModuleTypeDynamicLoader diff --git a/snapshot/linux/process_reader_linux_test.cc b/snapshot/linux/process_reader_linux_test.cc index 940114c6..d827f35d 100644 --- a/snapshot/linux/process_reader_linux_test.cc +++ b/snapshot/linux/process_reader_linux_test.cc @@ -14,6 +14,8 @@ #include "snapshot/linux/process_reader_linux.h" +#include +#include #include #include #include @@ -38,7 +40,11 @@ #include "test/linux/fake_ptrace_connection.h" #include "test/linux/get_tls.h" #include "test/multiprocess.h" +#include "test/scoped_module_handle.h" +#include "test/test_paths.h" #include "util/file/file_io.h" +#include "util/file/file_writer.h" +#include "util/file/filesystem.h" #include "util/linux/direct_ptrace_connection.h" #include "util/misc/address_sanitizer.h" #include "util/misc/from_pointer_cast.h" @@ -507,7 +513,220 @@ void ExpectModulesFromSelf( #endif // !OS_ANDROID || !ARCH_CPU_ARMEL || __ANDROID_API__ >= 21 } +bool WriteTestModule(const base::FilePath& module_path) { +#if defined(ARCH_CPU_64_BITS) + using Ehdr = Elf64_Ehdr; + using Phdr = Elf64_Phdr; + using Shdr = Elf64_Shdr; + using Dyn = Elf64_Dyn; + using Sym = Elf64_Sym; + unsigned char elf_class = ELFCLASS64; +#else + using Ehdr = Elf32_Ehdr; + using Phdr = Elf32_Phdr; + using Shdr = Elf32_Shdr; + using Dyn = Elf32_Dyn; + using Sym = Elf32_Sym; + unsigned char elf_class = ELFCLASS32; +#endif + + struct { + Ehdr ehdr; + struct { + Phdr load1; + Phdr load2; + Phdr dynamic; + } phdr_table; + struct { + Dyn hash; + Dyn strtab; + Dyn symtab; + Dyn strsz; + Dyn syment; + Dyn null; + } dynamic_array; + struct { + Elf32_Word nbucket; + Elf32_Word nchain; + Elf32_Word bucket; + Elf32_Word chain; + } hash_table; + struct { + } string_table; + struct { + Sym und_symbol; + } symbol_table; + struct { + Shdr null; + Shdr dynamic; + Shdr string_table; + } shdr_table; + } module = {}; + + module.ehdr.e_ident[EI_MAG0] = ELFMAG0; + module.ehdr.e_ident[EI_MAG1] = ELFMAG1; + module.ehdr.e_ident[EI_MAG2] = ELFMAG2; + module.ehdr.e_ident[EI_MAG3] = ELFMAG3; + + module.ehdr.e_ident[EI_CLASS] = elf_class; + +#if defined(ARCH_CPU_LITTLE_ENDIAN) + module.ehdr.e_ident[EI_DATA] = ELFDATA2LSB; +#else + module.ehdr.e_ident[EI_DATA] = ELFDATA2MSB; +#endif // ARCH_CPU_LITTLE_ENDIAN + + module.ehdr.e_ident[EI_VERSION] = EV_CURRENT; + + module.ehdr.e_type = ET_DYN; + +#if defined(ARCH_CPU_X86) + module.ehdr.e_machine = EM_386; +#elif defined(ARCH_CPU_X86_64) + module.ehdr.e_machine = EM_X86_64; +#elif defined(ARCH_CPU_ARMEL) + module.ehdr.e_machine = EM_ARM; +#elif defined(ARCH_CPU_ARM64) + module.ehdr.e_machine = EM_AARCH64; +#elif defined(ARCH_CPU_MIPSEL) || defined(ARCH_CPU_MIPS64EL) + module.ehdr.e_machine = EM_MIPS; +#endif + + module.ehdr.e_version = EV_CURRENT; + module.ehdr.e_ehsize = sizeof(module.ehdr); + + module.ehdr.e_phoff = offsetof(decltype(module), phdr_table); + module.ehdr.e_phnum = sizeof(module.phdr_table) / sizeof(Phdr); + module.ehdr.e_phentsize = sizeof(Phdr); + + module.ehdr.e_shoff = offsetof(decltype(module), shdr_table); + module.ehdr.e_shentsize = sizeof(Shdr); + module.ehdr.e_shnum = sizeof(module.shdr_table) / sizeof(Shdr); + module.ehdr.e_shstrndx = SHN_UNDEF; + + constexpr size_t load2_vaddr = 0x200000; + + module.phdr_table.load1.p_type = PT_LOAD; + module.phdr_table.load1.p_offset = 0; + module.phdr_table.load1.p_vaddr = 0; + module.phdr_table.load1.p_filesz = sizeof(module); + module.phdr_table.load1.p_memsz = sizeof(module); + module.phdr_table.load1.p_flags = PF_R; + module.phdr_table.load1.p_align = load2_vaddr; + + module.phdr_table.load2.p_type = PT_LOAD; + module.phdr_table.load2.p_offset = 0; + module.phdr_table.load2.p_vaddr = load2_vaddr; + module.phdr_table.load2.p_filesz = sizeof(module); + module.phdr_table.load2.p_memsz = sizeof(module); + module.phdr_table.load2.p_flags = PF_R | PF_W; + module.phdr_table.load2.p_align = load2_vaddr; + + module.phdr_table.dynamic.p_type = PT_DYNAMIC; + module.phdr_table.dynamic.p_offset = + offsetof(decltype(module), dynamic_array); + module.phdr_table.dynamic.p_vaddr = + load2_vaddr + module.phdr_table.dynamic.p_offset; + module.phdr_table.dynamic.p_filesz = sizeof(module.dynamic_array); + module.phdr_table.dynamic.p_memsz = sizeof(module.dynamic_array); + module.phdr_table.dynamic.p_flags = PF_R | PF_W; + module.phdr_table.dynamic.p_align = 8; + + module.dynamic_array.hash.d_tag = DT_HASH; + module.dynamic_array.hash.d_un.d_ptr = offsetof(decltype(module), hash_table); + module.dynamic_array.strtab.d_tag = DT_STRTAB; + module.dynamic_array.strtab.d_un.d_ptr = + offsetof(decltype(module), string_table); + module.dynamic_array.symtab.d_tag = DT_SYMTAB; + module.dynamic_array.symtab.d_un.d_ptr = + offsetof(decltype(module), symbol_table); + module.dynamic_array.strsz.d_tag = DT_STRSZ; + module.dynamic_array.strsz.d_un.d_val = sizeof(module.string_table); + module.dynamic_array.syment.d_tag = DT_SYMENT; + module.dynamic_array.syment.d_un.d_val = sizeof(Sym); + + module.dynamic_array.null.d_tag = DT_NULL; + + module.hash_table.nbucket = 1; + module.hash_table.nchain = 1; + module.hash_table.bucket = 0; + module.hash_table.chain = 0; + + module.shdr_table.null.sh_type = SHT_NULL; + + module.shdr_table.dynamic.sh_name = 0; + module.shdr_table.dynamic.sh_type = SHT_DYNAMIC; + module.shdr_table.dynamic.sh_flags = SHF_WRITE | SHF_ALLOC; + module.shdr_table.dynamic.sh_addr = module.phdr_table.dynamic.p_vaddr; + module.shdr_table.dynamic.sh_offset = module.phdr_table.dynamic.p_offset; + module.shdr_table.dynamic.sh_size = module.phdr_table.dynamic.p_filesz; + module.shdr_table.dynamic.sh_link = + offsetof(decltype(module.shdr_table), string_table) / sizeof(Shdr); + + module.shdr_table.string_table.sh_name = 0; + module.shdr_table.string_table.sh_type = SHT_STRTAB; + module.shdr_table.string_table.sh_offset = + offsetof(decltype(module), string_table); + + FileWriter writer; + if (!writer.Open(module_path, + FileWriteMode::kCreateOrFail, + FilePermissions::kWorldReadable)) { + ADD_FAILURE(); + return false; + } + + if (!writer.Write(&module, sizeof(module))) { + ADD_FAILURE(); + return false; + } + + return true; +} + +ScopedModuleHandle LoadTestModule(const std::string& module_name) { + base::FilePath module_path( + TestPaths::Executable().DirName().Append(module_name)); + + if (!WriteTestModule(module_path)) { + return ScopedModuleHandle(nullptr); + } + EXPECT_TRUE(IsRegularFile(module_path)); + + ScopedModuleHandle handle( + dlopen(module_path.value().c_str(), RTLD_LAZY | RTLD_LOCAL)); + EXPECT_TRUE(handle.valid()) + << "dlopen: " << module_path.value() << " " << dlerror(); + + EXPECT_TRUE(LoggingRemoveFile(module_path)); + + return handle; +} + +void ExpectTestModule(ProcessReaderLinux* reader, + const std::string& module_name) { + for (const auto& module : reader->Modules()) { + if (module.name.find(module_name) != std::string::npos) { + ASSERT_TRUE(module.elf_reader); + + VMAddress dynamic_addr; + ASSERT_TRUE(module.elf_reader->GetDynamicArrayAddress(&dynamic_addr)); + + auto dynamic_mapping = reader->GetMemoryMap()->FindMapping(dynamic_addr); + auto mappings = + reader->GetMemoryMap()->FindFilePossibleMmapStarts(*dynamic_mapping); + EXPECT_EQ(mappings.size(), 2u); + return; + } + } + ADD_FAILURE() << "Test module not found"; +} + TEST(ProcessReaderLinux, SelfModules) { + const std::string module_name = "test_module.so"; + ScopedModuleHandle empty_test_module(LoadTestModule(module_name)); + ASSERT_TRUE(empty_test_module.valid()); + FakePtraceConnection connection; connection.Initialize(getpid()); @@ -515,15 +734,19 @@ TEST(ProcessReaderLinux, SelfModules) { ASSERT_TRUE(process_reader.Initialize(&connection)); ExpectModulesFromSelf(process_reader.Modules()); + ExpectTestModule(&process_reader, module_name); } class ChildModuleTest : public Multiprocess { public: - ChildModuleTest() : Multiprocess() {} + ChildModuleTest() : Multiprocess(), module_name_("test_module.so") {} ~ChildModuleTest() = default; private: void MultiprocessParent() override { + char c; + ASSERT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, sizeof(c))); + DirectPtraceConnection connection; ASSERT_TRUE(connection.Initialize(ChildPID())); @@ -531,9 +754,20 @@ class ChildModuleTest : public Multiprocess { ASSERT_TRUE(process_reader.Initialize(&connection)); ExpectModulesFromSelf(process_reader.Modules()); + ExpectTestModule(&process_reader, module_name_); } - void MultiprocessChild() override { CheckedReadFileAtEOF(ReadPipeHandle()); } + void MultiprocessChild() override { + ScopedModuleHandle empty_test_module(LoadTestModule(module_name_)); + ASSERT_TRUE(empty_test_module.valid()); + + char c; + ASSERT_TRUE(LoggingWriteFile(WritePipeHandle(), &c, sizeof(c))); + + CheckedReadFileAtEOF(ReadPipeHandle()); + } + + const std::string module_name_; DISALLOW_COPY_AND_ASSIGN(ChildModuleTest); }; diff --git a/test/scoped_module_handle.cc b/test/scoped_module_handle.cc index 3854222b..df246fcf 100644 --- a/test/scoped_module_handle.cc +++ b/test/scoped_module_handle.cc @@ -36,6 +36,11 @@ void ScopedModuleHandle::Impl::Close(ModuleHandle handle) { ScopedModuleHandle::ScopedModuleHandle(ModuleHandle handle) : handle_(handle) {} +ScopedModuleHandle::ScopedModuleHandle(ScopedModuleHandle&& other) + : handle_(other.handle_) { + other.handle_ = nullptr; +} + ScopedModuleHandle::~ScopedModuleHandle() { if (valid()) { Impl::Close(handle_); diff --git a/test/scoped_module_handle.h b/test/scoped_module_handle.h index 9909d1c8..bbab066d 100644 --- a/test/scoped_module_handle.h +++ b/test/scoped_module_handle.h @@ -57,6 +57,7 @@ class ScopedModuleHandle { using ModuleHandle = Impl::ModuleHandle; explicit ScopedModuleHandle(ModuleHandle handle); + ScopedModuleHandle(ScopedModuleHandle&& handle); ~ScopedModuleHandle(); //! \return The module handle being managed. diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc index a274790e..345f63ca 100644 --- a/util/linux/memory_map.cc +++ b/util/linux/memory_map.cc @@ -296,40 +296,39 @@ const MemoryMap::Mapping* MemoryMap::FindMappingWithName( return nullptr; } -const MemoryMap::Mapping* MemoryMap::FindFileMmapStart( +std::vector MemoryMap::FindFilePossibleMmapStarts( const Mapping& mapping) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - size_t index = 0; - for (; index < mappings_.size(); ++index) { - if (mappings_[index].Equals(mapping)) { - break; - } - } - if (index >= mappings_.size()) { - LOG(ERROR) << "mapping not found"; - return nullptr; - } + std::vector possible_starts; // If the mapping is anonymous, as is for the VDSO, there is no mapped file to // find the start of, so just return the input mapping. if (mapping.device == 0 && mapping.inode == 0) { - return &mappings_[index]; + for (const auto& candidate : mappings_) { + if (mapping.Equals(candidate)) { + possible_starts.push_back(&candidate); + return possible_starts; + } + } + + LOG(ERROR) << "mapping not found"; + return std::vector(); } - do { - // There may by anonymous mappings or other files mapped into the holes, - // so check that the mapping uses the same file as the input, but keep - // searching if it doesn't. - if (mappings_[index].device == mapping.device && - mappings_[index].inode == mapping.inode && - mappings_[index].offset == 0) { - return &mappings_[index]; + for (const auto& candidate : mappings_) { + if (candidate.device == mapping.device && + candidate.inode == mapping.inode && + candidate.offset == 0) { + possible_starts.push_back(&candidate); } - } while (index--); + if (mapping.Equals(candidate)) { + return possible_starts; + } + } LOG(ERROR) << "mapping not found"; - return nullptr; + return std::vector(); } } // namespace crashpad diff --git a/util/linux/memory_map.h b/util/linux/memory_map.h index c8276ba6..af194e99 100644 --- a/util/linux/memory_map.h +++ b/util/linux/memory_map.h @@ -76,20 +76,27 @@ class MemoryMap { //! it was obtained from. const Mapping* FindMappingWithName(const std::string& name) const; - //! \brief Find the first Mapping in a series of mappings for the same file. + //! \brief Find Mappings that share a Mapping's file, mapped from offset 0. //! //! Executables and libaries are typically loaded into several mappings with - //! varying permissions for different segments. This method searches for the - //! mapping with the highest address at or below \a mapping, which maps the - //! same file as \a mapping from file offset 0. + //! varying permissions for different segments. Portions of an ELF file may + //! be mapped multiple times as part of loading the file, for example, when + //! initializing GNU_RELRO segments. This method searches for mappings at or + //! below \a mapping in memory that are mapped from the same file as \a + //! mapping from offset 0. //! - //! If \a mapping is not found, `nullptr` is returned. If \a mapping is found - //! but does not map a file, \a mapping is returned. + //! This method is intended to help identify the possible base address for + //! loaded modules, but it is the caller's responsibility to determine which + //! returned mapping is correct. + //! + //! If \a mapping does not refer to a valid mapping, an empty vector will be + //! returned and a message will be logged. If \a mapping is found but does not + //! map a file, \a mapping is returned in \a possible_starts. //! //! \param[in] mapping A Mapping whose series to find the start of. - //! \return The first Mapping in the series or `nullptr` on failure with a - //! message logged. - const Mapping* FindFileMmapStart(const Mapping& mapping) const; + //! \return a vector of the possible mapping starts. + std::vector FindFilePossibleMmapStarts( + const Mapping& mapping) const; private: std::vector mappings_; diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index a056589a..645a0b4d 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -355,8 +355,8 @@ TEST(MemoryMap, MapRunningChild) { // Expects first and third pages from mapping_start to refer to the same mapped // file. The second page should not. -void ExpectFindFileMmapStart(LinuxVMAddress mapping_start, - LinuxVMSize page_size) { +void ExpectFindFilePossibleMmapStarts(LinuxVMAddress mapping_start, + LinuxVMSize page_size) { FakePtraceConnection connection; ASSERT_TRUE(connection.Initialize(getpid())); @@ -373,17 +373,27 @@ void ExpectFindFileMmapStart(LinuxVMAddress mapping_start, ASSERT_NE(mapping1, mapping2); ASSERT_NE(mapping2, mapping3); - EXPECT_EQ(map.FindFileMmapStart(*mapping1), mapping1); - EXPECT_EQ(map.FindFileMmapStart(*mapping2), mapping2); - EXPECT_EQ(map.FindFileMmapStart(*mapping3), mapping1); + std::vector mappings; + + mappings = map.FindFilePossibleMmapStarts(*mapping1); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping1); + + mappings = map.FindFilePossibleMmapStarts(*mapping2); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping2); + + mappings = map.FindFilePossibleMmapStarts(*mapping3); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping1); } -TEST(MemoryMap, FindFileMmapStart) { +TEST(MemoryMap, FindFilePossibleMmapStarts) { const size_t page_size = getpagesize(); ScopedTempDir temp_dir; - base::FilePath path = - temp_dir.path().Append(FILE_PATH_LITERAL("FindFileMmapStartTestFile")); + base::FilePath path = temp_dir.path().Append( + FILE_PATH_LITERAL("FindFilePossibleMmapStartsTestFile")); ScopedFileHandle handle; size_t file_length = page_size * 3; ASSERT_NO_FATAL_FAILURE(InitializeFile(path, file_length, &handle)); @@ -418,9 +428,19 @@ TEST(MemoryMap, FindFileMmapStart) { ASSERT_NE(mapping1, mapping2); ASSERT_NE(mapping2, mapping3); - EXPECT_EQ(map.FindFileMmapStart(*mapping1), mapping1); - EXPECT_EQ(map.FindFileMmapStart(*mapping2), mapping1); - EXPECT_EQ(map.FindFileMmapStart(*mapping3), mapping1); + std::vector mappings; + + mappings = map.FindFilePossibleMmapStarts(*mapping1); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping1); + + mappings = map.FindFilePossibleMmapStarts(*mapping2); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping1); + + mappings = map.FindFilePossibleMmapStarts(*mapping3); + ASSERT_EQ(mappings.size(), 1u); + EXPECT_EQ(mappings[0], mapping1); #if defined(ARCH_CPU_64_BITS) constexpr bool is_64_bit = true; @@ -429,7 +449,7 @@ TEST(MemoryMap, FindFileMmapStart) { #endif MemoryMap::Mapping bad_mapping; bad_mapping.range.SetRange(is_64_bit, 0, 1); - EXPECT_EQ(map.FindFileMmapStart(bad_mapping), nullptr); + EXPECT_EQ(map.FindFilePossibleMmapStarts(bad_mapping).size(), 0u); } // Make the second page an anonymous mapping @@ -449,12 +469,12 @@ TEST(MemoryMap, FindFileMmapStart) { MAP_PRIVATE | MAP_FIXED, handle.get(), page_size * 2)); - ExpectFindFileMmapStart(mapping_start, page_size); + ExpectFindFilePossibleMmapStarts(mapping_start, page_size); // Map the second page to another file. ScopedFileHandle handle2; - base::FilePath path2 = - temp_dir.path().Append(FILE_PATH_LITERAL("FindFileMmapStartTestFile2")); + base::FilePath path2 = temp_dir.path().Append( + FILE_PATH_LITERAL("FindFilePossibleMmapStartsTestFile2")); ASSERT_NO_FATAL_FAILURE(InitializeFile(path2, page_size, &handle2)); page2_mapping.ResetMmap(file_mapping.addr_as() + page_size, @@ -463,7 +483,106 @@ TEST(MemoryMap, FindFileMmapStart) { MAP_PRIVATE | MAP_FIXED, handle2.get(), 0); - ExpectFindFileMmapStart(mapping_start, page_size); + ExpectFindFilePossibleMmapStarts(mapping_start, page_size); +} + +TEST(MemoryMap, FindFilePossibleMmapStarts_MultipleStarts) { + ScopedTempDir temp_dir; + base::FilePath path = + temp_dir.path().Append(FILE_PATH_LITERAL("MultipleStartsTestFile")); + const size_t page_size = getpagesize(); + ScopedFileHandle handle; + ASSERT_NO_FATAL_FAILURE(InitializeFile(path, page_size * 2, &handle)); + + // Locate a sequence of pages to setup a test in. + char* seq_addr; + { + ScopedMmap whole_mapping; + ASSERT_TRUE(whole_mapping.ResetMmap( + nullptr, page_size * 8, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0)); + seq_addr = whole_mapping.addr_as(); + } + + // Arrange file and anonymous mappings in the sequence. + ScopedMmap file_mapping0; + ASSERT_TRUE(file_mapping0.ResetMmap(seq_addr, + page_size, + PROT_READ, + MAP_PRIVATE | MAP_FIXED, + handle.get(), + page_size)); + + ScopedMmap file_mapping1; + ASSERT_TRUE(file_mapping1.ResetMmap(seq_addr + page_size, + page_size * 2, + PROT_READ, + MAP_PRIVATE | MAP_FIXED, + handle.get(), + 0)); + + ScopedMmap file_mapping2; + ASSERT_TRUE(file_mapping2.ResetMmap(seq_addr + page_size * 3, + page_size, + PROT_READ, + MAP_PRIVATE | MAP_FIXED, + handle.get(), + 0)); + + // Skip a page + + ScopedMmap file_mapping3; + ASSERT_TRUE(file_mapping3.ResetMmap(seq_addr + page_size * 5, + page_size, + PROT_READ, + MAP_PRIVATE | MAP_FIXED, + handle.get(), + 0)); + + ScopedMmap anon_mapping; + ASSERT_TRUE(anon_mapping.ResetMmap(seq_addr + page_size * 6, + page_size, + PROT_READ, + MAP_PRIVATE | MAP_ANON | MAP_FIXED, + -1, + 0)); + + ScopedMmap file_mapping4; + ASSERT_TRUE(file_mapping4.ResetMmap(seq_addr + page_size * 7, + page_size, + PROT_READ, + MAP_PRIVATE | MAP_FIXED, + handle.get(), + 0)); + + FakePtraceConnection connection; + ASSERT_TRUE(connection.Initialize(getpid())); + MemoryMap map; + ASSERT_TRUE(map.Initialize(&connection)); + + auto mapping = map.FindMapping(file_mapping0.addr_as()); + ASSERT_TRUE(mapping); + auto possible_starts = map.FindFilePossibleMmapStarts(*mapping); + EXPECT_EQ(possible_starts.size(), 0u); + + mapping = map.FindMapping(file_mapping1.addr_as()); + ASSERT_TRUE(mapping); + possible_starts = map.FindFilePossibleMmapStarts(*mapping); + EXPECT_EQ(possible_starts.size(), 1u); + + mapping = map.FindMapping(file_mapping2.addr_as()); + ASSERT_TRUE(mapping); + possible_starts = map.FindFilePossibleMmapStarts(*mapping); + EXPECT_EQ(possible_starts.size(), 2u); + + mapping = map.FindMapping(file_mapping3.addr_as()); + ASSERT_TRUE(mapping); + possible_starts = map.FindFilePossibleMmapStarts(*mapping); + EXPECT_EQ(possible_starts.size(), 3u); + + mapping = map.FindMapping(file_mapping4.addr_as()); + ASSERT_TRUE(mapping); + possible_starts = map.FindFilePossibleMmapStarts(*mapping); + EXPECT_EQ(possible_starts.size(), 4u); } } // namespace From 063ff78a25713a4a4a32fba8f843a2e7a39d2cdd Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 26 Jul 2018 09:08:01 -0700 Subject: [PATCH 09/11] Use __aarch64__ instead of __arm64__ __aarch64__ should always be defined for 64-bit ARM, while __arm64__ only sometimes is. Change-Id: I46a6469d8f5e74ad79b6ded51a809fbf88e5170a Reviewed-on: https://chromium-review.googlesource.com/1151541 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- compat/linux/sys/ptrace.h | 4 ++-- compat/linux/sys/user.h | 2 +- util/mach/symbolic_constants_mach.cc | 4 ++-- util/mach/symbolic_constants_mach_test.cc | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/compat/linux/sys/ptrace.h b/compat/linux/sys/ptrace.h index fa894381..c5e37a0f 100644 --- a/compat/linux/sys/ptrace.h +++ b/compat/linux/sys/ptrace.h @@ -26,7 +26,7 @@ static constexpr __ptrace_request PTRACE_GET_THREAD_AREA = static_cast<__ptrace_request>(25); #define PTRACE_GET_THREAD_AREA PTRACE_GET_THREAD_AREA -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) static constexpr __ptrace_request PTRACE_GET_THREAD_AREA = static_cast<__ptrace_request>(22); #define PTRACE_GET_THREAD_AREA PTRACE_GET_THREAD_AREA @@ -42,7 +42,7 @@ static constexpr __ptrace_request PTRACE_GET_THREAD_AREA_3264 = // https://sourceware.org/bugzilla/show_bug.cgi?id=22433 #if !defined(PTRACE_GETVFPREGS) && !defined(PT_GETVFPREGS) && \ - defined(__GLIBC__) && (defined(__arm__) || defined(__arm64__)) + defined(__GLIBC__) && (defined(__arm__) || defined(__aarch64__)) static constexpr __ptrace_request PTRACE_GETVFPREGS = static_cast<__ptrace_request>(27); #define PTRACE_GETVFPREGS PTRACE_GETVFPREGS diff --git a/compat/linux/sys/user.h b/compat/linux/sys/user.h index 0ce5338b..6ed77a98 100644 --- a/compat/linux/sys/user.h +++ b/compat/linux/sys/user.h @@ -20,7 +20,7 @@ #include // glibc for 64-bit ARM uses different names for these structs prior to 2.20. -#if defined(__arm64__) && defined(__GLIBC__) +#if defined(__aarch64__) && defined(__GLIBC__) #if !__GLIBC_PREREQ(2, 20) using user_regs_struct = user_pt_regs; using user_fpsimd_struct = user_fpsimd_state; diff --git a/util/mach/symbolic_constants_mach.cc b/util/mach/symbolic_constants_mach.cc index 8f95915e..98a2d203 100644 --- a/util/mach/symbolic_constants_mach.cc +++ b/util/mach/symbolic_constants_mach.cc @@ -101,7 +101,7 @@ constexpr const char* kFlavorNames[] = { "PPC_THREAD_STATE64", "PPC_EXCEPTION_STATE64", "THREAD_STATE_NONE", -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) // sed -Ene 's/^#define ((ARM|THREAD)_[[:graph:]]+)[[:space:]]+[[:digit:]]{1,2}.*$/ "\1",/p' // usr/include/mach/arm/thread_status.h // (iOS 7 SDK) @@ -153,7 +153,7 @@ std::string ThreadStateFlavorFullToShort(const base::StringPiece& flavor) { static constexpr char kArchPrefix[] = "x86_"; #elif defined(__ppc__) || defined(__ppc64__) static constexpr char kArchPrefix[] = "PPC_"; -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) static constexpr char kArchPrefix[] = "ARM_" #endif prefix_len = strlen(kArchPrefix); diff --git a/util/mach/symbolic_constants_mach_test.cc b/util/mach/symbolic_constants_mach_test.cc index 32711412..3bf5abb1 100644 --- a/util/mach/symbolic_constants_mach_test.cc +++ b/util/mach/symbolic_constants_mach_test.cc @@ -801,7 +801,7 @@ constexpr struct { {PPC_VECTOR_STATE, "PPC_VECTOR_STATE", "VECTOR"}, {PPC_THREAD_STATE64, "PPC_THREAD_STATE64", "THREAD64"}, {PPC_EXCEPTION_STATE64, "PPC_EXCEPTION_STATE64", "EXCEPTION64"}, -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) {ARM_THREAD_STATE, "ARM_THREAD_STATE", "THREAD"}, {ARM_VFP_STATE, "ARM_VFP_STATE", "VFP"}, {ARM_EXCEPTION_STATE, "ARM_EXCEPTION_STATE", "EXCEPTION"}, @@ -860,7 +860,7 @@ TEST(SymbolicConstantsMach, ThreadStateFlavorToString) { flavor <= x86_AVX_STATE #elif defined(__ppc__) || defined(__ppc64__) flavor <= THREAD_STATE_NONE -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) (flavor <= ARM_EXCEPTION_STATE64 || flavor == ARM_THREAD_STATE32 || (flavor >= ARM_DEBUG_STATE32 && flavor <= ARM_NEON_STATE64)) #endif @@ -948,7 +948,7 @@ TEST(SymbolicConstantsMach, StringToThreadStateFlavor) { "PPC_JUNK_STATE32", "x86_THREAD_STATE", "ARM_THREAD_STATE", -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) " ARM_THREAD_STATE64", "ARM_THREAD_STATE64 ", "ARM_THREAD_STATE642", @@ -1013,7 +1013,7 @@ TEST(SymbolicConstantsMach, StringToThreadStateFlavor) { NUL_TEST_DATA("PPC_THREAD_\0STATE64"), NUL_TEST_DATA("PPC_THREAD_STA\0TE64"), NUL_TEST_DATA("PPC_THREAD_STATE\00064"), -#elif defined(__arm__) || defined(__arm64__) +#elif defined(__arm__) || defined(__aarch64__) NUL_TEST_DATA("\0ARM_THREAD_STATE64"), NUL_TEST_DATA("ARM\0_THREAD_STATE64"), NUL_TEST_DATA("ARM_\0THREAD_STATE64"), From 1e662c2fcb6c0e0c37f55861cb5cec662e3766bf Mon Sep 17 00:00:00 2001 From: Ryan Tseng Date: Wed, 25 Jul 2018 17:44:36 -0700 Subject: [PATCH 10/11] [cq.cfg] Add all luci trybots as experimental Bug: 865729 Change-Id: I5236736803de19cf242a3ae9657b99a03420b08c Reviewed-on: https://chromium-review.googlesource.com/1150905 Reviewed-by: Dirk Pranke Commit-Queue: Ryan Tseng --- infra/config/cq.cfg | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/infra/config/cq.cfg b/infra/config/cq.cfg index b6bf0434..5fc8bad7 100644 --- a/infra/config/cq.cfg +++ b/infra/config/cq.cfg @@ -48,5 +48,22 @@ verifiers { #builders { name: "crashpad_try_win_x86_dbg" } #builders { name: "crashpad_try_win_x86_rel" } } + buckets { + name: "luci.crashpad.try" + builders { name: "crashpad_try_mac_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_mac_rel" experiment_percentage: 100 } + builders { name: "crashpad_try_win_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_win_rel" experiment_percentage: 100 } + builders { name: "crashpad_try_linux_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_linux_rel" experiment_percentage: 100 } + builders { name: "crashpad_try_fuchsia_arm64_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_fuchsia_arm64_rel" experiment_percentage: 100 } + builders { name: "crashpad_try_fuchsia_x64_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_fuchsia_x64_rel" experiment_percentage: 100 } + # https://crbug.com/743139 - disabled until we can move these to swarming, + # at which point we can just remove them. + #builders { name: "crashpad_try_win_x86_dbg" } + #builders { name: "crashpad_try_win_x86_rel" } + } } } From 42b57efa554a47745cb84860ead46bc9f32b77a9 Mon Sep 17 00:00:00 2001 From: Ryan Tseng Date: Fri, 27 Jul 2018 13:10:47 -0700 Subject: [PATCH 11/11] CQ: Flip all builders except Windows to LUCI Bug: 865729 Change-Id: I73ed47339c3374d86ee82609a18a4e728c601ab1 Reviewed-on: https://chromium-review.googlesource.com/1153610 Reviewed-by: Dirk Pranke Reviewed-by: Mark Mentovai Commit-Queue: Ryan Tseng --- infra/config/cq.cfg | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/infra/config/cq.cfg b/infra/config/cq.cfg index 5fc8bad7..1fc33243 100644 --- a/infra/config/cq.cfg +++ b/infra/config/cq.cfg @@ -33,10 +33,19 @@ verifiers { try_job { buckets { name: "master.client.crashpad" - builders { name: "crashpad_try_mac_dbg" } - builders { name: "crashpad_try_mac_rel" } builders { name: "crashpad_try_win_dbg" } builders { name: "crashpad_try_win_rel" } + # https://crbug.com/743139 - disabled until we can move these to swarming, + # at which point we can just remove them. + #builders { name: "crashpad_try_win_x86_dbg" } + #builders { name: "crashpad_try_win_x86_rel" } + } + buckets { + name: "luci.crashpad.try" + builders { name: "crashpad_try_mac_dbg" } + builders { name: "crashpad_try_mac_rel" } + builders { name: "crashpad_try_win_dbg" experiment_percentage: 100 } + builders { name: "crashpad_try_win_rel" experiment_percentage: 100 } builders { name: "crashpad_try_linux_dbg" } builders { name: "crashpad_try_linux_rel" } builders { name: "crashpad_try_fuchsia_arm64_dbg" } @@ -48,22 +57,5 @@ verifiers { #builders { name: "crashpad_try_win_x86_dbg" } #builders { name: "crashpad_try_win_x86_rel" } } - buckets { - name: "luci.crashpad.try" - builders { name: "crashpad_try_mac_dbg" experiment_percentage: 100 } - builders { name: "crashpad_try_mac_rel" experiment_percentage: 100 } - builders { name: "crashpad_try_win_dbg" experiment_percentage: 100 } - builders { name: "crashpad_try_win_rel" experiment_percentage: 100 } - builders { name: "crashpad_try_linux_dbg" experiment_percentage: 100 } - builders { name: "crashpad_try_linux_rel" experiment_percentage: 100 } - builders { name: "crashpad_try_fuchsia_arm64_dbg" experiment_percentage: 100 } - builders { name: "crashpad_try_fuchsia_arm64_rel" experiment_percentage: 100 } - builders { name: "crashpad_try_fuchsia_x64_dbg" experiment_percentage: 100 } - builders { name: "crashpad_try_fuchsia_x64_rel" experiment_percentage: 100 } - # https://crbug.com/743139 - disabled until we can move these to swarming, - # at which point we can just remove them. - #builders { name: "crashpad_try_win_x86_dbg" } - #builders { name: "crashpad_try_win_x86_rel" } - } } }