[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

129–133

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

154–158

@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

154–158

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

154–158

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

154–158

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,
                              "address range table at offset 0x%" PRIx64
                              " has invalid length",
                              Offset);
   if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
     return createStringError(errc::invalid_argument,
                              "address range table at offset 0x%" PRIx64
                              " has invalid address size",
                              Offset);
+  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;
   const uint32_t tuple_size = HeaderData.AddrSize * 2;
   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.
+  // As each tuple begins at an offset that is a multiple of the tuple size,
+  // the full length should also be a multiple of that.
+  if (full_length <= first_tuple_offset || full_length % tuple_size != 0)
+    return createStringError(errc::invalid_argument,
+                             "address range table at offset 0x%" PRIx64
+                             " has invalid length",
+                             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/test/DebugInfo/X86/dwarfdump-debug-aranges-error-cases.s

 # RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE4=0 -o - | \
 # RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
 # RUN:  FileCheck %s --check-prefix=CHECK4
 
 # RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE5=0 -o - | \
 # RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
 # RUN:  FileCheck %s --check-prefix=CHECK5
 
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE6=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK6
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE7=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK7
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE8=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK8
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE9=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK9
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE10=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK10
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj --defsym CASE11=0 -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=CHECK11
+
     .section .debug_aranges,"",@progbits
 .ifdef CASE1
 # CHECK1: address range table at offset 0x0 has invalid length
 .Llen:
     .long   .Lend - .Lversion + 1   # The length exceeds the section boundaries
 .Lversion:
     .short  2                       # Version
     .long   0                       # Debug Info Offset
.Ltuples:
     .long   0, 1                    # Address and length
     .long   0, 0                    # Termination tuple
 .Lend:
 .endif # CASE4
 
 .ifdef CASE5
 # CHECK5: address range table at offset 0x0 has unsupported reserved unit length of value 0xfffffff0
     .long   0xfffffff0              # Reserved unit length value
+    .space  40                      # Add some space to pass the basic check for the section size
 .endif # CASE5
+
+.ifdef CASE6
+# CHECK6: section is not large enough to contain an address range table at offset 0x0
+    .space  11
+.endif # CASE6
+
+.ifdef CASE7
+# CHECK7: section is not large enough to contain a DWARF64 address range table at offset 0x0
+    .long   0xffffffff              # DWARF64 mark
+    .space  19
+.endif # CASE7
+
+.ifdef CASE8
+# CHECK8: non-zero segment selector size in address range table at offset 0x0 is not supported
+.Llen:
+    .long   .Lend - .Lversion       # Length
+.Lversion:
+    .short  2                       # Version
+    .long   0                       # Debug Info Offset
+    .byte   4                       # Address Size
+    .byte   4                       # Segment Selector Size (not supported)
+    .space  11 - (. - .Llen + 11) % 12 # Padding
+.Ltuples:
+    .long   0, 0, 1                 # Segment selector, address and length
+    .long   0, 0, 0                 # Termination tuple
+.Lend:
+.endif # CASE8
+
+.ifdef CASE9
+# CHECK9: address range table at offset 0x0 has invalid length
+.Llen:
+    .long   .Lend - .Lversion       # Length (too short)
+.Lversion:
+    .short  2                       # Version
+    .long   0                       # Debug Info Offset
+    .byte   4                       # Address Size
+    .byte   0                       # Segment Selector Size
+    .space  7 - (. - .Llen + 7) % 8 # Padding
+.Lend:
+.endif # CASE9
+
+.ifdef CASE10
+# CHECK10: address range table at offset 0x0 has invalid length
+.Llen:
+    .long   .Lend - .Lversion - 1   # Length (not a multiple of tuple size)
+.Lversion:
+    .short  2                       # Version
+    .long   0                       # Debug Info Offset
+    .byte   4                       # Address Size
+    .byte   0                       # Segment Selector Size
+    .space  7 - (. - .Llen + 7) % 8 # Padding
+.Ltuples:
+    .long   0, 1                    # Address and length
+    .long   0, 0                    # Termination tuple
+.Lend:
+.endif # CASE10
+
+.ifdef CASE11
+# CHECK11: address range table at offset 0x0 has an invalid tuple (length = 0) at offset 0x18
+.Llen:
+    .long   .Lend - .Lversion       # Length
+.Lversion:
+    .short  2                       # Version
+    .long   0                       # Debug Info Offset
+    .byte   4                       # Address Size
+    .byte   0                       # Segment Selector Size
+    .space  7 - (. - .Llen + 7) % 8 # Padding
+.Ltuples:
+    .long   0, 1                    # Address and length
+    .long   1, 0                    # Address and invalid length
+    .long   0, 0                    # Termination tuple
+.Lend:
+.endif # CASE11