[DebugInfo] Add checks for v2 directory and file name table terminators

[jhenderson]

Summary

The DWARFv2-4 specification for the line table header states that the include directories and file name tables both end with a single null byte. Prior to this change, the parser did not detect if this byte was missing, because it also stopped reading the tables once it reached the prologue end, as claimed by the header_length field. This change adds a check that the terminator has been seen at the end of each table.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

165–171

Perhaps this'd be a little more terse (but not necessarily better) written as:

Terminated = S.empty();
if (Terminated)
  break;

or maybe

if ((Terminated = S.empty()))
  break;

Not necessary if you prefer the current version.

I'd probably more strongly suggest making the variable names a bit simpler - probably just using "Terminated" for both.

178

maybe make it a bit more explicit by saying "null terminated"?

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

165–171

Terminated +1

The variable IncludeDirectories below suggests what Terminated means.

Both the current form and

if ((Terminated = S.empty()))
  break;

look fine to me.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

165–171

I think I marginally prefer the current form for readability reasons, so I'll leave that as is. The renaming makese sense though. I considered making that change before putting it up for review.

178

That's fine with me, although I should point out that the strings themselves are always null terminated, so even if the null terminator byte is missing, the table will still have a null byte at the end.

[jhenderson]

Rebase and fixed a unit test.

[jhenderson]

Remove accidental changes to unit test caused by bad rebase/merge.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

for (uint32_t I = 0; I != FileNames.size(); ++I) {
         FileEntry.Source.dump(OS, DumpOptions);
         OS << '\n';
       }
     }
   }
 }
 
 // Parse v2-v4 directory and file tables.
-static void
+static Error
 parseV2DirFileTables(const DWARFDataExtractor &DebugLineData,
                      uint64_t *OffsetPtr, uint64_t EndPrologueOffset,
                      DWARFDebugLine::ContentTypeTracker &ContentTypes,
                      std::vector<DWARFFormValue> &IncludeDirectories,
                      std::vector<DWARFDebugLine::FileNameEntry> &FileNames) {
+  bool Terminated = false;
   while (*OffsetPtr < EndPrologueOffset) {
     StringRef S = DebugLineData.getCStrRef(OffsetPtr);
-    if (S.empty())
+    if (S.empty()) {
+      Terminated = true;
       break;
+    }
     DWARFFormValue Dir =
         DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, S.data());
     IncludeDirectories.push_back(Dir);
   }
 
+  if (!Terminated)
+    return createStringError(errc::invalid_argument,
+                             "include directories table was not null "
+                             "terminated before the end of the prologue");
+
+  Terminated = false;
   while (*OffsetPtr < EndPrologueOffset) {
     StringRef Name = DebugLineData.getCStrRef(OffsetPtr);
-    if (Name.empty())
+    if (Name.empty()) {
+      Terminated = true;
       break;
+    }
     DWARFDebugLine::FileNameEntry FileEntry;
     FileEntry.Name =
         DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, Name.data());
     FileEntry.DirIdx = DebugLineData.getULEB128(OffsetPtr);
     FileEntry.ModTime = DebugLineData.getULEB128(OffsetPtr);
     FileEntry.Length = DebugLineData.getULEB128(OffsetPtr);
     FileNames.push_back(FileEntry);
   }
 
   ContentTypes.HasModTime = true;
   ContentTypes.HasLength = true;
