DWARFDebugLoc: Make parsing and error reporting more robust

Summary:
While examining this class for possible use in lldb, I noticed two
things:
- it spits out parsing errors directly to stderr
- the loclists parser can incorrectly return valid location lists when
  parsing malformed (truncated) data

I improve the stderr situation by making the parseOneLocationList
functions return Expected<T>s. The errors are still dumped to stderr by
their callers, so this is only a partial fix, but it is enough for my
use case, as I intend to parse the locations lists one by one.

I fix the behavior in the truncated scenario by using the newly
introduced DataExtractor Cursor API.

I also add tests for handling the error cases, as they currently have no
coverage.

Reviewers: dblaikie, JDevlieghere, probinson

Subscribers: lldb-commits, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D63591

llvm-svn: 370363

2 files changed, 49 insertions, 60 deletions

diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
index 1469a210785..e932f98204e 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
@@ -28,11 +28,10 @@ using namespace llvm;
 // expression that LLVM doesn't produce. Guessing the wrong version means we
 // won't be able to pretty print expressions in DWARF2 binaries produced by
 // non-LLVM tools.
-static void dumpExpression(raw_ostream &OS, ArrayRef<char> Data,
+static void dumpExpression(raw_ostream &OS, ArrayRef<uint8_t> Data,
                            bool IsLittleEndian, unsigned AddressSize,
                            const MCRegisterInfo *MRI, DWARFUnit *U) {
-  DWARFDataExtractor Extractor(StringRef(Data.data(), Data.size()),
-                               IsLittleEndian, AddressSize);
+  DWARFDataExtractor Extractor(toStringRef(Data), IsLittleEndian, AddressSize);
   DWARFExpression(Extractor, dwarf::DWARF_VERSION, AddressSize).print(OS, MRI, U);
 }
 
@@ -83,47 +82,37 @@ void DWARFDebugLoc::dump(raw_ostream &OS, const MCRegisterInfo *MRI,
   }
 }
 
