[DWARF] Better detect errors in Address Range Tables.

[ikudrin]

Summary

The patch tries to cover most remaining cases of wrong data.

[dblaikie]

Could these tests all be in one run? (ie: even though the length is "invalid" in terms of parsing the contents of the section - but valid enough to jump to the next contribution and parse that - except, I guess, at an invalid 64 bit length perhaps, because that might be too long to be bothered testing - so perhaps that could be the last test case in the file)

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

138–142

Worth having two messages here to more specifically communicate the kind of invalidity? ("invalid length" is a bit vague/doesn't really say what to do about the length/why it's a problem - even if this remains as one message, perhaps the message should describe the two kinds of invalidity that could be the problem)

[ikudrin]

• Made the diagnostic a bit more distinct.
• Moved the tests to a gtest-based suite.

[ikudrin]

In D71932#1797512, @dblaikie wrote:

Could these tests all be in one run? (ie: even though the length is "invalid" in terms of parsing the contents of the section - but valid enough to jump to the next contribution and parse that - except, I guess, at an invalid 64 bit length perhaps, because that might be too long to be bothered testing - so perhaps that could be the last test case in the file)

As I said in D71876, that requires a separate patch and a separate consideration. Hope that gtest-based tests solve the issue in a better way.

[ikudrin]

Ping.

[dblaikie]

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

[dblaikie]

Sounds good - thanks!

[dblaikie]

Let's try that again - guess I'd already approved this before.

[ikudrin]

Thanks for the review!

In D71932#1827601, @dblaikie wrote:

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

My main intention is to add support for 64-bit DWARF. All other changes are just accompanying the main goal, like fixing issues when I found them or making other small improvements. I believe there should be no conflicts with James Henderson's changes.

[dblaikie]

In D71932#1827849, @ikudrin wrote:

Thanks for the review!

In D71932#1827601, @dblaikie wrote:

Looks good - future work done as separate patches for separable changes/diagnostics would probably be good (& if you've got some broader goals in terms of what functionality you need/would like in llvm-dwarfdump, an email to llvm-dev explaining the broader goals (do they overlap with James Henderson's goals, improving the line table error handling?) would probably be good - to link to in future reviews, etc)

My main intention is to add support for 64-bit DWARF.

Ah, cool - thanks for that! (out of curiosity, but by no means necessary - what's the motivation for that? LLVM doesn't currently produce 64-bit DWARF, I think & haven't seen any user-requests/had any discussions about adding it that I recall, but I might've missed some. Useful to know what companies/projects/etc are interested in these things, if there's more context you'd like to share)

All other changes are just accompanying the main goal, like fixing issues when I found them or making other small improvements. I believe there should be no conflicts with James Henderson's changes.

[ikudrin]

• Make expressions constexpr instead of static const.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

162–166

@ikudrin, I know this was committed a while back, but I only just ran into this.

Why was this check added? There's nothing in the DWARF spec that I know of that requires a non-zero length for all non-terminating entries. In our Sony linker, prior to the new tombstoning functionality that has recently been implemented, we would patch a reference in the .debug_aranges to have a zero length, and -1 for the address. This used to be fine, but recent versions of llvm-dwarfdump now can't dump this. Even without the Sony changes, there are occasionally functions in the DWARF with zero length (see also my post earlier today on the llvm-dev list for .debug_loc and zero length entry descriptions).

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

162–166

Yep, +1 we should allow empty ranges here. Even with better tombstoning - a) backwards compatibility with existing implementations (ld.bfd) b) even with proper tombstoning, it'd effectively create an empty range (-2, -2) - though arguably in that case we should detect the tombstoning first, before considering the range size/other issues

Here's a reproduction with commonly accessible tools:

$ cat range2.cpp
void f1() {
}
void f2() {
}
int main() {
  f1();
}
$ clang++ range2.cpp -g -fuse-ld=bfd -ffunction-sections -gdwarf-aranges -Wl,-gc-sections
$ llvm-dwarfdump -verify a.out
Verifying a.out:        file format elf64-x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
error: DIE address ranges are not contained in its parent's ranges:
0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("clang version 12.0.0 ([e-mail]:llvm/llvm-project.git 80e6844e28f10c1c2c31b301c3ef980c7a11ee6b)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("range2.cpp")
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (0x00000000
                 [0x0000000000401110, 0x0000000000401116)
                 [0x0000000000000001, 0x0000000000000001)
                 [0x0000000000401120, 0x000000000040112d))

0x00000043:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000006)
                DW_AT_frame_base        (DW_OP_reg6)
                DW_AT_linkage_name      ("_Z2f2v")
                DW_AT_name      ("f2")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/range2.cpp")
                DW_AT_decl_line (3)
                DW_AT_external  (true)