+
+  if (!Terminated)
+    return createStringError(errc::invalid_argument,
+                             "file names table was not null terminated before "
+                             "the end of the prologue");
+
+  return Error::success();
 }
 
 // Parse v5 directory/file entry content descriptions.
 // Returns the descriptors, or an error if we did not find a path or ran off
 // the end of the prologue.
 static llvm::Expected<ContentDescriptors>
 parseV5EntryFormat(const DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
                    DWARFDebugLine::ContentTypeTracker *ContentTypes) {
Error DWARFDebugLine::Prologue::parse(
   } else {
     StandardOpcodeLengths.reserve(OpcodeBase - 1);
     for (uint32_t I = 1; I < OpcodeBase; ++I) {
       uint8_t OpLen = DebugLineData.getU8(OffsetPtr);
       StandardOpcodeLengths.push_back(OpLen);
     }
   }
 
-  if (getVersion() >= 5) {
-    if (Error E =
-            parseV5DirFileTables(DebugLineData, OffsetPtr, FormParams, Ctx, U,
-                                 ContentTypes, IncludeDirectories, FileNames)) {
-      RecoverableErrorHandler(joinErrors(
-          createStringError(
-              errc::invalid_argument,
-              "parsing line table prologue at 0x%8.8" PRIx64
-              " found an invalid directory or file table description at"
-              " 0x%8.8" PRIx64,
-              PrologueOffset, *OffsetPtr),
-          std::move(E)));
-      // Skip to the end of the prologue, since the chances are that the parser
-      // did not read the whole table. This prevents the length check below from
-      // executing.
-      if (*OffsetPtr < EndPrologueOffset)
-        *OffsetPtr = EndPrologueOffset;
-    }
-  } else
-    parseV2DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
-                         ContentTypes, IncludeDirectories, FileNames);
+  auto ReportInvalidDirFileTable = [&](Error E) {
+  };
+  if (getVersion() >= 5) {
+    if (Error E =
+            parseV5DirFileTables(DebugLineData, OffsetPtr, FormParams, Ctx, U,
+                                 ContentTypes, IncludeDirectories, FileNames))
+      ReportInvalidDirFileTable(std::move(E));
+  } else if (Error E = parseV2DirFileTables(DebugLineData, OffsetPtr,
+                                            EndPrologueOffset, ContentTypes,
+                                            IncludeDirectories, FileNames))
+    ReportInvalidDirFileTable(std::move(E));
 
   if (*OffsetPtr != EndPrologueOffset) {
     RecoverableErrorHandler(createStringError(
         errc::invalid_argument,
         "parsing line table prologue at 0x%8.8" PRIx64
         " should have ended at 0x%8.8" PRIx64 " but it ended at 0x%8.8" PRIx64,
         PrologueOffset, EndPrologueOffset, *OffsetPtr));
     *OffsetPtr = EndPrologueOffset;

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s

 .byte   0
 .asciz "file1"          # File table
 .byte   0, 0, 0
 .asciz "file2"
 .byte   1, 2
 .Lprologue_short_prologue_end:
 .byte   6               # Read as part of the prologue,
                         # then later again as DW_LNS_negate_stmt.
-# FIXME: There should be an additional 0 byte here, but the file name parsing
-#        code does not recognise a missing null terminator.
 # Header end
 .byte   0, 9, 2         # DW_LNE_set_address
 .quad   0x1122334455667788
 .byte   0, 1, 1         # DW_LNE_end_sequence
 .Lunit_short_prologue_end:
 
 # Over-long prologue.
 .long   .Lunit_long_prologue_end - .Lunit_long_prologue_start # unit length

 .byte   0
 .Lzero_opcode_base_prologue_end:
 .byte   0, 9, 2        # DW_LNE_set_address
 .quad   0xffffeeeeddddcccc
 .byte   0x1            # Special opcode
 .byte   0, 1, 1        # DW_LNE_end_sequence
 .Lzero_opcode_base_end:
 
+# V4 table with unterminated include directory table.
+.long   .Lunterminated_include_end - .Lunterminated_include_start # unit length
+.Lunterminated_include_start:
+.short  4               # version
+.long   .Lunterminated_include_prologue_end-.Lunterminated_include_prologue_start # Length of Prologue
+.Lunterminated_include_prologue_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz  "dir1"          # Include table
+.Lunterminated_include_prologue_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0xabcdef0123456789
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunterminated_include_end:
+
+# V4 table with unterminated file name table.
+.long   .Lunterminated_files_end - .Lunterminated_files_start # unit length
+.Lunterminated_files_start:
+.short  4               # version
+.long   .Lunterminated_files_prologue_end-.Lunterminated_files_prologue_start # Length of Prologue
+.Lunterminated_files_prologue_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz  "dir1"          # Include table
+.byte   0
+.asciz  "foo.c"         # File table
+.byte   1, 2, 3
+.Lunterminated_files_prologue_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0xababcdcdefef0909
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunterminated_files_end:
+
 # Trailing good section.
 .long   .Lunit_good_end - .Lunit_good_start # Length of Unit (DWARF-32 format)
 .Lunit_good_start:
 .short  4               # DWARF version number
 .long   .Lprologue_good_end-.Lprologue_good_start # Length of Prologue
 .Lprologue_good_start:
 .byte   1               # Minimum Instruction Length
 .byte   1               # Maximum Operations per Instruction

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

 # RUN: FileCheck %s --input-file=%t-malformed-verbose.err --check-prefixes=ALL,OTHER
 
 ## We should still produce warnings for malformed tables after the specified unit.
 # RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2> %t-malformed-off-first.err \
 # RUN:   | FileCheck %s --check-prefixes=FIRST,NOLATER
 # RUN: FileCheck %s --input-file=%t-malformed-off-first.err --check-prefix=ALL
 
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x361 %t-malformed.o 2> %t-malformed-off-last.err \
+# RUN: llvm-dwarfdump -debug-line=0x3c9 %t-malformed.o 2> %t-malformed-off-last.err \
 # RUN:   | FileCheck %s --check-prefix=LAST --implicit-check-not='debug_line[{{.*}}]'
 # RUN: FileCheck %s --input-file=%t-malformed-off-last.err --check-prefix=ALL
 
 # FIRST:       debug_line[0x00000000]
 # FIRST:       0x000000000badbeef {{.*}} end_sequence
 # NOFIRST-NOT: debug_line[0x00000000]
 # NOFIRST-NOT: 0x000000000badbeef {{.*}} end_sequence
 # NOLATER-NOT: debug_line[{{.*}}]

 # NONFATAL-NEXT: file_names[  1]:
 # NONFATAL-NEXT:            name: "file1"
 # NONFATAL-NEXT:       dir_index: 1
 # NONFATAL-NEXT:        mod_time: 0x00000002
 # NONFATAL-NEXT:          length: 0x00000003
 # NONFATAL:      0xffffeeeeddddcccd 1 0 1 0 0 is_stmt{{$}}
 # NONFATAL:      0xffffeeeeddddcccd 1 0 1 0 0 is_stmt end_sequence{{$}}
 
-# LAST:          debug_line[0x00000361]
+## V4 table with unterminated include directory table.
+# NONFATAL:      debug_line[0x00000361]
+# NONFATAL-NEXT: Line table prologue
+# NONFATAL:      include_directories[  1] = "dir1"
+# NONFATAL-NOT:  file_names
+# NONFATAL:      0xabcdef0123456789 {{.*}} is_stmt end_sequence
+
+## V4 table with unterminated file name table.
+# NONFATAL:      debug_line[0x00000390]
+# NONFATAL-NEXT: Line table prologue
+# NONFATAL:      file_names[  1]:
+# NONFATAL-NEXT:            name: "foo.c"
+# NONFATAL-NEXT:       dir_index: 1
+# NONFATAL-NEXT:        mod_time: 0x00000002
+# NONFATAL-NEXT:          length: 0x00000003
+# NONFATAL-NOT:  file_names
+# NONFATAL:      0xababcdcdefef0909 {{.*}} is_stmt end_sequence
+
+# LAST:          debug_line[0x000003c9]
 # LAST:          0x00000000cafebabe {{.*}} end_sequence
 
 # RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
 
 # ALL-NOT:  warning:
 # ALL:      warning: parsing line table prologue at offset 0x00000048 found unsupported version 0
 # ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e found unsupported version 1
 # ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
 # ALL-NEXT: warning: failed to parse entry content descriptions because no path was found
+# ALL-NEXT: warning: parsing line table prologue at 0x00000081 found an invalid directory or file table description at 0x000000ba
+# ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
 # ALL-NEXT: warning: parsing line table prologue at 0x00000081 should have ended at 0x000000b9 but it ended at 0x000000ba
 # ALL-NEXT: warning: parsing line table prologue at 0x000000c8 should have ended at 0x00000103 but it ended at 0x00000102
 # OTHER-NEXT: warning: unexpected line op length at offset 0x00000158 expected 0x02 found 0x01
 # OTHER-NEXT: warning: unexpected line op length at offset 0x0000015c expected 0x01 found 0x02
 # OTHER-NEXT: warning: last sequence in debug line table at offset 0x0000016c is not terminated
 # ALL-NEXT: warning: parsing line table prologue at 0x000001b2 should have ended at 0x000001ce but it ended at 0x000001e1
 # ALL-NEXT: warning: parsing line table prologue at 0x000001ee should have ended at 0x00000219 but it ended at 0x00000220
 # ALL-NEXT: warning: parsing line table prologue at 0x0000022f should have ended at 0x00000251 but it ended at 0x0000025e
 # ALL-NEXT: warning: parsing line table prologue at 0x0000026b found an invalid directory or file table description at 0x0000029f
 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # ALL-NEXT: warning: parsing line table prologue at 0x000002ae found an invalid directory or file table description at 0x000002e0
 # ALL-NEXT: warning: failed to parse file entry because the MD5 hash is invalid
 # ALL-NEXT: warning: parsing line table prologue at 0x000002ae should have ended at 0x000002d9 but it ended at 0x000002e0
 # ALL-NEXT: warning: parsing line table prologue at 0x000002ec found an invalid directory or file table description at 0x00000315
 # ALL-NEXT: warning: failed to parse directory entry because skipping the form value failed.
 # ALL-NEXT: warning: parsing line table prologue at offset 0x00000332 found opcode base of 0. Assuming no standard opcodes
+# ALL-NEXT: warning: parsing line table prologue at 0x00000361 found an invalid directory or file table description at 0x00000382
+# ALL-NEXT: warning: include directories table was not null terminated before the end of the prologue
+# ALL-NEXT: warning: parsing line table prologue at 0x00000390 found an invalid directory or file table description at 0x000003bb
+# ALL-NEXT: warning: file names table was not null terminated before the end of the prologue
 # ALL-NOT:  warning:

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
   if (!setupGenerator(Version))
     return;
 
   SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
                (Format == DWARF64 ? "DWARF64" : "DWARF32"));
 
   LineTable &LT = Gen->addLineTable(Format);
   DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
