Object/Minidump: Add support for reading the ModuleList stream

[labath]

Summary

The ModuleList stream consists of an integer giving the number of
entries in the list, followed by the list itself. Each entry in the list
describes a module (dynamically loaded objects which were loaded in the
process when it crashed (or when the minidump was generated).

The code for reading the list is relatively straight-forward, with a
single gotcha. Some minidump writers are emitting padding after the
"count" field in order to align the subsequent list on 8 byte boundary
(this depends on how their ModuleList type was defined and the native
alignment of various types on their platform). Fortunately, the minidump
format contains enough redundancy (in the form of the stream length
field in the stream directory), which allows us to detect this situation
and correct it.

This patch just adds the ability to parse the stream. Code for
conversion to/from yaml will come in a follow-up patch.

[jhenderson]

Code basically looks fine, but somebody else should confirm that the format is correct.

lib/Object/Minidump.cpp

55 ↗

(On Diff #193257)

I wonder whether it would be worth a new class of errors for minidump files? After all, invalid_section_index feels a bit forced for a format without sections!

unittests/Object/MinidumpTest.cpp

317 ↗

(On Diff #193257)

It's a bit jarring seeing both hex and decimal numbers in here at the same time. Is there a particular reason you've used hex here, but decimal for e.g. 116 below?

[labath]

In D60121#1454817, @jhenderson wrote:

Code basically looks fine, but somebody else should confirm that the format is correct.

BTW, the structure definitions are just copied from lldb (https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/minidump/MinidumpTypes.h and other files in that directory). I'm also porting the lldb parsing code to use the new llvm stuff as soon as the llvm stuff is ready as a way to ensure things remain interoperable with the existing minidumps and related tests.

lib/Object/Minidump.cpp

55 ↗

(On Diff #193257)

Yes, I've been wondering about that too. In practice, I don't expect that the consumers will wan't to differentiate the error cases too much here (it usually does not matter if the stream was not present in the file, or if it was truncated -- the end result is the same -- you cannot use it). However, for clarity it might be better to do have a separate type anyway. I'll whip up a separate patch for that.

unittests/Object/MinidumpTest.cpp

317 ↗

(On Diff #193257)

Yeah, I agree this is not ideal. I started with hex here because it looked more "binary", but then I realized that:
a) decimal is shorter
b) the "size" fields are present in the static_asserts as decimal, and so it was easier to copy the numbers from there as decimal

I'll replace all of these to use decimal, except the "version" field, as that is defined in hex in the header.

[labath]

• avoid using mismatched error codes
• use decimal numbers more consistently in the tests

[labath]

lib/Object/Minidump.cpp

55 ↗

(On Diff #193257)

After some soul-searching I decided to just abandon the idea of returning error codes here, and just return the generic parse_failed error. That's what most other Object formats do (presumably they don't have a good use for distinguishing the codes either), and having an own error type would require some non-obvious design choices. (Like, since I'd still inherit from BinaryError, which inherits from ECError, I'd still have to interoperate with std::error_code somehow. This means I'd still have to map things to the object_error enum, or I'd have to define a new error_category, which is a deprecated way of doing things).

[jhenderson]

Okay, no more comments from me.

llvm/trunk/include/llvm/BinaryFormat/Minidump.h

struct SystemInfo {
 
   support::ulittle16_t SuiteMask;
   support::ulittle16_t Reserved;
 
   CPUInfo CPU;
 };
 static_assert(sizeof(SystemInfo) == 56, "");
 
+struct VSFixedFileInfo {
+  support::ulittle32_t Signature;
+  support::ulittle32_t StructVersion;
+  support::ulittle32_t FileVersionHigh;
+  support::ulittle32_t FileVersionLow;
+  support::ulittle32_t ProductVersionHigh;
+  support::ulittle32_t ProductVersionLow;
+  support::ulittle32_t FileFlagsMask;
+  support::ulittle32_t FileFlags;
+  support::ulittle32_t FileOS;
+  support::ulittle32_t FileType;
+  support::ulittle32_t FileSubtype;
+  support::ulittle32_t FileDateHigh;
+  support::ulittle32_t FileDateLow;
+};
+static_assert(sizeof(VSFixedFileInfo) == 52, "");
+
+struct Module {
+  support::ulittle64_t BaseOfImage;
+  support::ulittle32_t SizeOfImage;
+  support::ulittle32_t Checksum;
+  support::ulittle32_t TimeDateStamp;
+  support::ulittle32_t ModuleNameRVA;
+  VSFixedFileInfo VersionInfo;
+  LocationDescriptor CvRecord;
+  LocationDescriptor MiscRecord;
+  support::ulittle64_t Reserved0;
+  support::ulittle64_t Reserved1;
+};
+static_assert(sizeof(Module) == 108, "");
+
 } // namespace minidump
 
 template <> struct DenseMapInfo<minidump::StreamType> {
   static minidump::StreamType getEmptyKey() { return minidump::StreamType(-1); }
 
   static minidump::StreamType getTombstoneKey() {
     return minidump::StreamType(-2);
   }

llvm/trunk/include/llvm/Object/Minidump.h

public:
   /// Returns the contents of the SystemInfo stream, cast to the appropriate
   /// type. An error is returned if the file does not contain this stream, or
   /// the stream is smaller than the size of the SystemInfo structure. The
   /// internal consistency of the stream is not checked in any way.
   Expected<const minidump::SystemInfo &> getSystemInfo() const {
     return getStream<minidump::SystemInfo>(minidump::StreamType::SystemInfo);
   }
 
+  /// Returns the module list embedded in the ModuleList stream. An error is
+  /// returned if the file does not contain this stream, or if the stream is
+  /// not large enough to contain the number of modules declared in the stream
+  /// header. The consistency of the Module entries themselves is not checked in
+  /// any way.
+  Expected<ArrayRef<minidump::Module>> getModuleList() const;
+
 private:
-  static Error createError(StringRef Str,
-                           object_error Err = object_error::parse_failed) {
-    return make_error<GenericBinaryError>(Str, Err);
+  static Error createError(StringRef Str) {
+    return make_error<GenericBinaryError>(Str, object_error::parse_failed);
   }
 
   static Error createEOFError() {
-    return createError("Unexpected EOF", object_error::unexpected_eof);
+    return make_error<GenericBinaryError>("Unexpected EOF",
+                                          object_error::unexpected_eof);
   }
 
   /// Return a slice of the given data array, with bounds checking.
   static Expected<ArrayRef<uint8_t>> getDataSlice(ArrayRef<uint8_t> Data,
                                                   size_t Offset, size_t Size);
 
   /// Return the slice of the given data array as an array of objects of the
   /// given type. The function checks that the input array is large enough to
private:
   DenseMap<minidump::StreamType, std::size_t> StreamMap;
 };
 
 template <typename T>
 Expected<const T &> MinidumpFile::getStream(minidump::StreamType Stream) const {
   if (auto OptionalStream = getRawStream(Stream)) {
     if (OptionalStream->size() >= sizeof(T))
       return *reinterpret_cast<const T *>(OptionalStream->data());
-    return createError("Malformed stream", object_error::unexpected_eof);
+    return createEOFError();
   }
-  return createError("No such stream", object_error::invalid_section_index);
+  return createError("No such stream");
 }
 
 template <typename T>
 Expected<ArrayRef<T>> MinidumpFile::getDataSliceAs(ArrayRef<uint8_t> Data,
                                                    size_t Offset,
                                                    size_t Count) {
   // Check for overflow.
   if (Count > std::numeric_limits<size_t>::max() / sizeof(T))

llvm/trunk/lib/Object/Minidump.cpp

Expected<std::string> MinidumpFile::getString(size_t Offset) const {
 
   std::string Result;
   if (!convertUTF16ToUTF8String(WStr, Result))
     return createError("String decoding failed");
 
   return Result;
 }
 
+Expected<ArrayRef<Module>> MinidumpFile::getModuleList() const {
+  auto OptionalStream = getRawStream(StreamType::ModuleList);
+  if (!OptionalStream)
+    return createError("No such stream");
+  auto ExpectedSize =
+      getDataSliceAs<support::ulittle32_t>(*OptionalStream, 0, 1);
+  if (!ExpectedSize)
+    return ExpectedSize.takeError();
+
+  size_t ListSize = ExpectedSize.get()[0];
+
+  size_t ListOffset = 4;
+  // Some producers insert additional padding bytes to align the module list to
+  // 8-byte boundary. Check for that by comparing the module list size with the
+  // overall stream size.
+  if (ListOffset + sizeof(Module) * ListSize < OptionalStream->size())
+    ListOffset = 8;
+
+  return getDataSliceAs<Module>(*OptionalStream, ListOffset, ListSize);
+}
+
 Expected<ArrayRef<uint8_t>>
 MinidumpFile::getDataSlice(ArrayRef<uint8_t> Data, size_t Offset, size_t Size) {
   // Check for overflow.
   if (Offset + Size < Offset || Offset + Size < Size ||
       Offset + Size > Data.size())
     return createEOFError();
   return Data.slice(Offset, Size);
 }

llvm/trunk/unittests/Object/MinidumpTest.cpp

TEST(MinidumpFile, getString) {
   EXPECT_THAT_EXPECTED(File.getString(62), HasValue(""));
   EXPECT_THAT_EXPECTED(File.getString(68), HasValue("a"));
   EXPECT_THAT_EXPECTED(File.getString(75), HasValue("a"));
 
   // Check the case when the size field does not fit into the remaining data.
   EXPECT_THAT_EXPECTED(File.getString(ManyStrings.size() - 2),
                        Failed<BinaryError>());
 }
+
+TEST(MinidumpFile, getModuleList) {
+  std::vector<uint8_t> OneModule{
+      // Header
+      'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+      1, 0, 0, 0,                           // NumberOfStreams,
+      32, 0, 0, 0,                          // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
+      0, 0, 0, 0, 0, 0, 0, 0,               // Flags
+                                            // Stream Directory
+      4, 0, 0, 0, 112, 0, 0, 0,             // Type, DataSize,
+      44, 0, 0, 0,                          // RVA
+      // ModuleList
+      1, 0, 0, 0,             // NumberOfModules
+      1, 2, 3, 4, 5, 6, 7, 8, // BaseOfImage
+      9, 0, 1, 2, 3, 4, 5, 6, // SizeOfImage, Checksum
+      7, 8, 9, 0, 1, 2, 3, 4, // TimeDateStamp, ModuleNameRVA
+      0, 0, 0, 0, 0, 0, 0, 0, // Signature, StructVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // ProductVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileFlagsMask, FileFlags
+      0, 0, 0, 0,             // FileOS
+      0, 0, 0, 0, 0, 0, 0, 0, // FileType, FileSubType
+      0, 0, 0, 0, 0, 0, 0, 0, // FileDate
+      1, 2, 3, 4, 5, 6, 7, 8, // CvRecord
+      9, 0, 1, 2, 3, 4, 5, 6, // MiscRecord
+      7, 8, 9, 0, 1, 2, 3, 4, // Reserved0
+      5, 6, 7, 8, 9, 0, 1, 2, // Reserved1
+  };
+  // Same as before, but with a padded module list.
+  std::vector<uint8_t> PaddedModule{
+      // Header
+      'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+      1, 0, 0, 0,                           // NumberOfStreams,
+      32, 0, 0, 0,                          // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
+      0, 0, 0, 0, 0, 0, 0, 0,               // Flags
+                                            // Stream Directory
+      4, 0, 0, 0, 116, 0, 0, 0,             // Type, DataSize,
+      44, 0, 0, 0,                          // RVA
+      // ModuleList
+      1, 0, 0, 0,             // NumberOfModules
+      0, 0, 0, 0,             // Padding
+      1, 2, 3, 4, 5, 6, 7, 8, // BaseOfImage
+      9, 0, 1, 2, 3, 4, 5, 6, // SizeOfImage, Checksum
+      7, 8, 9, 0, 1, 2, 3, 4, // TimeDateStamp, ModuleNameRVA
+      0, 0, 0, 0, 0, 0, 0, 0, // Signature, StructVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // ProductVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileFlagsMask, FileFlags
+      0, 0, 0, 0,             // FileOS
+      0, 0, 0, 0, 0, 0, 0, 0, // FileType, FileSubType
+      0, 0, 0, 0, 0, 0, 0, 0, // FileDate
+      1, 2, 3, 4, 5, 6, 7, 8, // CvRecord
+      9, 0, 1, 2, 3, 4, 5, 6, // MiscRecord
+      7, 8, 9, 0, 1, 2, 3, 4, // Reserved0
+      5, 6, 7, 8, 9, 0, 1, 2, // Reserved1
+  };
+
+  for (const std::vector<uint8_t> &Data : {OneModule, PaddedModule}) {
+    auto ExpectedFile = create(Data);
+    ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+    const MinidumpFile &File = **ExpectedFile;
+    Expected<ArrayRef<Module>> ExpectedModule = File.getModuleList();
+    ASSERT_THAT_EXPECTED(ExpectedModule, Succeeded());
+    ASSERT_EQ(1u, ExpectedModule->size());
+    const Module &M = ExpectedModule.get()[0];
+    EXPECT_EQ(0x0807060504030201u, M.BaseOfImage);
+    EXPECT_EQ(0x02010009u, M.SizeOfImage);
+    EXPECT_EQ(0x06050403u, M.Checksum);
+    EXPECT_EQ(0x00090807u, M.TimeDateStamp);
+    EXPECT_EQ(0x04030201u, M.ModuleNameRVA);
+    EXPECT_EQ(0x04030201u, M.CvRecord.DataSize);
+    EXPECT_EQ(0x08070605u, M.CvRecord.RVA);
+    EXPECT_EQ(0x02010009u, M.MiscRecord.DataSize);
+    EXPECT_EQ(0x06050403u, M.MiscRecord.RVA);
+    EXPECT_EQ(0x0403020100090807u, M.Reserved0);
+    EXPECT_EQ(0x0201000908070605u, M.Reserved1);
+  }
+
+  std::vector<uint8_t> StreamTooShort{
+      // Header
+      'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+      1, 0, 0, 0,                           // NumberOfStreams,
+      32, 0, 0, 0,                          // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
+      0, 0, 0, 0, 0, 0, 0, 0,               // Flags
+                                            // Stream Directory
+      4, 0, 0, 0, 111, 0, 0, 0,             // Type, DataSize,
+      44, 0, 0, 0,                          // RVA
+      // ModuleList
+      1, 0, 0, 0,             // NumberOfModules
+      1, 2, 3, 4, 5, 6, 7, 8, // BaseOfImage
+      9, 0, 1, 2, 3, 4, 5, 6, // SizeOfImage, Checksum
+      7, 8, 9, 0, 1, 2, 3, 4, // TimeDateStamp, ModuleNameRVA
+      0, 0, 0, 0, 0, 0, 0, 0, // Signature, StructVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // ProductVersion
+      0, 0, 0, 0, 0, 0, 0, 0, // FileFlagsMask, FileFlags
+      0, 0, 0, 0,             // FileOS
+      0, 0, 0, 0, 0, 0, 0, 0, // FileType, FileSubType
+      0, 0, 0, 0, 0, 0, 0, 0, // FileDate
+      1, 2, 3, 4, 5, 6, 7, 8, // CvRecord
+      9, 0, 1, 2, 3, 4, 5, 6, // MiscRecord
+      7, 8, 9, 0, 1, 2, 3, 4, // Reserved0
+      5, 6, 7, 8, 9, 0, 1, 2, // Reserved1
+  };
+  auto ExpectedFile = create(StreamTooShort);
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  const MinidumpFile &File = **ExpectedFile;
+  EXPECT_THAT_EXPECTED(File.getModuleList(), Failed<BinaryError>());
+}