Minidump: Add support for reading/writing strings

[labath]

Summary

Strings in minidump files are stored as a 32-bit length field, giving
the length of the string in *bytes*, which is followed by the
appropriate number of UTF16 code units. The string is also supposed to
be null-terminated, and the null-terminator is not a part of the length
field. This patch:

• adds support for reading these strings out of the minidump file (this implementation does not depend on proper null-termination)
• adds support for writing them to a minidump file
• using the previous two pieces implements proper (de)serialization of the CSDVersion field of the SystemInfo stream. Previously, this was only read/written as hex, and no attempt was made to access the referenced string -- now this string is read and written correctly.

The changes are tested via yaml2obj|obj2yaml round-trip as well as a
unit test which checks the corner cases of the string deserialization
logic.

[clayborg]

Looks fine to me, but probably need a LLVM specific person for the final ok

[labath]

In D59775#1442987, @clayborg wrote:

Looks fine to me, but probably need a LLVM specific person for the final ok

Maybe @zturner or @amccarth could be the LLVM person ? :)

[jhenderson]

include/llvm/ObjectYAML/MinidumpYAML.h

70 ↗

(On Diff #192121)

Don't know about lifetime here, but could this be a StringRef?

unittests/Object/MinidumpTest.cpp

270 ↗

(On Diff #192121)

What about a case where the size field cannot fit in the remaining data?

[labath]

include/llvm/ObjectYAML/MinidumpYAML.h

70 ↗

(On Diff #192121)

A StringRef would work when constructing this object from YAML (as there the yaml file can provide backing storage). However, when constructing this from a Minidump binary, that would not work, as the data is stored there in the utf16 form (so not the kind of "string" we usually work with).

It would probably be possible to pull of some kind of a trick like yaml::BinaryRef does, where this would be a special object, which could be backed either by a real (utf8) string, or by the minidump utf16 thingy. However, I don't think that would be worth the trouble, as these strings are fairly small. (Also, in order to validate the utf16 data (so I don't end up creating an invalid object and crash during serialization), I'd have to essentially decode the data into a std::string anyway and then throw the result away.)

unittests/Object/MinidumpTest.cpp

270 ↗

(On Diff #192121)

Ah yes, good catch. I'll add that.

[labath]

add the size-does-not-fit test case

[jhenderson]

LGTM.

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

public:
   ArrayRef<uint8_t> getRawStream(const minidump::Directory &Stream) const {
     return getData().slice(Stream.Location.RVA, Stream.Location.DataSize);
   }
 
   /// Returns the raw contents of the stream of the given type, or None if the
   /// file does not contain a stream of this type.
   Optional<ArrayRef<uint8_t>> getRawStream(minidump::StreamType Type) const;
 
+  /// Returns the minidump string at the given offset. An error is returned if
+  /// we fail to parse the string, or the string is invalid UTF16.
+  Expected<std::string> getString(size_t Offset) const;
+
   /// 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);
   }
 

llvm/trunk/include/llvm/ObjectYAML/MinidumpYAML.h

struct RawContentStream : public Stream {
   static bool classof(const Stream *S) {
     return S->Kind == StreamKind::RawContent;
   }
 };
 
 /// SystemInfo minidump stream.
 struct SystemInfoStream : public Stream {
   minidump::SystemInfo Info;
+  std::string CSDVersion;
 
-  explicit SystemInfoStream(const minidump::SystemInfo &Info)
+  explicit SystemInfoStream(const minidump::SystemInfo &Info,
+                            std::string CSDVersion)
       : Stream(StreamKind::SystemInfo, minidump::StreamType::SystemInfo),
-        Info(Info) {}
+        Info(Info), CSDVersion(std::move(CSDVersion)) {}
 
   SystemInfoStream()
       : Stream(StreamKind::SystemInfo, minidump::StreamType::SystemInfo) {
     memset(&Info, 0, sizeof(Info));
   }
 
   static bool classof(const Stream *S) {
     return S->Kind == StreamKind::SystemInfo;

llvm/trunk/lib/Object/Minidump.cpp

 //===- Minidump.cpp - Minidump object file implementation -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/Minidump.h"
 #include "llvm/Object/Error.h"
+#include "llvm/Support/ConvertUTF.h"
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::minidump;
 
 Optional<ArrayRef<uint8_t>>
 MinidumpFile::getRawStream(minidump::StreamType Type) const {
   auto It = StreamMap.find(Type);
   if (It != StreamMap.end())
     return getRawStream(Streams[It->second]);
   return None;
 }
 
+Expected<std::string> MinidumpFile::getString(size_t Offset) const {
+  // Minidump strings consist of a 32-bit length field, which gives the size of
+  // the string in *bytes*. This is followed by the actual string encoded in
+  // UTF16.
+  auto ExpectedSize =
+      getDataSliceAs<support::ulittle32_t>(getData(), Offset, 1);
+  if (!ExpectedSize)
+    return ExpectedSize.takeError();
+  size_t Size = (*ExpectedSize)[0];
+  if (Size % 2 != 0)
+    return createError("String size not even");
+  Size /= 2;
+  if (Size == 0)
+    return "";
+
+  Offset += sizeof(support::ulittle32_t);
+  auto ExpectedData = getDataSliceAs<UTF16>(getData(), Offset, Size);
+  if (!ExpectedData)
+    return ExpectedData.takeError();
+
+  std::string Result;
+  if (!convertUTF16ToUTF8String(*ExpectedData, Result))
+    return createError("String decoding failed");
+
+  return Result;
+}
+
 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/lib/ObjectYAML/MinidumpYAML.cpp

 //===- MinidumpYAML.cpp - Minidump YAMLIO implementation ------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ObjectYAML/MinidumpYAML.h"
+#include "llvm/Support/ConvertUTF.h"
 
 using namespace llvm;
 using namespace llvm::MinidumpYAML;
 using namespace llvm::minidump;
 
 namespace {
 class BlobAllocator {
 public:
template <typename T> size_t allocateArray(ArrayRef<T> Data) {
     return allocateBytes({reinterpret_cast<const uint8_t *>(Data.data()),
                           sizeof(T) * Data.size()});
   }
 
   template <typename T> size_t allocateObject(const T &Data) {
     return allocateArray(makeArrayRef(Data));
   }
 
+  size_t allocateString(StringRef Str);
+
   void writeTo(raw_ostream &OS) const;
 
 private:
   size_t NextOffset = 0;
 
   std::vector<std::function<void(raw_ostream &)>> Callbacks;
 };
 } // namespace
 
+size_t BlobAllocator::allocateString(StringRef Str) {
+  SmallVector<UTF16, 32> WStr;
+  bool OK = convertUTF8ToUTF16String(Str, WStr);
+  assert(OK && "Invalid UTF8 in Str?");
+  (void)OK;
+
+  SmallVector<support::ulittle16_t, 32> EndianStr(WStr.size() + 1,
+                                                  support::ulittle16_t());
+  copy(WStr, EndianStr.begin());
+  return allocateCallback(
+      sizeof(uint32_t) + EndianStr.size() * sizeof(support::ulittle16_t),
+      [EndianStr](raw_ostream &OS) {
+        // Length does not include the null-terminator.
+        support::ulittle32_t Length(2 * (EndianStr.size() - 1));
+        OS.write(reinterpret_cast<const char *>(&Length), sizeof(Length));
+        OS.write(reinterpret_cast<const char *>(EndianStr.begin()),
+                 sizeof(support::ulittle16_t) * EndianStr.size());
+      });
+}
+
 void BlobAllocator::writeTo(raw_ostream &OS) const {
   size_t BeginOffset = OS.tell();
   for (const auto &Callback : Callbacks)
     Callback(OS);
   assert(OS.tell() == BeginOffset + NextOffset &&
          "Callbacks wrote an unexpected number of bytes.");
   (void)BeginOffset;
 }
static void streamMapping(yaml::IO &IO, SystemInfoStream &Stream) {
   mapOptional(IO, "Processor Level", Info.ProcessorLevel, 0);
   mapOptional(IO, "Processor Revision", Info.ProcessorRevision, 0);
   IO.mapOptional("Number of Processors", Info.NumberOfProcessors, 0);
   IO.mapOptional("Product type", Info.ProductType, 0);
   mapOptional(IO, "Major Version", Info.MajorVersion, 0);
   mapOptional(IO, "Minor Version", Info.MinorVersion, 0);
   mapOptional(IO, "Build Number", Info.BuildNumber, 0);
   IO.mapRequired("Platform ID", Info.PlatformId);
-  mapOptionalHex(IO, "CSD Version RVA", Info.CSDVersionRVA, 0);
+  IO.mapOptional("CSD Version", Stream.CSDVersion, "");
   mapOptionalHex(IO, "Suite Mask", Info.SuiteMask, 0);
   mapOptionalHex(IO, "Reserved", Info.Reserved, 0);
   switch (static_cast<ProcessorArchitecture>(Info.ProcessorArch)) {
   case ProcessorArchitecture::X86:
   case ProcessorArchitecture::AMD64:
     IO.mapOptional("CPU", Info.CPU.X86);
     break;
   case ProcessorArchitecture::ARM:
void yaml::MappingTraits<Object>::mapping(IO &IO, Object &O) {
   mapOptionalHex(IO, "Flags", O.Header.Flags, 0);
   IO.mapRequired("Streams", O.Streams);
 }
 
 static Directory layout(BlobAllocator &File, Stream &S) {
   Directory Result;
   Result.Type = S.Type;
   Result.Location.RVA = File.tell();
+  Optional<size_t> DataEnd;
   switch (S.Kind) {
   case Stream::StreamKind::RawContent: {
     RawContentStream &Raw = cast<RawContentStream>(S);
     File.allocateCallback(Raw.Size, [&Raw](raw_ostream &OS) {
       Raw.Content.writeAsBinary(OS);
       assert(Raw.Content.binary_size() <= Raw.Size);
       OS << std::string(Raw.Size - Raw.Content.binary_size(), '\0');
     });
     break;
   }
-  case Stream::StreamKind::SystemInfo:
-    File.allocateObject(cast<SystemInfoStream>(S).Info);
+  case Stream::StreamKind::SystemInfo: {
+    SystemInfoStream &SystemInfo = cast<SystemInfoStream>(S);
+    File.allocateObject(SystemInfo.Info);
+    // The CSD string is not a part of the stream.
+    DataEnd = File.tell();
+    SystemInfo.Info.CSDVersionRVA = File.allocateString(SystemInfo.CSDVersion);
     break;
+  }
   case Stream::StreamKind::TextContent:
     File.allocateArray(arrayRefFromStringRef(cast<TextContentStream>(S).Text));
     break;
   }
-  Result.Location.DataSize = File.tell() - Result.Location.RVA;
+  // If DataEnd is not set, we assume everything we generated is a part of the
+  // stream.
+  Result.Location.DataSize =
+      DataEnd.getValueOr(File.tell()) - Result.Location.RVA;
   return Result;
 }
 
 void MinidumpYAML::writeAsBinary(Object &Obj, raw_ostream &OS) {
   BlobAllocator File;
   File.allocateObject(Obj.Header);
 
   std::vector<Directory> StreamDirectory(Obj.Streams.size());
Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
   switch (Kind) {
   case StreamKind::RawContent:
     return make_unique<RawContentStream>(StreamDesc.Type,
                                          File.getRawStream(StreamDesc));
   case StreamKind::SystemInfo: {
     auto ExpectedInfo = File.getSystemInfo();
     if (!ExpectedInfo)
       return ExpectedInfo.takeError();
-    return make_unique<SystemInfoStream>(*ExpectedInfo);
+    auto ExpectedCSDVersion = File.getString(ExpectedInfo->CSDVersionRVA);
+    if (!ExpectedCSDVersion)
+      return ExpectedInfo.takeError();
+    return make_unique<SystemInfoStream>(*ExpectedInfo,
+                                         std::move(*ExpectedCSDVersion));
   }
   case StreamKind::TextContent:
     return make_unique<TextContentStream>(
         StreamDesc.Type, toStringRef(File.getRawStream(StreamDesc)));
   }
   llvm_unreachable("Unhandled stream kind!");
 }
 

llvm/trunk/test/tools/obj2yaml/basic-minidump.yaml

 # RUN: yaml2obj %s | obj2yaml - | FileCheck %s
 
 --- !minidump
 Streams:         
   - Type:            SystemInfo
     Processor Arch:  ARM64
     Platform ID:     Linux
-    CSD Version RVA: 0x01020304
+    CSD Version:     Linux 3.13.0-91-generic
     CPU:             
       CPUID:           0x05060708
   - Type:            LinuxAuxv
     Content:         DEADBEEFBAADF00D
   - Type:            LinuxMaps
     Text:             |
       400d9000-400db000 r-xp 00000000 b3:04 227        /system/bin/app_process
       400db000-400dc000 r--p 00001000 b3:04 227        /system/bin/app_process
 
 ...
 
 # CHECK:      --- !minidump
 # CHECK-NEXT: Streams:         
 # CHECK-NEXT:   - Type:            SystemInfo
 # CHECK-NEXT:     Processor Arch:  ARM64
 # CHECK-NEXT:     Platform ID:     Linux
-# CHECK-NEXT:     CSD Version RVA: 0x01020304
+# CHECK-NEXT:     CSD Version:     Linux 3.13.0-91-generic
 # CHECK-NEXT:     CPU:             
 # CHECK-NEXT:       CPUID:           0x05060708
 # CHECK-NEXT:   - Type:            LinuxAuxv
 # CHECK-NEXT:     Content:         DEADBEEFBAADF00D
 # CHECK-NEXT:   - Type:            LinuxMaps
 # CHECK-NEXT:     Text:             |
 # CHECK-NEXT:       400d9000-400db000 r-xp 00000000 b3:04 227        /system/bin/app_process
 # CHECK-NEXT:       400db000-400dc000 r--p 00001000 b3:04 227        /system/bin/app_process
 # CHECK-EMPTY:
 # CHECK-NEXT: ...

llvm/trunk/unittests/Object/MinidumpTest.cpp

TEST(MinidumpFile, getSystemInfo) {
   EXPECT_EQ(0x0605u, Info.SuiteMask);
   EXPECT_EQ(0x0807u, Info.Reserved);
   EXPECT_EQ("LLVMLLVMLLVM", llvm::StringRef(Info.CPU.X86.VendorID,
                                             sizeof(Info.CPU.X86.VendorID)));
   EXPECT_EQ(0x04030201u, Info.CPU.X86.VersionInfo);
   EXPECT_EQ(0x08070605u, Info.CPU.X86.FeatureInfo);
   EXPECT_EQ(0x02010009u, Info.CPU.X86.AMDExtendedFeatures);
 }
+
+TEST(MinidumpFile, getString) {
+  std::vector<uint8_t> ManyStrings{
+      // Header
+      'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+      2, 0, 0, 0,                           // NumberOfStreams,
+      0x20, 0, 0, 0,                        // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
+      8, 9, 0, 1, 2, 3, 4, 5,               // Flags
+                                            // Stream Directory
+      0, 0, 0, 0, 0, 0, 0, 0,               // Type, DataSize,
+      0x20, 0, 0, 0,                        // RVA
+      1, 0, 0, 0, 0, 0,                     // String1 - odd length
+      0, 0, 1, 0, 0, 0,                     // String2 - too long
+      2, 0, 0, 0, 0, 0xd8,                  // String3 - invalid utf16
+      0, 0, 0, 0, 0, 0,                     // String4 - ""
+      2, 0, 0, 0, 'a', 0,                   // String5 - "a"
+      0,                                    // Mis-align next string
+      2, 0, 0, 0, 'a', 0,                   // String6 - "a"
+
+  };
+  auto ExpectedFile = create(ManyStrings);
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  const MinidumpFile &File = **ExpectedFile;
+  EXPECT_THAT_EXPECTED(File.getString(44), Failed<BinaryError>());
+  EXPECT_THAT_EXPECTED(File.getString(50), Failed<BinaryError>());
+  EXPECT_THAT_EXPECTED(File.getString(56), Failed<BinaryError>());
+  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>());
+}

llvm/trunk/unittests/ObjectYAML/MinidumpYAMLTest.cpp

 TEST(MinidumpYAML, Basic) {
   SmallString<0> Storage;
   auto ExpectedFile = toBinary(Storage, R"(
 --- !minidump
 Streams:
   - Type:            SystemInfo
     Processor Arch:  ARM64
     Platform ID:     Linux
-    CSD Version RVA: 0x01020304
     CPU:
       CPUID:           0x05060708
   - Type:            LinuxMaps
     Text:             |
       400d9000-400db000 r-xp 00000000 b3:04 227        /system/bin/app_process
       400db000-400dc000 r--p 00001000 b3:04 227        /system/bin/app_process
 
   - Type:            LinuxAuxv
     Content:         DEADBEEFBAADF00D)");
   ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
   object::MinidumpFile &File = **ExpectedFile;
 
   ASSERT_EQ(3u, File.streams().size());
 
   EXPECT_EQ(StreamType::SystemInfo, File.streams()[0].Type);
   auto ExpectedSysInfo = File.getSystemInfo();
   ASSERT_THAT_EXPECTED(ExpectedSysInfo, Succeeded());
   const SystemInfo &SysInfo = *ExpectedSysInfo;
   EXPECT_EQ(ProcessorArchitecture::ARM64, SysInfo.ProcessorArch);
   EXPECT_EQ(OSPlatform::Linux, SysInfo.PlatformId);
-  EXPECT_EQ(0x01020304u, SysInfo.CSDVersionRVA);
   EXPECT_EQ(0x05060708u, SysInfo.CPU.Arm.CPUID);
 
   EXPECT_EQ(StreamType::LinuxMaps, File.streams()[1].Type);
   EXPECT_EQ("400d9000-400db000 r-xp 00000000 b3:04 227        "
             "/system/bin/app_process\n"
             "400db000-400dc000 r--p 00001000 b3:04 227        "
             "/system/bin/app_process\n",
             toStringRef(*File.getRawStream(StreamType::LinuxMaps)));