-Optional<DWARFDebugLoc::LocationList>
-DWARFDebugLoc::parseOneLocationList(DWARFDataExtractor Data, uint64_t *Offset) {
+Expected<DWARFDebugLoc::LocationList>
+DWARFDebugLoc::parseOneLocationList(const DWARFDataExtractor &Data,
+                                    uint64_t *Offset) {
   LocationList LL;
   LL.Offset = *Offset;
+  DataExtractor::Cursor C(*Offset);
 
   // 2.6.2 Location Lists
   // A location list entry consists of:
   while (true) {
     Entry E;
-    if (!Data.isValidOffsetForDataOfSize(*Offset, 2 * Data.getAddressSize())) {
-      WithColor::error() << "location list overflows the debug_loc section.\n";
-      return None;
-    }
 
     // 1. A beginning address offset. ...
-    E.Begin = Data.getRelocatedAddress(Offset);
+    E.Begin = Data.getRelocatedAddress(C);
 
     // 2. An ending address offset. ...
-    E.End = Data.getRelocatedAddress(Offset);
+    E.End = Data.getRelocatedAddress(C);
 
+    if (Error Err = C.takeError())
+      return std::move(Err);
     // The end of any given location list is marked by an end of list entry,
     // which consists of a 0 for the beginning address offset and a 0 for the
     // ending address offset.
-    if (E.Begin == 0 && E.End == 0)
+    if (E.Begin == 0 && E.End == 0) {
+      *Offset = C.tell();
       return LL;
-
-    if (!Data.isValidOffsetForDataOfSize(*Offset, 2)) {
-      WithColor::error() << "location list overflows the debug_loc section.\n";
-      return None;
     }
 
-    unsigned Bytes = Data.getU16(Offset);
-    if (!Data.isValidOffsetForDataOfSize(*Offset, Bytes)) {
-      WithColor::error() << "location list overflows the debug_loc section.\n";
-      return None;
-    }
+    unsigned Bytes = Data.getU16(C);
     // A single location description describing the location of the object...
-    StringRef str = Data.getData().substr(*Offset, Bytes);
-    *Offset += Bytes;
-    E.Loc.reserve(str.size());
-    llvm::copy(str, std::back_inserter(E.Loc));
+    Data.getU8(C, E.Loc, Bytes);
     LL.Entries.push_back(std::move(E));
   }
 }
@@ -133,67 +122,65 @@ void DWARFDebugLoc::parse(const DWARFDataExtractor &data) {
   AddressSize = data.getAddressSize();
 
   uint64_t Offset = 0;
-  while (data.isValidOffset(Offset + data.getAddressSize() - 1)) {
+  while (Offset < data.getData().size()) {
     if (auto LL = parseOneLocationList(data, &Offset))
       Locations.push_back(std::move(*LL));
-    else
+    else {
+      logAllUnhandledErrors(LL.takeError(), WithColor::error());
       break;
+    }
   }
-  if (data.isValidOffset(Offset))
-    WithColor::error() << "failed to consume entire .debug_loc section\n";
 }
 
-Optional<DWARFDebugLoclists::LocationList>
-DWARFDebugLoclists::parseOneLocationList(DataExtractor Data, uint64_t *Offset,
-                                         unsigned Version) {
+Expected<DWARFDebugLoclists::LocationList>
+DWARFDebugLoclists::parseOneLocationList(const DataExtractor &Data,
+                                         uint64_t *Offset, unsigned Version) {
   LocationList LL;
   LL.Offset = *Offset;
+  DataExtractor::Cursor C(*Offset);
 
   // dwarf::DW_LLE_end_of_list_entry is 0 and indicates the end of the list.
-  while (auto Kind =
-             static_cast<dwarf::LocationListEntry>(Data.getU8(Offset))) {
-
+  while (auto Kind = static_cast<dwarf::LocationListEntry>(Data.getU8(C))) {
     Entry E;
     E.Kind = Kind;
     switch (Kind) {
     case dwarf::DW_LLE_startx_length:
-      E.Value0 = Data.getULEB128(Offset);
+      E.Value0 = Data.getULEB128(C);
       // Pre-DWARF 5 has different interpretation of the length field. We have
       // to support both pre- and standartized styles for the compatibility.
       if (Version < 5)
-        E.Value1 = Data.getU32(Offset);
+        E.Value1 = Data.getU32(C);
       else
-        E.Value1 = Data.getULEB128(Offset);
+        E.Value1 = Data.getULEB128(C);
       break;
     case dwarf::DW_LLE_start_length:
-      E.Value0 = Data.getAddress(Offset);
-      E.Value1 = Data.getULEB128(Offset);
+      E.Value0 = Data.getAddress(C);
+      E.Value1 = Data.getULEB128(C);
       break;
     case dwarf::DW_LLE_offset_pair:
-      E.Value0 = Data.getULEB128(Offset);
-      E.Value1 = Data.getULEB128(Offset);
+      E.Value0 = Data.getULEB128(C);
+      E.Value1 = Data.getULEB128(C);
       break;
     case dwarf::DW_LLE_base_address:
-      E.Value0 = Data.getAddress(Offset);
+      E.Value0 = Data.getAddress(C);
       break;
     default:
-      WithColor::error() << "dumping support for LLE of kind " << (int)Kind
-                         << " not implemented\n";
-      return None;
+      cantFail(C.takeError());
+      return createStringError(errc::illegal_byte_sequence,
+                               "LLE of kind %x not supported", (int)Kind);
     }
 
     if (Kind != dwarf::DW_LLE_base_address) {
-      unsigned Bytes =
-          Version >= 5 ? Data.getULEB128(Offset) : Data.getU16(Offset);
+      unsigned Bytes = Version >= 5 ? Data.getULEB128(C) : Data.getU16(C);
       // A single location description describing the location of the object...
-      StringRef str = Data.getData().substr(*Offset, Bytes);
-      *Offset += Bytes;
-      E.Loc.resize(str.size());
-      llvm::copy(str, E.Loc.begin());
+      Data.getU8(C, E.Loc, Bytes);
     }
 
     LL.Entries.push_back(std::move(E));
   }
+  if (Error Err = C.takeError())
+    return std::move(Err);
+  *Offset = C.tell();
   return LL;
 }
 
@@ -202,11 +189,13 @@ void DWARFDebugLoclists::parse(DataExtractor data, unsigned Version) {
   AddressSize = data.getAddressSize();
 
   uint64_t Offset = 0;
-  while (data.isValidOffset(Offset)) {
+  while (Offset < data.getData().size()) {
     if (auto LL = parseOneLocationList(data, &Offset, Version))
       Locations.push_back(std::move(*LL));
-    else
+    else {
+      logAllUnhandledErrors(LL.takeError(), WithColor::error());
       return;
+    }
   }
 }
 
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
index 8ea27402bc4..69b93e7d7cf 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -466,9 +466,9 @@ unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
     ReportError("DIE has invalid DW_AT_stmt_list encoding:");
     break;
   case DW_AT_location: {
-    auto VerifyLocationExpr = [&](StringRef D) {
+    auto VerifyLocationExpr = [&](ArrayRef<uint8_t> D) {
       DWARFUnit *U = Die.getDwarfUnit();
-      DataExtractor Data(D, DCtx.isLittleEndian(), 0);
+      DataExtractor Data(toStringRef(D), DCtx.isLittleEndian(), 0);
       DWARFExpression Expression(Data, U->getVersion(),
                                  U->getAddressByteSize());
       bool Error = llvm::any_of(Expression, [](DWARFExpression::Operation &Op) {
@@ -479,7 +479,7 @@ unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
     };
     if (Optional<ArrayRef<uint8_t>> Expr = AttrValue.Value.getAsBlock()) {
       // Verify inlined location.
-      VerifyLocationExpr(llvm::toStringRef(*Expr));
+      VerifyLocationExpr(*Expr);
     } else if (auto LocOffset = AttrValue.Value.getAsSectionOffset()) {
       // Verify location list.
       if (auto DebugLoc = DCtx.getDebugLoc())
@@ -1277,9 +1277,9 @@ static bool isVariableIndexable(const DWARFDie &Die, DWARFContext &DCtx) {
   if (!Location)
     return false;
 
-  auto ContainsInterestingOperators = [&](StringRef D) {
+  auto ContainsInterestingOperators = [&](ArrayRef<uint8_t> D) {
     DWARFUnit *U = Die.getDwarfUnit();
-    DataExtractor Data(D, DCtx.isLittleEndian(), U->getAddressByteSize());
+    DataExtractor Data(toStringRef(D), DCtx.isLittleEndian(), U->getAddressByteSize());
     DWARFExpression Expression(Data, U->getVersion(), U->getAddressByteSize());
     return any_of(Expression, [](DWARFExpression::Operation &Op) {
       return !Op.isError() && (Op.getCode() == DW_OP_addr ||
@@ -1290,7 +1290,7 @@ static bool isVariableIndexable(const DWARFDie &Die, DWARFContext &DCtx) {
 
   if (Optional<ArrayRef<uint8_t>> Expr = Location->getAsBlock()) {
     // Inlined location.
-    if (ContainsInterestingOperators(toStringRef(*Expr)))
+    if (ContainsInterestingOperators(*Expr))
       return true;
   } else if (Optional<uint64_t> Offset = Location->getAsSectionOffset()) {
     // Location list.