Verifying .debug_info references...
Verifying .debug_types Unit Header Chain...
Errors detected.

Similarly, with GCC (which produces debug_aranges by default):

$ g++ range2.cpp -g -fuse-ld=bfd -ffunction-sections -Wl,-gc-sections
$ llvm-dwarfdump -verify a.out
Verifying a.out:        file format elf64-x86-64
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
error: DIE address ranges are not contained in its parent's ranges:
0x0000000b: DW_TAG_compile_unit
              DW_AT_producer    ("GNU C++14 10.0.0 20200111 (experimental) -mtune=generic -march=x86-64 -g -fuse-ld=bfd -ffunction-sections")
              DW_AT_language    (DW_LANG_C_plus_plus)
              DW_AT_name        ("range2.cpp")
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_ranges      (0x00000000
                 [0x0000000000401102, 0x0000000000401109)
                 [0x0000000000000001, 0x0000000000000001)
                 [0x0000000000401109, 0x0000000000401119))
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_stmt_list   (0x00000000)

0x0000004e:   DW_TAG_subprogram
                DW_AT_external  (true)
                DW_AT_name      ("f2")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/range2.cpp")
                DW_AT_decl_line (3)
                DW_AT_decl_column       (0x06)
                DW_AT_linkage_name      ("_Z2f2v")
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x0000000000000007)
                DW_AT_frame_base        (DW_OP_call_frame_cfa)
                DW_AT_GNU_all_call_sites        (true)

Verifying .debug_info references...
Verifying .debug_types Unit Header Chain...
Errors detected.

[jhenderson]

Thanks @dblaikie - I've filed https://bugs.llvm.org/show_bug.cgi?id=46805 to track this. Not got time currently to fix it myself - I've hacked out the error message for now locally to unblock my work, but that removes the useful check for a premature terminator.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

162–166

Why was this check added? There's nothing in the DWARF spec that I know of that requires a non-zero length for all non-terminating entries.

I tried to follow the Standard here, see 6.1.2@DWARFv5, p. 148:

Each descriptor is a triple consisting of a segment selector, the beginning address within that segment of a range of text or data covered by some entry owned by the corresponding compilation unit, followed by the non-zero length of that range.

It looks like the DWARF spec should be updated to reflect the practice.

@jhenderson, it would be great if you prepare the fix. I'll be AFK for a couple of weeks.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

162–166

Huh, you're right. We must have missed that bit in the spec when we implemented our internal solution for stripped references. I guess the trouble is that if you do remove the non-zero length bit from the spec, a system with a zero address would treat it as a terminator, unless we also removed the terminator from the spec (I don't see why we couldn't - an aranges table has a known length). I'll post on https://bugs.llvm.org/show_bug.cgi?id=46805 that point, to see if I can get some feedback from other DWARF folks.

I may not get around to the fix for a bit - I've got a few other things on my plate currently, but this is certainly going to stay on my radar so if I do get a chance I'll come back to it. I don't think it's an urgent enough issue to warrant a release branch patch, unless somebody else feels it is.

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

