[DebugInfo] Add checks for v2 directory and file name table terminators
authorJames Henderson <james.henderson@sony.com>
Wed, 12 Feb 2020 14:37:10 +0000 (14:37 +0000)
committerJames Henderson <james.henderson@sony.com>
Wed, 12 Feb 2020 14:49:22 +0000 (14:49 +0000)
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.

Reviewed by: dblaikie, MaskRay

Differential Revision: https://reviews.llvm.org/D74413

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

index 2c869df..7bdc72a 100644 (file)
@@ -155,25 +155,36 @@ void DWARFDebugLine::Prologue::dump(raw_ostream &OS,
 }
 
 // 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());
@@ -185,6 +196,13 @@ parseV2DirFileTables(const DWARFDataExtractor &DebugLineData,
 
   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.
@@ -373,27 +391,30 @@ Error DWARFDebugLine::Prologue::parse(
     }
   }
 
+  auto ReportInvalidDirFileTable = [&](Error E) {
+    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;
+  };
   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);
+                                 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(
index 0477a66..090ebe6 100644 (file)
@@ -83,8 +83,6 @@
 .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
 .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:
index d4b5045..f93035a 100644 (file)
@@ -36,7 +36,7 @@
 # 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
 
 # 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-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
 # 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:
index 795b9b4..d6d9691 100644 (file)
@@ -449,12 +449,7 @@ TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
 
   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;
+  Prologue.PrologueLength -= 2;
   LT.setPrologue(Prologue);
 
   generate();
@@ -465,23 +460,34 @@ TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
   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;
+  Result.Prologue.PrologueLength += 2;
   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(