From 22c386d1ac092bdd9e9ea720f68a6f054ea9e6e3 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Fri, 19 Jan 2024 13:41:26 -0500 Subject: [PATCH] ios: Allow missing exception thread id from thread list. It's expected that iOS intermediate dumps can be written with missing information, but it's better to try and report as much as possible rather than drop the incomplete minidump. Bug: b/284959148 Change-Id: I04110b576a4ee552814234d559c9ba85db0382f0 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4582167 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- minidump/minidump_exception_writer.cc | 12 +++++++++--- minidump/minidump_exception_writer.h | 7 ++++++- minidump/minidump_exception_writer_test.cc | 5 ++++- minidump/minidump_file_writer.cc | 12 +++++++++++- ...rocess_snapshot_ios_intermediate_dump_test.cc | 16 ++++++++++++++++ 5 files changed, 46 insertions(+), 6 deletions(-) diff --git a/minidump/minidump_exception_writer.cc b/minidump/minidump_exception_writer.cc index 3057c6a7..cd3a139d 100644 --- a/minidump/minidump_exception_writer.cc +++ b/minidump/minidump_exception_writer.cc @@ -34,13 +34,19 @@ MinidumpExceptionWriter::~MinidumpExceptionWriter() { void MinidumpExceptionWriter::InitializeFromSnapshot( const ExceptionSnapshot* exception_snapshot, - const MinidumpThreadIDMap& thread_id_map) { + const MinidumpThreadIDMap& thread_id_map, + bool allow_missing_thread_id_from_map) { DCHECK_EQ(state(), kStateMutable); DCHECK(!context_); auto thread_id_it = thread_id_map.find(exception_snapshot->ThreadID()); - DCHECK(thread_id_it != thread_id_map.end()); - SetThreadID(thread_id_it->second); + bool thread_id_missing = thread_id_it == thread_id_map.end(); + if (allow_missing_thread_id_from_map && thread_id_missing) { + SetThreadID(static_cast(exception_snapshot->ThreadID())); + } else { + DCHECK(!thread_id_missing); + SetThreadID(thread_id_it->second); + } SetExceptionCode(exception_snapshot->Exception()); SetExceptionFlags(exception_snapshot->ExceptionInfo()); diff --git a/minidump/minidump_exception_writer.h b/minidump/minidump_exception_writer.h index 8e6847e9..4c711127 100644 --- a/minidump/minidump_exception_writer.h +++ b/minidump/minidump_exception_writer.h @@ -50,12 +50,17 @@ class MinidumpExceptionWriter final : public internal::MinidumpStreamWriter { //! \param[in] thread_id_map A MinidumpThreadIDMap to be consulted to //! determine the 32-bit minidump thread ID to use for the thread //! identified by \a exception_snapshot. + //! \param[in] allow_missing_thread_id_from_map Whether it is valid + //! for \a exception_snapshot->ThreadID() to be absent from the + //! \a thread_id_map, such as in an incomplete iOS intermediate dump. When + //! false a missing thread id is considered invalid and will DCHECK. //! //! \note Valid in #kStateMutable. No mutator methods may be called before //! this method, and it is not normally necessary to call any mutator //! methods after this method. void InitializeFromSnapshot(const ExceptionSnapshot* exception_snapshot, - const MinidumpThreadIDMap& thread_id_map); + const MinidumpThreadIDMap& thread_id_map, + bool allow_missing_thread_id_from_map); //! \brief Arranges for MINIDUMP_EXCEPTION_STREAM::ThreadContext to point to //! the CPU context to be written by \a context. diff --git a/minidump/minidump_exception_writer_test.cc b/minidump/minidump_exception_writer_test.cc index 9b5e1f9f..06a921ea 100644 --- a/minidump/minidump_exception_writer_test.cc +++ b/minidump/minidump_exception_writer_test.cc @@ -235,7 +235,10 @@ TEST(MinidumpExceptionWriter, InitializeFromSnapshot) { thread_id_map[kThreadID] = expect_exception.ThreadId; auto exception_writer = std::make_unique(); - exception_writer->InitializeFromSnapshot(&exception_snapshot, thread_id_map); + exception_writer->InitializeFromSnapshot( + &exception_snapshot, + thread_id_map, + /*allow_missing_thread_id_from_map=*/false); MinidumpFileWriter minidump_file_writer; ASSERT_TRUE(minidump_file_writer.AddStream(std::move(exception_writer))); diff --git a/minidump/minidump_file_writer.cc b/minidump/minidump_file_writer.cc index 021b1876..35d226bb 100644 --- a/minidump/minidump_file_writer.cc +++ b/minidump/minidump_file_writer.cc @@ -18,6 +18,7 @@ #include "base/check_op.h" #include "base/logging.h" +#include "build/build_config.h" #include "minidump/minidump_crashpad_info_writer.h" #include "minidump/minidump_exception_writer.h" #include "minidump/minidump_handle_writer.h" @@ -115,7 +116,16 @@ void MinidumpFileWriter::InitializeFromSnapshot( const ExceptionSnapshot* exception_snapshot = process_snapshot->Exception(); if (exception_snapshot) { auto exception = std::make_unique(); - exception->InitializeFromSnapshot(exception_snapshot, thread_id_map); +#if BUILDFLAG(IS_IOS) + // It's expected that iOS intermediate dumps can be written with missing + // information, but it's better to try and report as much as possible + // rather than drop the incomplete minidump. + constexpr bool allow_missing_thread_id_from_map = true; +#else + constexpr bool allow_missing_thread_id_from_map = false; +#endif + exception->InitializeFromSnapshot( + exception_snapshot, thread_id_map, allow_missing_thread_id_from_map); add_stream_result = AddStream(std::move(exception)); DCHECK(add_stream_result); } diff --git a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc index 238c52e9..29d26a83 100644 --- a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc +++ b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc @@ -775,6 +775,22 @@ TEST_F(ProcessSnapshotIOSIntermediateDumpTest, FuzzTestCases) { EXPECT_TRUE(process_snapshot4.InitializeWithFilePath(fuzz_path, {})); } +TEST_F(ProcessSnapshotIOSIntermediateDumpTest, WriteNoThreads) { + { + IOSIntermediateDumpWriter::ScopedRootMap rootMap(writer()); + uint8_t version = 1; + EXPECT_TRUE(writer()->AddProperty(Key::kVersion, &version)); + WriteSystemInfo(writer()); + WriteProcessInfo(writer()); + WriteMachException(writer()); + } + CloseWriter(); + ProcessSnapshotIOSIntermediateDump process_snapshot; + ASSERT_TRUE(process_snapshot.InitializeWithFilePath(path(), annotations())); + EXPECT_FALSE(IsRegularFile(path())); + EXPECT_TRUE(DumpSnapshot(process_snapshot)); +} + } // namespace } // namespace test } // namespace crashpad