ios: Handle partial intermediate dumps and exception contexts.

Testing in beta has shown a few examples of a cropped intermediate dump
still providing useful information, but due to the order intermediate
dump data is written, could be improved.

- Change the order of writing data to the intermediate dump by
increasing the priority of the Exception block from:
 Header / Process / System / Threads/ Modules / Exception
to
 Header / Process / System / Exception / Threads / Modules

- Annotate minidump reports generated from incomplete intermediate
dumps with the key 'crashpad_intermediate_dump_incomplete'.

- Handle partial exception contexts rather than throwing them away.

Change-Id: I543c1d3135c42e5b8e339e498ea0c86002f37ea3
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3294862
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/ios_handler/in_process_handler.cc

@@ -308,12 +308,21 @@ InProcessHandler::ScopedReport::ScopedReport(
     const std::map<std::string, std::string>& 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() {

client/ios_handler/in_process_handler.h

@@ -159,11 +159,14 @@ class InProcessHandler {
                  const std::map<std::string, std::string>& 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_;
   };
 

snapshot/ios/exception_snapshot_ios_intermediate_dump.cc

@@ -293,16 +293,21 @@ void ExceptionSnapshotIOSIntermediateDump::LoadContextFromThread(
     const IOSIntermediateDumpData* state_dump =
         GetDataFromMap(exception_data, Key::kState);
     if (state_dump) {
-      const std::vector<uint8_t>& bytes = state_dump->bytes();
+      std::vector<uint8_t> 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
+      if (actual_length < expected_length) {
-      // shorter than expected_length, and tolerating actual_length longer than
+        // Zero out bytes if actual_length is shorter than expected_length.
-      // expected_length.
+        bytes.resize(expected_length, 0);
-      if (expected_length == actual_length) {
+        actual_length = bytes.size();
+        LOG(WARNING) << "Exception context length " << actual_length
+                     << " shorter than expected length " << expected_length;
+      }
       const ConstThreadState state =
           reinterpret_cast<const ConstThreadState>(bytes.data());
-        mach_msg_type_number_t state_count = bytes.size() / sizeof(uint32_t);
+      // 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,
@@ -324,7 +329,6 @@ void ExceptionSnapshotIOSIntermediateDump::LoadContextFromThread(
 #endif
     }
   }
-  }
 
   // Normally, for EXC_BAD_ACCESS exceptions, the exception address is present
   // in code[1]. It may or may not be the instruction pointer address (usually

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();

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<const void*>(&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

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
+    result = IOSIntermediateDumpReaderInitializeResult::kIncomplete;
-    // useful minidump out of a partial intermediate dump.
   }
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
-  return true;
+  return result;
 }
 
 const IOSIntermediateDumpMap* IOSIntermediateDumpReader::RootMap() {

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.

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();