-  // FIXME: Ideally, we'd test for 1 less than expected, but the code does not
-  // currently fail if missing only the terminator of a v2-4 file table.
-  if (Version < 5)
-    Prologue.PrologueLength -= 2;
-  else
-    Prologue.PrologueLength -= 1;
   LT.setPrologue(Prologue);
 
   generate();
 
   auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
                                                     nullptr, RecordRecoverable);
   ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
   DWARFDebugLine::LineTable Result(**ExpectedLineTable);
   // Undo the earlier modification so that it can be compared against a
   // "default" prologue.
-  if (Version < 5)
-    Result.Prologue.PrologueLength += 2;
-  else
-    Result.Prologue.PrologueLength += 1;
   checkDefaultPrologue(Version, Format, Result.Prologue, 0);
 
   uint64_t ExpectedEnd =
-      Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
-  if (Version < 5)
-    --ExpectedEnd;
-  checkError(
+      Prologue.TotalLength - 2 + Prologue.sizeofTotalLength();
+  std::vector<std::string> Errs;
+  // Parsing of a DWARFv2-4 file table stops at the end of an entry once the
+  // prologue end has been reached, whether or not the trailing null terminator
+  // has been found. As such, the expected error message will be slightly
+  // different.
+  uint64_t ActualEnd = Version == 5 ? ExpectedEnd + 2 : ExpectedEnd + 1;
+  if (Version != 5) {
+    Errs.emplace_back(
+        (Twine("parsing line table prologue at 0x00000000 found an invalid "
+               "directory or file table description at 0x000000") +
+         Twine::utohexstr(ActualEnd))
+            .str());
+    Errs.emplace_back("file names table was not null terminated before the end "
+                      "of the prologue");
+  }
+  Errs.emplace_back(
       (Twine("parsing line table prologue at 0x00000000 should have ended at "
              "0x000000") +
        Twine::utohexstr(ExpectedEnd) + " but it ended at 0x000000" +
-       Twine::utohexstr(ExpectedEnd + 1))
-          .str(),
-      std::move(Recoverable));
+       Twine::utohexstr(ActualEnd))
+          .str());
+  std::vector<StringRef> ErrRefs(Errs.begin(), Errs.end());
+  checkError(ErrRefs, std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
     LineTableTestParams, DebugLineParameterisedFixture,
     Values(std::make_pair(
                2, DWARF32), // Test lower-bound of v2-3 fields and DWARF32.
            std::make_pair(3, DWARF32), // Test upper-bound of v2-3 fields.
            std::make_pair(4, DWARF64), // Test v4 fields and DWARF64.