Error DWARFDebugArangeSet::extract(DataExtractor data, uint64_t *offset_ptr) {
   // a segment, an address and a length. The segment selector size is given by
   // the segment_selector_size field of the header; the address and length
   // size are each given by the address_size field of the header. Each set of
   // tuples is terminated by a 0 for the segment, a 0 for the address and 0
   // for the length. If the segment_selector_size field in the header is zero,
   // the segment selectors are omitted from all tuples, including
   // the terminating tuple.
 
+  static const unsigned DWARF32HeaderLength = 4 + 2 + 4 + 1 + 1;
+  static const unsigned DWARF64HeaderLength = 12 + 2 + 8 + 1 + 1;
+
+  if (!data.isValidOffsetForDataOfSize(Offset, DWARF32HeaderLength))
+    return createStringError(errc::invalid_argument,
+                             "section is not large enough to contain "
+                             "an address range table at offset 0x%" PRIx64,
+                             Offset);
+
   // We do not call FormParams::getRefAddrByteSize(),
   // so 4 for Version and 4 for AddrSize are taken arbitrary.
   dwarf::FormParams params = {4, 4, dwarf::DWARF32};
   HeaderData.Length = data.getU32(offset_ptr);
   if (HeaderData.Length == dwarf::DW_LENGTH_DWARF64) {
+    if (!data.isValidOffsetForDataOfSize(Offset, DWARF64HeaderLength))
+      return createStringError(
+          errc::invalid_argument,
+          "section is not large enough to contain a DWARF64 "
+          "address range table at offset 0x%" PRIx64,
+          Offset);
     HeaderData.Length = data.getU64(offset_ptr);
     params.Format = dwarf::DWARF64;
   } else if (HeaderData.Length >= dwarf::DW_LENGTH_lo_reserved) {
     return createStringError(
         errc::invalid_argument,
         "address range table at offset 0x%" PRIx64
         " has unsupported reserved unit length of value 0x%8.8" PRIx64,
         Offset, HeaderData.Length);
return createStringError(errc::invalid_argument,
                              "0x%" PRIx64 " exceeds section size",
                              Offset);
   if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
     return createStringError(errc::invalid_argument,
                              "address range table at offset 0x%" PRIx64
                              " has unsupported address size: %d "
                              "(4 and 8 supported)",
                              Offset, HeaderData.AddrSize);
+  if (HeaderData.SegSize != 0)
+    return createStringError(errc::not_supported,
+                             "non-zero segment selector size in address range "
+                             "table at offset 0x%" PRIx64 " is not supported",
+                             Offset);
 
-  // The first tuple following the header in each set begins at an offset
-  // that is a multiple of the size of a single tuple (that is, twice the
-  // size of an address). The header is padded, if necessary, to the
-  // appropriate boundary.
-  const uint32_t header_size = *offset_ptr - Offset;
+  // The first tuple following the header in each set begins at an offset that
+  // is a multiple of the size of a single tuple (that is, twice the size of
+  // an address because we do not support non-zero segment selector sizes).
+  // Therefore, the full length should also be a multiple of the tuple size.
   const uint32_t tuple_size = HeaderData.AddrSize * 2;
+  if (full_length % tuple_size != 0)
+    return createStringError(
+        errc::invalid_argument,
+        "address range table at offset 0x%" PRIx64
+        " has length that is not a multiple of the tuple size",
+        Offset);
+
+  // The header is padded, if necessary, to the appropriate boundary.
+  const uint32_t header_size = *offset_ptr - Offset;
   uint32_t first_tuple_offset = 0;
   while (first_tuple_offset < header_size)
     first_tuple_offset += tuple_size;
 
+  // There should be space for at least one tuple.
+  if (full_length <= first_tuple_offset)
+    return createStringError(
+        errc::invalid_argument,
+        "address range table at offset 0x%" PRIx64
+        " has an insufficient length to contain any entries",
+        Offset);
+
   *offset_ptr = Offset + first_tuple_offset;
 
   Descriptor arangeDescriptor;
 
   static_assert(sizeof(arangeDescriptor.Address) ==
                     sizeof(arangeDescriptor.Length),
                 "Different datatypes for addresses and sizes!");
   assert(sizeof(arangeDescriptor.Address) >= HeaderData.AddrSize);
 
-  while (data.isValidOffset(*offset_ptr)) {
+  uint64_t end_offset = Offset + full_length;
+  while (*offset_ptr < end_offset) {
     arangeDescriptor.Address = data.getUnsigned(offset_ptr, HeaderData.AddrSize);
     arangeDescriptor.Length = data.getUnsigned(offset_ptr, HeaderData.AddrSize);
 
+    if (arangeDescriptor.Length == 0) {
       // Each set of tuples is terminated by a 0 for the address and 0
       // for the length.
-    if (arangeDescriptor.Address == 0 && arangeDescriptor.Length == 0)
+      if (arangeDescriptor.Address == 0 && *offset_ptr == end_offset)
         return ErrorSuccess();
+      return createStringError(
+          errc::invalid_argument,
+          "address range table at offset 0x%" PRIx64
+          " has an invalid tuple (length = 0) at offset 0x%" PRIx64,
+          Offset, *offset_ptr - tuple_size);
+    }
+
     ArangeDescriptors.push_back(arangeDescriptor);
   }
 
   return createStringError(errc::invalid_argument,
                            "address range table at offset 0x%" PRIx64
                            " is not terminated by null entry",
                            Offset);
 }

llvm/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp

static const char DebugArangesSecRaw[] =
                           // No padding
       "\x00\x00\x00\x00"; // Termination tuple
   ExpectExtractError(
       DebugArangesSecRaw,
       "address range table at offset 0x0 has unsupported address size: 2 "
       "(4 and 8 supported)");
 }
 
+TEST(DWARFDebugArangeSet, UnsupportedSegmentSelectorSize) {
+  static const char DebugArangesSecRaw[] =
+      "\x14\x00\x00\x00" // Length
+      "\x02\x00"         // Version
+      "\x00\x00\x00\x00" // Debug Info Offset
+      "\x04"             // Address Size
+      "\x04"             // Segment Selector Size (not supported)
+                         // No padding
+      "\x00\x00\x00\x00" // Termination tuple
+      "\x00\x00\x00\x00"
+      "\x00\x00\x00\x00";
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "non-zero segment selector size in address range table at offset 0x0 "
+      "is not supported");
+}
+
 TEST(DWARFDebugArangeSet, NoTerminationEntry) {
   static const char DebugArangesSecRaw[] =
       "\x14\x00\x00\x00" // Length
       "\x02\x00"         // Version
       "\x00\x00\x00\x00" // Debug Info Offset
       "\x04"             // Address Size
       "\x00"             // Segment Selector Size
       "\x00\x00\x00\x00" // Padding
       "\x00\x00\x00\x00" // Entry: Address
       "\x01\x00\x00\x00" //        Length
       ;                  // No termination tuple
   ExpectExtractError(
       DebugArangesSecRaw,
       "address range table at offset 0x0 is not terminated by null entry");
 }
 
 TEST(DWARFDebugArangeSet, ReservedUnitLength) {
-  static const char DebugArangesSecRaw[] =
+  // Note: 12 is the minimum length to pass the basic check for the size of
+  // the section. 1 will be automatically subtracted in ExpectExtractError().
+  static const char DebugArangesSecRaw[12 + 1] =
       "\xf0\xff\xff\xff"; // Reserved unit length value
   ExpectExtractError(
       DebugArangesSecRaw,
       "address range table at offset 0x0 has unsupported reserved unit length "
       "of value 0xfffffff0");
 }
 
