Restructure the minidump loading path and add early & explicit consistency checks

[lemo]

Summary

Corrupted minidumps was leading to unpredictable behavior.

This change adds explicit consistency checks for the minidump early on. The checks are not comprehensive but they should catch obvious structural violations:

• streams with type == 0
• duplicate streams (same type)
• overlapping streams
• truncated minidumps

Another early check is to make sure we actually support the minidump architecture instead of crashing at a random place deep inside LLDB.

[amccarth]

source/Plugins/Process/minidump/MinidumpParser.cpp

23 ↗

(On Diff #155049)

Why add <unordered_set>? It looks like your new map is just a vector.

367 ↗

(On Diff #155049)

This looks like an inadvertent change. Please change "i" back to "index".

454 ↗

(On Diff #155049)

For consistency, please consider renaming result to error.

558 ↗

(On Diff #155049)

This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), but the comments here are really good, so I'm OK with it.

source/Plugins/Process/minidump/MinidumpParser.h

93 ↗

(On Diff #155049)

I'm not a big fan of 2-step initialization, but that seems to be the way of LLDB. :-(

source/Plugins/Process/minidump/ProcessMinidump.cpp

172 ↗

(On Diff #155049)

Should the architecture check be in the MinidumpParser::Initialize with the other checks?

I don't know the answer; I'm just asking for your thinking about this.

193 ↗

(On Diff #155049)

I realize nothing is perfectly consistent, but I think LLVM tends to start error strings with a lowercase letter (except for proper nouns). Can you revert this case change and make sure your new error strings follow that pattern?

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

I would rather see this function return the result of the Initialize and let the individual tests check the expectation explicitly.

I know that will lead to a little bit of duplication in the tests, but it will make the individual tests easier to understand in isolation. It also makes it possible for each test to decide whether to ASSERT or EXPECT. And it eliminates the need for the bool parameter which is hard to decipher at the call sites.

80 ↗

(On Diff #155049)

See comment on SetUpData. As much as practical, I'd rather have the EXPECTs and ASSERTs directly in the tests rather than hiding them in a helper function. Also note that, while the first two arguments are pretty intuitive, there's no way to understand the true/false in the third argument without going to look up to see what SetUpData does.

[lemo]

Incorporating CR feedback

source/Plugins/Process/minidump/MinidumpParser.cpp

23 ↗

(On Diff #155049)

Good catch

558 ↗

(On Diff #155049)

I agree size-wise it's borderline. It's also a bit smelly in that it does two things (checks + initialization).

I originally had a separate "ConsistencyCheck", but there was significant duplication (the directory traversal) and the combined version won by a tiny margin :) If this grows with more checks I'd be happy to revisit and refactor it.

source/Plugins/Process/minidump/MinidumpParser.h

93 ↗

(On Diff #155049)

Agreed. I don't see how to avoid this w/o much bigger changes though (we don't have a chance to return meaningful errors during process creation).

source/Plugins/Process/minidump/ProcessMinidump.cpp

172 ↗

(On Diff #155049)

Good question, here's my take: the checks are for consistency and a minidump with a currently unsupported architecture is a valid minidump.

So I think it's better to have this check external since the architecture support is not a minidump parser concern. WDYT?

193 ↗

(On Diff #155049)

Sigh, sure. I was hoping it's the other way around.

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

Good idea, although gunit doesn't let us ASSERT in non-void returning functions.

I agree that the bool parameter is ugly, so I created a separate TruncateMinidump() helper (which cleans up the SetUpData since the load_size was only used for truncation)

[amccarth]

Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap). Is it difficult to create such malformed minidumps?

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

This isn't quit what I meant. I'd rather not have the ASSERTs in helper functions at all (regardless of return type). The helpers should return a status and the actual test code should do whatever ASSERT or EXPECT is appropriate.

[amccarth]

source/Plugins/Process/minidump/ProcessMinidump.cpp

172 ↗

(On Diff #155049)

Yes, this makes sense to me.

[lemo]

Regarding test for the other checks, I'll try to fabricate a few invalid minidumps (although it would obviously provide limited coverage)

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

So how would we handle the existing checks in SetupData()?

[lemo]

Adding a few ill-formed minidumps for testing the new checks

[amccarth]

LGTM if you don't want to consider my remaining suggestion for this patch.

Thanks for the extra tests.

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

SetUpData would just return an error status instead of ASSERTing. The actual ASSERTs would be placed in the tests that call SetUpData. As I said, that would add some duplication, because individual tests would have to ASSERT (or EXPECT) on the result of SetUpData, but it makes the tests easier to read and maintain the tests.

Since there are other tests using SetUpData, they would have to be updated, so maybe you want to declare this suggestion as out-of-scope for this patch. Anyway, I'm happy that you eliminated the boolean argument.

[lemo]

Thanks Adrian for the thorough review.

unittests/Process/minidump/MinidumpParserTest.cpp

52 ↗

(On Diff #155049)

I think I understand the idea and I agree it's tempting. The problem is that not all the checks in SetupData() are status based, so there's no direct mapping from each operation to a return value.

It doesn't seem terribly bad to let the helper encapsulate some of the checks.

[labath]

I don't agree with the two-stage initialization of the MinidumpParser class being introduced here. We deliberately introduced the Create static function to avoid this. If this Initialize function in checking invariants which are assumed to be hold by other parser methods, then it should be done by the Create function. Ideally this would be done before even constructing the parser object, but if this is impractical for some reason then you can make the Initialize function private and call it directly from Create. This way a user will never be able to see an malformed parser object. To make sure you propagate the error, you can change the return type of Create to llvm::Expected<MinidumpParser (the only reason we did not do this back then was that there was no precedent for using Expected in LLDB, but this is no longer the case).

[lemo]

The problem is not returning an error from Minidump::Create() - if that was
the case this could easily be improved indeed. The two-phase initialization
is a consequence of the LLDB plugin lookup:

1. Target::CreateProcess() calls Process::FindPlugin()
2. ProcessMinidump::CreateInstance() then has to inspect the core file to

see if it's a minidump
2b. ... if it is a minidump, we need to create a ProcessMinidump (which
calls MinidumpParser::Create())

3. once the plugin is selected, Process::LoadCore() is finally called and

this the earliest we can do minidump-specific error checking

Note that at step 2b. we don't have a way to propagate the error since
we're just doing the plugin lookup (the most we can pass on the lookup to
the rest of the plugins). We can't easily defer the
MinidumpParser::Create() as step 2b either since that only morphs into a
different kind of two-stage initialization (having a ProcessMinidump w/o a
parser).

I agree that it would be nicer with a one step initialization but overall
changing the LLDB plugin lookup is too intrusive for the relatively small
benefit. If you have any suggestions I'd love to hear them.

[clayborg]

I have an upcoming patch for adding ARM and ARM64 support to the minidump parser and i was curious how you created the invalid minidump files that are part of this patch?

[lemo]

Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Regarding the invalid minidumps, I used my favorite hex editor
https://www.sweetscape.com/010editor/ to manually corrupt the minidump
streams directory.

[clayborg]

In D49202#1165455, @lemo wrote:

Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Currently I am adding support for the Breakpad ARM and ARM64 architectures.

Regarding the invalid minidumps, I used my favorite hex editor
https://www.sweetscape.com/010editor/ to manually corrupt the minidump
streams directory.

Gotcha. I have a minidump generator I am writing as part of a python module that can save minidumps from any live process so I will continue to use that.

lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h

public:
   const MinidumpExceptionStream *GetExceptionStream();
 
   llvm::Optional<Range> FindMemoryRange(lldb::addr_t addr);
 
   llvm::ArrayRef<uint8_t> GetMemory(lldb::addr_t addr, size_t size);
 
   llvm::Optional<MemoryRegionInfo> GetMemoryRegionInfo(lldb::addr_t);
 
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
+private:
+  MinidumpParser(const lldb::DataBufferSP &data_buf_sp);
+
 private:
   lldb::DataBufferSP m_data_sp;
   llvm::DenseMap<uint32_t, MinidumpLocationDescriptor> m_directory_map;
-
-  MinidumpParser(
-      const lldb::DataBufferSP &data_buf_sp,
-      llvm::DenseMap<uint32_t, MinidumpLocationDescriptor> &&directory_map);
 };
 
 } // end namespace minidump
 } // end namespace lldb_private
 #endif // liblldb_MinidumpParser_h_

lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp

 //===-- MinidumpParser.cpp ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Project includes
 #include "MinidumpParser.h"
 #include "NtStructures.h"
 #include "RegisterContextMinidump_x86_32.h"
 
 // Other libraries and framework includes
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Utility/LLDBAssert.h"
 
 // C includes
 // C++ includes
+#include <algorithm>
 #include <map>
+#include <vector>
 
 using namespace lldb_private;
 using namespace minidump;
 
 llvm::Optional<MinidumpParser>
 MinidumpParser::Create(const lldb::DataBufferSP &data_buf_sp) {
   if (data_buf_sp->GetByteSize() < sizeof(MinidumpHeader)) {
     return llvm::None;
   }
-
-  llvm::ArrayRef<uint8_t> header_data(data_buf_sp->GetBytes(),
-                                      sizeof(MinidumpHeader));
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
-
-  if (header == nullptr) {
-    return llvm::None;
-  }
-
-  lldb::offset_t directory_list_offset = header->stream_directory_rva;
-  // check if there is enough data for the parsing of the directory list
-  if ((directory_list_offset +
-       sizeof(MinidumpDirectory) * header->streams_count) >
-      data_buf_sp->GetByteSize()) {
-    return llvm::None;
-  }
+  return MinidumpParser(data_buf_sp);
 
-  const MinidumpDirectory *directory = nullptr;
-  Status error;
-  llvm::ArrayRef<uint8_t> directory_data(
-      data_buf_sp->GetBytes() + directory_list_offset,
-      sizeof(MinidumpDirectory) * header->streams_count);
-  llvm::DenseMap<uint32_t, MinidumpLocationDescriptor> directory_map;
-
-  for (uint32_t i = 0; i < header->streams_count; ++i) {
-    error = consumeObject(directory_data, directory);
-    if (error.Fail()) {
-      return llvm::None;
-    }
-    directory_map[static_cast<const uint32_t>(directory->stream_type)] =
-        directory->location;
-  }
-
-  return MinidumpParser(data_buf_sp, std::move(directory_map));
-}
-
-MinidumpParser::MinidumpParser(
-    const lldb::DataBufferSP &data_buf_sp,
-    llvm::DenseMap<uint32_t, MinidumpLocationDescriptor> &&directory_map)
-    : m_data_sp(data_buf_sp), m_directory_map(directory_map) {}
+MinidumpParser::MinidumpParser(const lldb::DataBufferSP &data_buf_sp)
+    : m_data_sp(data_buf_sp) {}
 
 llvm::ArrayRef<uint8_t> MinidumpParser::GetData() {
   return llvm::ArrayRef<uint8_t>(m_data_sp->GetBytes(),
                                  m_data_sp->GetByteSize());
 }
 
 llvm::ArrayRef<uint8_t>
 MinidumpParser::GetStream(MinidumpStreamType stream_type) {
MinidumpParser::GetMemoryRegionInfo(lldb::addr_t load_addr) {
   info.SetExecutable(no);
   info.SetMapped(no);
 
   // Note that the memory info list doesn't seem to contain ranges in kernel
   // space, so if you're walking a stack that has kernel frames, the stack may
   // appear truncated.
   return info;
 }
+
+Status MinidumpParser::Initialize() {
+  Status error;
+
+  lldbassert(m_directory_map.empty());
+
+  llvm::ArrayRef<uint8_t> header_data(m_data_sp->GetBytes(),
+                                      sizeof(MinidumpHeader));
+  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  if (header == nullptr) {
+    error.SetErrorString("invalid minidump: can't parse the header");
+    return error;
+  }
+
+  // A minidump without at least one stream is clearly ill-formed
+  if (header->streams_count == 0) {
+    error.SetErrorString("invalid minidump: no streams present");
+    return error;
+  }
+
+  struct FileRange {
+    uint32_t offset = 0;
+    uint32_t size = 0;
+
+    FileRange(uint32_t offset, uint32_t size) : offset(offset), size(size) {}
+    uint32_t end() const { return offset + size; }
+  };
+
+  const uint32_t file_size = m_data_sp->GetByteSize();
+
+  // Build a global minidump file map, checking for:
+  // - overlapping streams/data structures
+  // - truncation (streams pointing past the end of file)
+  std::vector<FileRange> minidump_map;
+
+  // Add the minidump header to the file map
+  if (sizeof(MinidumpHeader) > file_size) {
+    error.SetErrorString("invalid minidump: truncated header");
+    return error;
+  }
+  minidump_map.emplace_back( 0, sizeof(MinidumpHeader) );
+
+  // Add the directory entries to the file map
+  FileRange directory_range(header->stream_directory_rva,
+                            header->streams_count *
+                                sizeof(MinidumpDirectory));
+  if (directory_range.end() > file_size) {
+    error.SetErrorString("invalid minidump: truncated streams directory");
+    return error;
+  }
+  minidump_map.push_back(directory_range);
+
+  // Parse stream directory entries
+  llvm::ArrayRef<uint8_t> directory_data(
+      m_data_sp->GetBytes() + directory_range.offset, directory_range.size);
+  for (uint32_t i = 0; i < header->streams_count; ++i) {
+    const MinidumpDirectory *directory_entry = nullptr;
+    error = consumeObject(directory_data, directory_entry);
+    if (error.Fail())
+      return error;
+    if (directory_entry->stream_type == 0) {
+      // Ignore dummy streams (technically ill-formed, but a number of
+      // existing minidumps seem to contain such streams)
+      if (directory_entry->location.data_size == 0)
+        continue;
+      error.SetErrorString("invalid minidump: bad stream type");
+      return error;
+    }
+    // Update the streams map, checking for duplicate stream types
+    if (!m_directory_map
+             .insert({directory_entry->stream_type, directory_entry->location})
+             .second) {
+      error.SetErrorString("invalid minidump: duplicate stream type");
+      return error;
+    }
+    // Ignore the zero-length streams for layout checks
+    if (directory_entry->location.data_size != 0) {
+      minidump_map.emplace_back(directory_entry->location.rva,
+                                directory_entry->location.data_size);
+    }
+  }
+
+  // Sort the file map ranges by start offset
+  std::sort(minidump_map.begin(), minidump_map.end(),
+            [](const FileRange &a, const FileRange &b) {
+              return a.offset < b.offset;
+            });
+
+  // Check for overlapping streams/data structures
+  for (size_t i = 1; i < minidump_map.size(); ++i) {
+    const auto &prev_range = minidump_map[i - 1];
+    if (prev_range.end() > minidump_map[i].offset) {
+      error.SetErrorString("invalid minidump: overlapping streams");
+      return error;
+    }
+  }
+
+  // Check for streams past the end of file
+  const auto &last_range = minidump_map.back();
+  if (last_range.end() > file_size) {
+    error.SetErrorString("invalid minidump: truncated stream");
+    return error;
+  }
+
+  return error;
+}

lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp

const MinidumpHeaderConstants version =
       static_cast<const MinidumpHeaderConstants>(
           static_cast<const uint32_t>(header->version) & 0x0000ffff);
   // the high 16 bits of the version field are implementation specific
 
   if (error.Fail() || signature != MinidumpHeaderConstants::Signature ||
       version != MinidumpHeaderConstants::Version)
     return nullptr;
 
-  // TODO check for max number of streams ?
-  // TODO more sanity checks ?
-
   return header;
 }
 
 // Minidump string
 llvm::Optional<std::string>
 lldb_private::minidump::parseMinidumpString(llvm::ArrayRef<uint8_t> &data) {
   std::string result;
 

lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

lldb::ProcessSP ProcessMinidump::CreateInstance(lldb::TargetSP target_sp,
   lldb::ProcessSP process_sp;
   // Read enough data for the Minidump header
   constexpr size_t header_size = sizeof(MinidumpHeader);
   auto DataPtr =
       DataBufferLLVM::CreateSliceFromPath(crash_file->GetPath(), header_size, 0);
   if (!DataPtr)
     return nullptr;
 
-  assert(DataPtr->GetByteSize() == header_size);
+  lldbassert(DataPtr->GetByteSize() == header_size);
 
   // first, only try to parse the header, beacuse we need to be fast
   llvm::ArrayRef<uint8_t> HeaderBytes = DataPtr->GetData();
   const MinidumpHeader *header = MinidumpHeader::Parse(HeaderBytes);
   if (header == nullptr)
     return nullptr;
 
   auto AllData = DataBufferLLVM::CreateSliceFromPath(crash_file->GetPath(), -1, 0);

 
 void ProcessMinidump::Terminate() {
   PluginManager::UnregisterPlugin(ProcessMinidump::CreateInstance);
 }
 
 Status ProcessMinidump::DoLoadCore() {
   Status error;
 
+  // Minidump parser initialization & consistency checks
+  error = m_minidump_parser.Initialize();
+  if (error.Fail())
+    return error;
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
+  switch (arch.GetMachine()) {
+  case llvm::Triple::x86:
+  case llvm::Triple::x86_64:
+    // supported
+    break;
+
+  default:
+    error.SetErrorStringWithFormat("unsupported minidump architecture: %s",
+                                   arch.GetArchitectureName());
+    return error;
+  }
+
   m_thread_list = m_minidump_parser.GetThreads();
   m_active_exception = m_minidump_parser.GetExceptionStream();
   ReadModuleList();
-  GetTarget().SetArchitecture(GetArchitecture());
+  GetTarget().SetArchitecture(arch);
 
   llvm::Optional<lldb::pid_t> pid = m_minidump_parser.GetPid();
   if (!pid) {
     error.SetErrorString("failed to parse PID");
     return error;
   }
   SetID(pid.getValue());
 

lldb/trunk/unittests/Process/minidump/CMakeLists.txt

LINK_COMPONENTS
     Support
   )
 
 set(test_inputs
    linux-i386.dmp
    linux-x86_64.dmp
    linux-x86_64_not_crashed.dmp
    fizzbuzz_no_heap.dmp
-   fizzbuzz_wow64.dmp)
+   fizzbuzz_wow64.dmp
+   bad_duplicate_streams.dmp
+   bad_overlapping_streams.dmp)
 
 add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")

lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

 // C++ includes
 #include <memory>
 
 using namespace lldb_private;
 using namespace minidump;
 
 class MinidumpParserTest : public testing::Test {
 public:
-  void SetUpData(const char *minidump_filename,
-                 uint64_t load_size = UINT64_MAX) {
+  void SetUpData(const char *minidump_filename) {
     std::string filename = GetInputFilePath(minidump_filename);
-    auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+    auto BufferPtr = DataBufferLLVM::CreateSliceFromPath(filename, -1, 0);
+    ASSERT_NE(BufferPtr, nullptr);
+    llvm::Optional<MinidumpParser> optional_parser =
+        MinidumpParser::Create(BufferPtr);
+    ASSERT_TRUE(optional_parser.hasValue());
+    parser.reset(new MinidumpParser(optional_parser.getValue()));
+    ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    ASSERT_TRUE(result.Success()) << result.AsCString();
+  }
+
+  void InvalidMinidump(const char *minidump_filename, uint64_t load_size) {
+    std::string filename = GetInputFilePath(minidump_filename);
+    auto BufferPtr =
+        DataBufferLLVM::CreateSliceFromPath(filename, load_size, 0);
+    ASSERT_NE(BufferPtr, nullptr);
 
     llvm::Optional<MinidumpParser> optional_parser =
         MinidumpParser::Create(BufferPtr);
     ASSERT_TRUE(optional_parser.hasValue());
     parser.reset(new MinidumpParser(optional_parser.getValue()));
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    ASSERT_TRUE(result.Fail());
   }
 
   std::unique_ptr<MinidumpParser> parser;
 };
 
 TEST_F(MinidumpParserTest, GetThreadsAndGetThreadContext) {
   SetUpData("linux-x86_64.dmp");
   llvm::ArrayRef<MinidumpThread> thread_list;
 
   thread_list = parser->GetThreads();
   ASSERT_EQ(1UL, thread_list.size());
 
   const MinidumpThread thread = thread_list[0];
 
   EXPECT_EQ(16001UL, thread.thread_id);
 
   llvm::ArrayRef<uint8_t> context = parser->GetThreadContext(thread);
   EXPECT_EQ(1232UL, context.size());
 }
 
-TEST_F(MinidumpParserTest, GetThreadsTruncatedFile) {
-  SetUpData("linux-x86_64.dmp", 200);
-  llvm::ArrayRef<MinidumpThread> thread_list;
+TEST_F(MinidumpParserTest, TruncatedMinidumps) {
+  InvalidMinidump("linux-x86_64.dmp", 32);
+  InvalidMinidump("linux-x86_64.dmp", 100);
+  InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
+}
 
-  thread_list = parser->GetThreads();
-  ASSERT_EQ(0UL, thread_list.size());
+TEST_F(MinidumpParserTest, IllFormedMinidumps) {
+  InvalidMinidump("bad_duplicate_streams.dmp", -1);
+  InvalidMinidump("bad_overlapping_streams.dmp", -1);
 }
 
 TEST_F(MinidumpParserTest, GetArchitecture) {
   SetUpData("linux-x86_64.dmp");
   ASSERT_EQ(llvm::Triple::ArchType::x86_64,
             parser->GetArchitecture().GetMachine());
   ASSERT_EQ(llvm::Triple::OSType::Linux,
             parser->GetArchitecture().GetTriple().getOS());