diff --git a/client/ios_handler/in_process_handler.cc b/client/ios_handler/in_process_handler.cc index 285218fd..67987348 100644 --- a/client/ios_handler/in_process_handler.cc +++ b/client/ios_handler/in_process_handler.cc @@ -308,12 +308,21 @@ InProcessHandler::ScopedReport::ScopedReport( const std::map& annotations, const uint64_t* frames, const size_t num_frames) - : rootMap_(writer) { + : writer_(writer), + frames_(frames), + num_frames_(num_frames), + rootMap_(writer) { InProcessIntermediateDumpHandler::WriteHeader(writer); InProcessIntermediateDumpHandler::WriteProcessInfo(writer, annotations); InProcessIntermediateDumpHandler::WriteSystemInfo(writer, system_data); - InProcessIntermediateDumpHandler::WriteThreadInfo(writer, frames, num_frames); - InProcessIntermediateDumpHandler::WriteModuleInfo(writer); +} + +InProcessHandler::ScopedReport::~ScopedReport() { + // Write threads and modules last (after the exception itself is written by + // DumpExceptionFrom*.) + InProcessIntermediateDumpHandler::WriteThreadInfo( + writer_, frames_, num_frames_); + InProcessIntermediateDumpHandler::WriteModuleInfo(writer_); } bool InProcessHandler::OpenNewFile() { diff --git a/client/ios_handler/in_process_handler.h b/client/ios_handler/in_process_handler.h index e586219a..d94f19e7 100644 --- a/client/ios_handler/in_process_handler.h +++ b/client/ios_handler/in_process_handler.h @@ -159,11 +159,14 @@ class InProcessHandler { const std::map& annotations, const uint64_t* frames = nullptr, const size_t num_frames = 0); - ~ScopedReport() {} + ~ScopedReport(); ScopedReport(const ScopedReport&) = delete; ScopedReport& operator=(const ScopedReport&) = delete; private: + IOSIntermediateDumpWriter* writer_; + const uint64_t* frames_; + const size_t num_frames_; IOSIntermediateDumpWriter::ScopedRootMap rootMap_; }; diff --git a/snapshot/ios/exception_snapshot_ios_intermediate_dump.cc b/snapshot/ios/exception_snapshot_ios_intermediate_dump.cc index 5ad02076..c4d99e77 100644 --- a/snapshot/ios/exception_snapshot_ios_intermediate_dump.cc +++ b/snapshot/ios/exception_snapshot_ios_intermediate_dump.cc @@ -293,36 +293,40 @@ void ExceptionSnapshotIOSIntermediateDump::LoadContextFromThread( const IOSIntermediateDumpData* state_dump = GetDataFromMap(exception_data, Key::kState); if (state_dump) { - const std::vector& bytes = state_dump->bytes(); + std::vector bytes = state_dump->bytes(); size_t actual_length = bytes.size(); size_t expected_length = ThreadStateLengthForFlavor(flavor); - // TODO(justincohen): Consider zero-ing out bytes if actual_length is - // shorter than expected_length, and tolerating actual_length longer than - // expected_length. - if (expected_length == actual_length) { - const ConstThreadState state = - reinterpret_cast(bytes.data()); - mach_msg_type_number_t state_count = bytes.size() / sizeof(uint32_t); + if (actual_length < expected_length) { + // Zero out bytes if actual_length is shorter than expected_length. + bytes.resize(expected_length, 0); + actual_length = bytes.size(); + LOG(WARNING) << "Exception context length " << actual_length + << " shorter than expected length " << expected_length; + } + const ConstThreadState state = + reinterpret_cast(bytes.data()); + // Tolerating actual_length longer than expected_length by setting + // state_count based on expected_length, not bytes.size(). + mach_msg_type_number_t state_count = expected_length / sizeof(uint32_t); #if defined(ARCH_CPU_X86_64) - InitializeCPUContextX86_64(&context_x86_64_, - flavor, - state, - state_count, - &thread_state, - &float_state, - &debug_state); + InitializeCPUContextX86_64(&context_x86_64_, + flavor, + state, + state_count, + &thread_state, + &float_state, + &debug_state); #elif defined(ARCH_CPU_ARM64) - InitializeCPUContextARM64(&context_arm64_, - flavor, - state, - state_count, - &thread_state, - &float_state, - &debug_state); + InitializeCPUContextARM64(&context_arm64_, + flavor, + state, + state_count, + &thread_state, + &float_state, + &debug_state); #else #error Port to your CPU architecture #endif - } } } diff --git a/snapshot/ios/process_snapshot_ios_intermediate_dump.cc b/snapshot/ios/process_snapshot_ios_intermediate_dump.cc index 3f9414d9..bf973a3a 100644 --- a/snapshot/ios/process_snapshot_ios_intermediate_dump.cc +++ b/snapshot/ios/process_snapshot_ios_intermediate_dump.cc @@ -53,8 +53,12 @@ bool ProcessSnapshotIOSIntermediateDump::InitializeWithFileInterface( annotations_simple_map_ = annotations; - if (!reader_.Initialize(dump_interface)) { + IOSIntermediateDumpReaderInitializeResult result = + reader_.Initialize(dump_interface); + if (result == IOSIntermediateDumpReaderInitializeResult::kFailure) { return false; + } else if (result == IOSIntermediateDumpReaderInitializeResult::kIncomplete) { + annotations_simple_map_["crashpad_intermediate_dump_incomplete"] = "yes"; } const IOSIntermediateDumpMap* root_map = reader_.RootMap(); diff --git a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc index 910c124d..84f6a7b0 100644 --- a/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc +++ b/snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc @@ -273,7 +273,8 @@ class ProcessSnapshotIOSIntermediateDumpTest : public testing::Test { } } - void WriteMachException(IOSIntermediateDumpWriter* writer) { + void WriteMachException(IOSIntermediateDumpWriter* writer, + bool short_context = false) { IOSIntermediateDumpWriter::ScopedMap machExceptionMap(writer, Key::kMachException); exception_type_t exception = 5; @@ -298,6 +299,10 @@ class ProcessSnapshotIOSIntermediateDumpTest : public testing::Test { EXPECT_TRUE(writer->AddProperty(Key::kException, &exception)); EXPECT_TRUE(writer->AddProperty(Key::kCodes, code, code_count)); EXPECT_TRUE(writer->AddProperty(Key::kFlavor, &flavor)); + + if (short_context) { + state_length -= 10; + } EXPECT_TRUE(writer->AddPropertyBytes( Key::kState, reinterpret_cast(&state), state_length)); uint64_t thread_id = 1; @@ -613,6 +618,24 @@ TEST_F(ProcessSnapshotIOSIntermediateDumpTest, EmptyUncaughtNSExceptionDump) { EXPECT_TRUE(DumpSnapshot(process_snapshot)); } +TEST_F(ProcessSnapshotIOSIntermediateDumpTest, ShortContext) { + { + IOSIntermediateDumpWriter::ScopedRootMap rootMap(writer()); + uint8_t version = 1; + EXPECT_TRUE(writer()->AddProperty(Key::kVersion, &version)); + WriteSystemInfo(writer()); + WriteProcessInfo(writer()); + WriteThreads(writer()); + WriteModules(writer()); + WriteMachException(writer(), true /* short_context=true*/); + } + ProcessSnapshotIOSIntermediateDump process_snapshot; + ASSERT_TRUE(process_snapshot.InitializeWithFilePath(path(), annotations())); + EXPECT_FALSE(IsRegularFile(path())); + EXPECT_TRUE(DumpSnapshot(process_snapshot)); + ExpectSnapshot(process_snapshot); +} + TEST_F(ProcessSnapshotIOSIntermediateDumpTest, FullReport) { { IOSIntermediateDumpWriter::ScopedRootMap rootMap(writer()); @@ -637,6 +660,10 @@ TEST_F(ProcessSnapshotIOSIntermediateDumpTest, FuzzTestCases) { crashpad::internal::ProcessSnapshotIOSIntermediateDump process_snapshot; EXPECT_TRUE(process_snapshot.InitializeWithFilePath(fuzz_path, {})); EXPECT_TRUE(LoggingRemoveFile(path())); + + auto map = process_snapshot.AnnotationsSimpleMap(); + ASSERT_TRUE(map.find("crashpad_intermediate_dump_incomplete") != map.end()); + EXPECT_EQ(map["crashpad_intermediate_dump_incomplete"], "yes"); } } // namespace diff --git a/util/ios/ios_intermediate_dump_reader.cc b/util/ios/ios_intermediate_dump_reader.cc index 0b41eab5..022133bc 100644 --- a/util/ios/ios_intermediate_dump_reader.cc +++ b/util/ios/ios_intermediate_dump_reader.cc @@ -29,24 +29,25 @@ namespace crashpad { namespace internal { -bool IOSIntermediateDumpReader::Initialize( +IOSIntermediateDumpReaderInitializeResult IOSIntermediateDumpReader::Initialize( const IOSIntermediateDumpInterface& dump_interface) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); // Don't initialize empty files. FileOffset size = dump_interface.Size(); if (size == 0) { - return false; + return IOSIntermediateDumpReaderInitializeResult::kFailure; } + IOSIntermediateDumpReaderInitializeResult result = + IOSIntermediateDumpReaderInitializeResult::kSuccess; if (!Parse(dump_interface.FileReader(), size)) { LOG(ERROR) << "Intermediate dump parsing failed"; - // Intentially do not return false here, as it may be possible to extract a - // useful minidump out of a partial intermediate dump. + result = IOSIntermediateDumpReaderInitializeResult::kIncomplete; } INITIALIZATION_STATE_SET_VALID(initialized_); - return true; + return result; } const IOSIntermediateDumpMap* IOSIntermediateDumpReader::RootMap() { diff --git a/util/ios/ios_intermediate_dump_reader.h b/util/ios/ios_intermediate_dump_reader.h index 513d5227..fced1f27 100644 --- a/util/ios/ios_intermediate_dump_reader.h +++ b/util/ios/ios_intermediate_dump_reader.h @@ -22,6 +22,21 @@ namespace crashpad { namespace internal { +//! \brief The return value for IOSIntermediateDumpReader::Initialize. +enum class IOSIntermediateDumpReaderInitializeResult : int { + //! \brief The intermediate dump was read successfully, initialization + //! succeeded. + kSuccess, + + //! \brief The intermediate dump could be loaded, but parsing was incomplete. + //! An attempt to parse the RootMap should still be made, as there may + //! still be valuable information to put into a minidump. + kIncomplete, + + //! \brief The intermediate dump could not be loaded, initialization failed. + kFailure, +}; + //! \brief Open and parse iOS intermediate dumps. class IOSIntermediateDumpReader { public: @@ -43,7 +58,8 @@ class IOSIntermediateDumpReader { //! \return On success, returns `true`, otherwise returns `false`. Clients may //! still attempt to parse RootMap, as partial minidumps may still be //! usable. - bool Initialize(const IOSIntermediateDumpInterface& dump_interface); + IOSIntermediateDumpReaderInitializeResult Initialize( + const IOSIntermediateDumpInterface& dump_interface); //! \brief Returns an IOSIntermediateDumpMap corresponding to the root of the //! intermediate dump. diff --git a/util/ios/ios_intermediate_dump_reader_test.cc b/util/ios/ios_intermediate_dump_reader_test.cc index a223b441..a66d6c3d 100644 --- a/util/ios/ios_intermediate_dump_reader_test.cc +++ b/util/ios/ios_intermediate_dump_reader_test.cc @@ -33,6 +33,7 @@ namespace test { namespace { using Key = internal::IntermediateDumpKey; +using Result = internal::IOSIntermediateDumpReaderInitializeResult; using internal::IOSIntermediateDumpWriter; class IOSIntermediateDumpReaderTest : public testing::Test { @@ -80,7 +81,7 @@ TEST_F(IOSIntermediateDumpReaderTest, ReadNoFile) { TEST_F(IOSIntermediateDumpReaderTest, ReadEmptyFile) { internal::IOSIntermediateDumpReader reader; - EXPECT_FALSE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kFailure); EXPECT_FALSE(IsRegularFile(path())); } @@ -89,7 +90,7 @@ TEST_F(IOSIntermediateDumpReaderTest, ReadHelloWorld) { EXPECT_TRUE( LoggingWriteFile(fd(), hello_world.c_str(), hello_world.length())); internal::IOSIntermediateDumpReader reader; - EXPECT_TRUE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kIncomplete); EXPECT_FALSE(IsRegularFile(path())); const auto root_map = reader.RootMap(); @@ -116,7 +117,7 @@ TEST_F(IOSIntermediateDumpReaderTest, FuzzTestCases) { internal::IOSIntermediateDumpByteArray dump_interface(fuzz1, sizeof(fuzz1)); internal::IOSIntermediateDumpReader reader; - EXPECT_TRUE(reader.Initialize(dump_interface)); + EXPECT_EQ(reader.Initialize(dump_interface), Result::kIncomplete); const auto root_map = reader.RootMap(); EXPECT_TRUE(root_map->empty()); } @@ -135,7 +136,7 @@ TEST_F(IOSIntermediateDumpReaderTest, WriteBadPropertyDataLength) { size_t value_length = 999999; EXPECT_TRUE(LoggingWriteFile(fd(), &value_length, sizeof(size_t))); EXPECT_TRUE(LoggingWriteFile(fd(), &value, sizeof(value))); - EXPECT_TRUE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kIncomplete); EXPECT_FALSE(IsRegularFile(path())); const auto root_map = reader.RootMap(); @@ -158,7 +159,7 @@ TEST_F(IOSIntermediateDumpReaderTest, InvalidArrayInArray) { writer_->AddProperty(Key::kVersion, &version); } EXPECT_TRUE(writer_->Close()); - EXPECT_TRUE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kIncomplete); EXPECT_FALSE(IsRegularFile(path())); const auto root_map = reader.RootMap(); @@ -180,7 +181,7 @@ TEST_F(IOSIntermediateDumpReaderTest, InvalidPropertyInArray) { writer_->AddProperty(Key::kVersion, &version); } EXPECT_TRUE(writer_->Close()); - EXPECT_TRUE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kIncomplete); EXPECT_FALSE(IsRegularFile(path())); const auto root_map = reader.RootMap(); @@ -217,7 +218,7 @@ TEST_F(IOSIntermediateDumpReaderTest, ReadValidData) { } EXPECT_TRUE(writer_->Close()); - EXPECT_TRUE(reader.Initialize(dump_interface())); + EXPECT_EQ(reader.Initialize(dump_interface()), Result::kSuccess); EXPECT_FALSE(IsRegularFile(path())); auto root_map = reader.RootMap();