[PATCH] D63713: WIP: DataExtractor error handling

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

The new API doesn't let you eliminate *all* checks. :-)
Also it introduces some new dependencies as noted inline.



================
Comment at: include/llvm/Support/DataExtractor.h:425
 
+class DataStream {
+public:
----------------
JDevlieghere wrote:
> We'll definitely need to add some comment here to motivate its existence, probably contrasting it with the statelessness of the DataExtractor. I saw this patch first and I wondered "why not add this to the DataExtractor directly?", before catching up on the other patch. 
Also that this depends on DataExtractor "get" methods returning zero for past-the-end calls.


================
Comment at: include/llvm/Support/DataExtractor.h:428
+  DataStream(const DataExtractor &DE, uint32_t Offset = 0)
+      : DE(DE), Offset(Offset), Error(false) {}
+  DataStream(const DataStream &) = delete;
----------------
`assert(Offset <= DE.size());` to guard against construction with a bogus Offset?


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:60
+    auto AtomForm = static_cast<dwarf::Form>(DS.getU16());
+    HdrData.Atoms.push_back(std::make_pair(AtomType, AtomForm));
+  }
----------------
NumAtoms still isn't validated before being used, and could consume all available memory before you get to the error check.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:389
+  AugmentationStringSize = alignTo(DS.getU32(), 4);
   AugmentationString.resize(AugmentationStringSize);
+  DS.getU8(reinterpret_cast<uint8_t *>(AugmentationString.data()),
----------------
You're now resizing the string without validating the new size.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:441
 
-    Result.emplace_back(*AttrEncOr);
+    Result.emplace_back(AttrEnc);
   }
----------------
labath wrote:
> This check wasn't fully correct, because we could still cross the `EntriesBase` boundary after reading the two ULEB fields.
This depends on `extractAttributeEncoding()`returning a sentinel if you run off the end of the stream.  That depends on `sentinalAttrEnc()` using the same values that `getULEB128()` returns when it runs off the end.  These dependencies need to be documented.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:492
+    if (isSentinel(Abbr))
+      break;
 
----------------
Again the requirements on sentinels need to be documented (not here, but where those functions are defined).


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:148
 
+  // dwarf::DW_LLE_end_of_list_entry is 0 and indicates the end of the list.
+  while (auto Kind = static_cast<dwarf::LocationListEntry>(DS.getU8())) {
----------------
Might want a static_assert for that?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:181
+      StringRef str = DS.getBytes(Bytes);
       E.Loc.resize(str.size());
       llvm::copy(str, E.Loc.begin());
----------------
You've lost a range check on the size.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63713/new/

https://reviews.llvm.org/D63713