+TEST(DWARFDebugArangeSet, SectionTooShort) {
+  // Note: 1 will be automatically subtracted in ExpectExtractError().
+  static const char DebugArangesSecRaw[11 + 1] = {0};
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "section is not large enough to contain an address range table "
+      "at offset 0x0");
+}
+
+TEST(DWARFDebugArangeSet, SectionTooShortDWARF64) {
+  // Note: 1 will be automatically subtracted in ExpectExtractError().
+  static const char DebugArangesSecRaw[23 + 1] =
+      "\xff\xff\xff\xff"; // DWARF64 mark
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "section is not large enough to contain a DWARF64 address range table "
+      "at offset 0x0");
+}
+
+TEST(DWARFDebugArangeSet, NoSpaceForEntries) {
+  static const char DebugArangesSecRaw[] =
+      "\x0c\x00\x00\x00" // Length
+      "\x02\x00"         // Version
+      "\x00\x00\x00\x00" // Debug Info Offset
+      "\x04"             // Address Size
+      "\x00"             // Segment Selector Size
+      "\x00\x00\x00\x00" // Padding
+      ;                  // No entries
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "address range table at offset 0x0 has an insufficient length "
+      "to contain any entries");
+}
+
+TEST(DWARFDebugArangeSet, UnevenLength) {
+  static const char DebugArangesSecRaw[] =
+      "\x1b\x00\x00\x00" // Length (not a multiple of tuple size)
+      "\x02\x00"         // Version
+      "\x00\x00\x00\x00" // Debug Info Offset
+      "\x04"             // Address Size
+      "\x00"             // Segment Selector Size
+      "\x00\x00\x00\x00" // Padding
+      "\x00\x00\x00\x00" // Entry: Address
+      "\x01\x00\x00\x00" //        Length
+      "\x00\x00\x00\x00" // Termination tuple
+      "\x00\x00\x00\x00";
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "address range table at offset 0x0 has length that is not a multiple "
+      "of the tuple size");
+}
+
+TEST(DWARFDebugArangeSet, ZeroLengthEntry) {
+  static const char DebugArangesSecRaw[] =
+      "\x24\x00\x00\x00" // Length
+      "\x02\x00"         // Version
+      "\x00\x00\x00\x00" // Debug Info Offset
+      "\x04"             // Address Size
+      "\x00"             // Segment Selector Size
+      "\x00\x00\x00\x00" // Padding
+      "\x00\x00\x00\x00" // Entry1: Address
+      "\x01\x00\x00\x00" //         Length
+      "\x01\x00\x00\x00" // Entry2: Address
+      "\x00\x00\x00\x00" //         Length (invalid)
+      "\x00\x00\x00\x00" // Termination tuple
+      "\x00\x00\x00\x00";
+  ExpectExtractError(
+      DebugArangesSecRaw,
+      "address range table at offset 0x0 has an invalid tuple (length = 0) "
+      "at offset 0x18");
+}
+
 } // end anonymous namespace