From d4bdb997a6605125909efab3e0e11e7cdf41cb64 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 10 Nov 2021 13:04:10 -0500 Subject: [PATCH] ios: Store Crashpad client annotations in the intermediate dump. The iOS crashpad client was mistakenly setting the process annotations (typically things like version and product name) when converting the intermediate dump into a minidump. This is incorrect, as those annotations are determined at intermediate dump creation time. Instead, correctly write those annotations during intermediate dump creation. Passing extra annotations during intermediate dump to minidump is still supported. Bug: crashpad: 31 Change-Id: Ic5e29debdc123011d130f75a48345071575466d9 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3266127 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- client/ios_handler/in_process_handler.cc | 41 +++++++------------ client/ios_handler/in_process_handler.h | 4 +- .../in_process_intermediate_dump_handler.cc | 21 +++++++++- .../in_process_intermediate_dump_handler.h | 7 +++- ..._process_intermediate_dump_handler_test.cc | 6 ++- .../process_snapshot_ios_intermediate_dump.cc | 15 +++++++ test/ios/crash_type_xctest.mm | 27 ++++++++++++ test/ios/host/cptest_application_delegate.mm | 33 ++++++++++++++- test/ios/host/cptest_shared_object.h | 4 ++ 9 files changed, 124 insertions(+), 34 deletions(-) diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc index 0b4c4b74..6e440bef 100644 --- a/client/ios_handler/in_process_handler.cc +++ b/client/ios_handler/in_process_handler.cc @@ -114,7 +114,7 @@ void InProcessHandler::DumpExceptionFromSignal( ucontext_t* context) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); { - ScopedReport report(writer_.get(), system_data); + ScopedReport report(writer_.get(), system_data, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromSignal( writer_.get(), system_data, siginfo, context); } @@ -133,7 +133,7 @@ void InProcessHandler::DumpExceptionFromMachException( mach_msg_type_number_t old_state_count) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); { - ScopedReport report(writer_.get(), system_data); + ScopedReport report(writer_.get(), system_data, annotations_); InProcessIntermediateDumpHandler::WriteExceptionFromMachException( writer_.get(), behavior, @@ -153,7 +153,8 @@ void InProcessHandler::DumpExceptionFromNSExceptionFrames( const uint64_t* frames, const size_t num_frames) { { - ScopedReport report(writer_.get(), system_data, frames, num_frames); + ScopedReport report( + writer_.get(), system_data, annotations_, frames, num_frames); InProcessIntermediateDumpHandler::WriteExceptionFromNSException( writer_.get()); } @@ -161,34 +162,14 @@ void InProcessHandler::DumpExceptionFromNSExceptionFrames( } void InProcessHandler::ProcessIntermediateDumps( - const std::map& extra_annotations) { + const std::map& annotations) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - std::map annotations(annotations_); - annotations.insert(extra_annotations.begin(), extra_annotations.end()); - for (auto& file : PendingFiles()) - ProcessIntermediateDumpWithCompleteAnnotations(file, annotations); + ProcessIntermediateDump(file, annotations); } void InProcessHandler::ProcessIntermediateDump( - const base::FilePath& file, - const std::map& extra_annotations) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - std::map annotations(annotations_); - annotations.insert(extra_annotations.begin(), extra_annotations.end()); - ProcessIntermediateDumpWithCompleteAnnotations(file, annotations); -} - -void InProcessHandler::StartProcessingPendingReports() { - if (!upload_thread_started_ && upload_thread_) { - upload_thread_->Start(); - upload_thread_started_ = true; - } -} - -void InProcessHandler::ProcessIntermediateDumpWithCompleteAnnotations( const base::FilePath& file, const std::map& annotations) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); @@ -199,6 +180,13 @@ void InProcessHandler::ProcessIntermediateDumpWithCompleteAnnotations( } } +void InProcessHandler::StartProcessingPendingReports() { + if (!upload_thread_started_ && upload_thread_) { + upload_thread_->Start(); + upload_thread_started_ = true; + } +} + void InProcessHandler::SaveSnapshot( ProcessSnapshotIOSIntermediateDump& process_snapshot) { std::unique_ptr new_report; @@ -293,11 +281,12 @@ InProcessHandler::ScopedAlternateWriter::~ScopedAlternateWriter() { InProcessHandler::ScopedReport::ScopedReport( IOSIntermediateDumpWriter* writer, const IOSSystemDataCollector& system_data, + const std::map& annotations, const uint64_t* frames, const size_t num_frames) : rootMap_(writer) { InProcessIntermediateDumpHandler::WriteHeader(writer); - InProcessIntermediateDumpHandler::WriteProcessInfo(writer); + InProcessIntermediateDumpHandler::WriteProcessInfo(writer, annotations); InProcessIntermediateDumpHandler::WriteSystemInfo(writer, system_data); InProcessIntermediateDumpHandler::WriteThreadInfo(writer, frames, num_frames); InProcessIntermediateDumpHandler::WriteModuleInfo(writer); diff --git a/client/ios_handler/in_process_handler.h b/client/ios_handler/in_process_handler.h index 1e6ebc4b..888337b9 100644 --- a/client/ios_handler/in_process_handler.h +++ b/client/ios_handler/in_process_handler.h @@ -156,6 +156,7 @@ class InProcessHandler { public: ScopedReport(IOSIntermediateDumpWriter* writer, const IOSSystemDataCollector& system_data, + const std::map& annotations, const uint64_t* frames = nullptr, const size_t num_frames = 0); ~ScopedReport() {} @@ -178,9 +179,6 @@ class InProcessHandler { open_new_file_after_report_ = open_new_file_after_report; } - void ProcessIntermediateDumpWithCompleteAnnotations( - const base::FilePath& file, - const std::map& annotations); void SaveSnapshot(ProcessSnapshotIOSIntermediateDump& process_snapshot); std::vector PendingFiles(); bool OpenNewFile(); diff --git a/client/ios_handler/in_process_intermediate_dump_handler.cc b/client/ios_handler/in_process_intermediate_dump_handler.cc index e2c77223..12dcf2b4 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler.cc +++ b/client/ios_handler/in_process_intermediate_dump_handler.cc @@ -582,7 +582,8 @@ void InProcessIntermediateDumpHandler::WriteHeader( // static void InProcessIntermediateDumpHandler::WriteProcessInfo( - IOSIntermediateDumpWriter* writer) { + IOSIntermediateDumpWriter* writer, + const std::map& annotations) { IOSIntermediateDumpWriter::ScopedMap process_map( writer, IntermediateDumpKey::kProcessInfo); @@ -647,6 +648,24 @@ void InProcessIntermediateDumpHandler::WriteProcessInfo( } else { CRASHPAD_RAW_LOG("task_info task_basic_info"); } + + if (!annotations.empty()) { + IOSIntermediateDumpWriter::ScopedArray simple_annotations_array( + writer, IntermediateDumpKey::kAnnotationsSimpleMap); + for (const auto& annotation_pair : annotations) { + const std::string& key = annotation_pair.first; + const std::string& value = annotation_pair.second; + IOSIntermediateDumpWriter::ScopedArrayMap annotation_map(writer); + WriteProperty(writer, + IntermediateDumpKey::kAnnotationName, + key.c_str(), + key.length()); + WriteProperty(writer, + IntermediateDumpKey::kAnnotationValue, + value.c_str(), + value.length()); + } + } } // static diff --git a/client/ios_handler/in_process_intermediate_dump_handler.h b/client/ios_handler/in_process_intermediate_dump_handler.h index 62815630..f6db28d5 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler.h +++ b/client/ios_handler/in_process_intermediate_dump_handler.h @@ -20,6 +20,8 @@ #include #include +#include + #include "client/crashpad_info.h" #include "util/ios/ios_intermediate_dump_writer.h" #include "util/ios/ios_system_data_collector.h" @@ -46,7 +48,10 @@ class InProcessIntermediateDumpHandler final { //! \brief Write ProcessSnapshot data to the intermediate dump. //! //! \param[in] writer The dump writer - static void WriteProcessInfo(IOSIntermediateDumpWriter* writer); + //! \param[in] annotations The simple map annotations. + static void WriteProcessInfo( + IOSIntermediateDumpWriter* writer, + const std::map& annotations); //! \brief Write SystemSnapshot data to the intermediate dump. //! diff --git a/client/ios_handler/in_process_intermediate_dump_handler_test.cc b/client/ios_handler/in_process_intermediate_dump_handler_test.cc index 9616ce75..26c40c0b 100644 --- a/client/ios_handler/in_process_intermediate_dump_handler_test.cc +++ b/client/ios_handler/in_process_intermediate_dump_handler_test.cc @@ -55,7 +55,8 @@ class InProcessIntermediateDumpHandlerTest : public testing::Test { void WriteReport() { internal::IOSIntermediateDumpWriter::ScopedRootMap rootMap(writer_.get()); InProcessIntermediateDumpHandler::WriteHeader(writer_.get()); - InProcessIntermediateDumpHandler::WriteProcessInfo(writer_.get()); + InProcessIntermediateDumpHandler::WriteProcessInfo( + writer_.get(), {{"before_dump", "pre"}}); InProcessIntermediateDumpHandler::WriteSystemInfo(writer_.get(), system_data_); InProcessIntermediateDumpHandler::WriteThreadInfo(writer_.get(), 0, 0); @@ -147,7 +148,8 @@ TEST_F(InProcessIntermediateDumpHandlerTest, TestAnnotations) { ASSERT_TRUE(process_snapshot.Initialize(path(), {{"after_dump", "post"}})); auto process_map = process_snapshot.AnnotationsSimpleMap(); - EXPECT_EQ(process_map.size(), 1u); + EXPECT_EQ(process_map.size(), 2u); + EXPECT_EQ(process_map["before_dump"], "pre"); EXPECT_EQ(process_map["after_dump"], "post"); std::map all_annotations_simple_map; diff --git a/snapshot/ios/process_snapshot_ios_intermediate_dump.cc b/snapshot/ios/process_snapshot_ios_intermediate_dump.cc index 619ea958..f4769ff6 100644 --- a/snapshot/ios/process_snapshot_ios_intermediate_dump.cc +++ b/snapshot/ios/process_snapshot_ios_intermediate_dump.cc @@ -84,6 +84,21 @@ bool ProcessSnapshotIOSIntermediateDump::Initialize( GetDataValueFromMap(process_info, Key::kSnapshotTime, &snapshot_time_); + const IOSIntermediateDumpList* simple_map_dump = + process_info->GetAsList(IntermediateDumpKey::kAnnotationsSimpleMap); + if (simple_map_dump) { + for (auto& annotation : *simple_map_dump) { + const IOSIntermediateDumpData* name_dump = + annotation->GetAsData(IntermediateDumpKey::kAnnotationName); + const IOSIntermediateDumpData* value_dump = + annotation->GetAsData(IntermediateDumpKey::kAnnotationValue); + if (name_dump && value_dump) { + annotations_simple_map_.insert( + make_pair(name_dump->GetString(), value_dump->GetString())); + } + } + } + const IOSIntermediateDumpMap* system_info = GetMapFromMap(root_map, Key::kSystemInfo); if (!system_info) { diff --git a/test/ios/crash_type_xctest.mm b/test/ios/crash_type_xctest.mm index b4b22cb5..c9317965 100644 --- a/test/ios/crash_type_xctest.mm +++ b/test/ios/crash_type_xctest.mm @@ -184,6 +184,33 @@ [self verifyCrashReportException:SIGHUP]; } +- (void)testClientAnnotations { + [rootObject_ crashKillAbort]; + + // Set app launch args to trigger different client annotations. + NSArray* old_args = app_.launchArguments; + app_.launchArguments = @[ @"--alternate-client-annotations" ]; + [self verifyCrashReportException:SIGABRT]; + app_.launchArguments = old_args; + + // Confirm the initial crash took the standard annotations. + NSDictionary* dict = [rootObject_ getProcessAnnotations]; + XCTAssertTrue([dict[@"crashpad"] isEqualToString:@"yes"]); + XCTAssertTrue([dict[@"plat"] isEqualToString:@"iOS"]); + XCTAssertTrue([dict[@"prod"] isEqualToString:@"xcuitest"]); + XCTAssertTrue([dict[@"ver"] isEqualToString:@"1"]); + + // Confirm passing alternate client annotation args works. + [rootObject_ clearPendingReports]; + [rootObject_ crashKillAbort]; + [self verifyCrashReportException:SIGABRT]; + dict = [rootObject_ getProcessAnnotations]; + XCTAssertTrue([dict[@"crashpad"] isEqualToString:@"no"]); + XCTAssertTrue([dict[@"plat"] isEqualToString:@"macOS"]); + XCTAssertTrue([dict[@"prod"] isEqualToString:@"some_app"]); + XCTAssertTrue([dict[@"ver"] isEqualToString:@"42"]); +} + - (void)testCrashWithCrashInfoMessage { if (@available(iOS 15.0, *)) { // Figure out how to test this on iOS15. diff --git a/test/ios/host/cptest_application_delegate.mm b/test/ios/host/cptest_application_delegate.mm index 6216257a..6ff8378a 100644 --- a/test/ios/host/cptest_application_delegate.mm +++ b/test/ios/host/cptest_application_delegate.mm @@ -92,7 +92,18 @@ OperationStatus GetPendingReports(std::vector* pending_reports) { - (BOOL)application:(UIApplication*)application didFinishLaunchingWithOptions:(NSDictionary*)launchOptions { // Start up crashpad. - if (client_.StartCrashpadInProcessHandler(GetDatabaseDir(), "", {})) { + std::map annotations = { + {"prod", "xcuitest"}, {"ver", "1"}, {"plat", "iOS"}, {"crashpad", "yes"}}; + + NSArray* arguments = [[NSProcessInfo processInfo] arguments]; + if ([arguments containsObject:@"--alternate-client-annotations"]) { + annotations = {{"prod", "some_app"}, + {"ver", "42"}, + {"plat", "macOS"}, + {"crashpad", "no"}}; + } + if (client_.StartCrashpadInProcessHandler( + GetDatabaseDir(), "", annotations)) { client_.ProcessIntermediateDumps(); } @@ -203,6 +214,26 @@ OperationStatus GetPendingReports(std::vector* pending_reports) { return [dict passByValue]; } +- (NSDictionary*)getProcessAnnotations { + std::vector pending_reports; + OperationStatus status = GetPendingReports(&pending_reports); + if (status != crashpad::CrashReportDatabase::kNoError || + pending_reports.size() != 1) { + return @{}; + } + + auto reader = std::make_unique(); + reader->Open(pending_reports[0].file_path); + crashpad::ProcessSnapshotMinidump process_snapshot; + process_snapshot.Initialize(reader.get()); + NSDictionary* dict = [@{} mutableCopy]; + for (const auto& kv : process_snapshot.AnnotationsSimpleMap()) { + [dict setValue:@(kv.second.c_str()) forKey:@(kv.first.c_str())]; + } + + return [dict passByValue]; +} + - (void)crashBadAccess { strcpy(nullptr, "bla"); } diff --git a/test/ios/host/cptest_shared_object.h b/test/ios/host/cptest_shared_object.h index e75849b3..4a781933 100644 --- a/test/ios/host/cptest_shared_object.h +++ b/test/ios/host/cptest_shared_object.h @@ -42,6 +42,10 @@ // (strings only) respectively. - (NSDictionary*)getAnnotations; +// Return an NSDictionary representing the ProcessSnapshotMinidump +// AnnotationsSimpleMap. +- (NSDictionary*)getProcessAnnotations; + // Triggers an EXC_BAD_ACCESS exception and crash. - (void)crashBadAccess;