[PATCH] D35166: [DWARF] Introduce verification for the unit header chain in .debug_info section to llvm-dwarfdump.

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:241
 
+  static bool validateUnitType(uint8_t UnitType) {
+    return UnitType == dwarf::DW_UT_compile || UnitType == dwarf::DW_UT_type ||
----------------
If these static methods are used only by the verifier, maybe they should just be local functions in DWARFVerifier.cpp.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:39
+
+  uint32_t Offset = 4;
+  uint16_t Version = DebugInfoData.getU16(&Offset);
----------------
This is assuming DWARF-32 format.  It's okay to support only one format, if you have a check for DWARF-64 and explicitly reject it.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:44
+    UnitType = DebugInfoData.getU8(&Offset);
+    HeaderSize = DWARFUnit::computeDWARF5HeaderSize(UnitType);
+  }
----------------
This could call computeDWARF5HeaderSize with an invalid unit type.  That's okay if you document the contract in the declaration of that method.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:51
+    ValidOffset = DebugInfoData.isValidOffset(Offset);
+    Length = DebugInfoData.getU32(&Offset);
+    Version = DebugInfoData.getU16(&Offset);
----------------
Might want a DWARF-64 check here.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:54
+
+    if (Version >= 5) {
+      UnitType = DebugInfoData.getU8(&Offset);
----------------
Improper TOCTOU.  Admittedly this is far from security-sensitive, but using data before validating it is generally poor practice.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:59
+      ValidType = DWARFUnit::validateUnitType(UnitType);
+      HeaderSize = DWARFUnit::computeDWARF5HeaderSize(UnitType);
+    } else {
----------------
Again could call computeDWARF5HeaderSize with an invalid unit type.


https://reviews.llvm.org/D35166