[DWARFDebugLine] Check for errors when parsing v2 file/dir lists

[labath]

Summary

Without this we could silently accept an invalid prologue because the
default DataExtractor behavior is to return an empty string when
reaching the end of file. And empty string is also used to terminate
these lists.

This makes the parsing code slightly more complicated, but this
complexity will go away once the parser starts working with truncating
data extractors. The reason I am doing it this way is because without
this, the truncation would regress the quality of error messages (right
now, we produce bad error messages only near EOF, but truncation would
make everything behave as if it was near EOF).

[dblaikie]

Looks good - some of the suggestions I made don't hold or hold less with one of your other changes applied (truncated data extractors) - but might still be worth considering even combined with that.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

173–192

I realize this is the way the existing code is - so feel free to ignore this, do it as a follow-up or precommit, or roll it is as you see fit: The "Terminated" flag here seems to complicate this code compared to doing the error handling and returning immediately in the one way out of the loop above where Terminated would be false (this would also allow you to narrow the scope of the the "Error" variable to inside the loop)

197–199

If you changed this code to be:

if (!Err && Name.empty()) {
  Terminated = true;
  break;
}

Would that let the later error handling catch handle both errors - then you could do the same as above, and roll the error handling in tighter without needing the "Terminated" flag, etc?

427–429

Rather than doing this, could we return early in these cases (eg: have ReportInvalidDirFileTable return Error::Success and have the two calls be "return ReportInvalid..." perhaps? or just two separate statements, (ReportInvalid + return Success)) so the follow-on check is never run, rather than having to create a fake state to avoid it reporting?

441

Does this only catch cases where the offset is < the end prologue offset now? If so, it might be nice to assert that it's not > and write the test as < and rephrase the error message to be more specific about the mismatch being too short, etc.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

173–192

It was a little bit of time ago since I added the Terminated flag, but IIRC the reason it needed to exist was because a missing null terminator versus running off the section end would be undetectable without it. I think it becomes redundant once the other planned changes are made, since we no longer need to check the offset when reading, and can keep going until the EOF error or a null terminator is hit.

207–209

Are there separate test cases for each of these being truncated?

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

80–84

Why did this change?

[labath]

• add tests
• remove the "Terminated" flag

[labath]

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

173–192

We can't get rid of checking for termination in a fairly convoluted way (but we will after the other patches), but we can avoid materializing the check into a Terminated variable, which is what I believe David was proposing.

207–209

There are now.

427–429

I've done that, but slightly differently -- I reorganized the code to remove the lambda and added a return success() after the error is reported.

441

That isn't fully the case yet -- there are still some situations in which the v5 parser can escape the prologue. However, it will be the case after this starts using truncated data extractors. I'll update this in the other patch.

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

80–84

This is due to the additional error checking in the parser (line 204) -- it now checks for errors before appending an entry to the list and if it detects an error (or that the offset is beyond the prologue bound) it rejects that entries.

[jhenderson]

LGTM, I think. Thanks.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 // Parse v2-v4 directory and file tables.
 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);
+  Error Err = Error::success();
+  while (true) {
+    StringRef S = DebugLineData.getCStrRef(OffsetPtr, &Err);
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     if (S.empty()) {
       Terminated = true;
       break;
     }
     DWARFFormValue Dir =
         DWARFFormValue::createFromPValue(dwarf::DW_FORM_string, S.data());
     IncludeDirectories.push_back(Dir);
   }
 
-  if (!Terminated)
+  if (!Terminated) {
+    consumeError(std::move(Err));
     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);
+  while (true) {
+    StringRef Name = DebugLineData.getCStrRef(OffsetPtr, &Err);
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     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);
+    FileEntry.DirIdx = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.ModTime = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.Length = DebugLineData.getULEB128(OffsetPtr, &Err);
+
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     FileNames.push_back(FileEntry);
   }
 
   ContentTypes.HasModTime = true;
   ContentTypes.HasLength = true;
 
-  if (!Terminated)
+  if (!Terminated) {
+    consumeError(std::move(Err));
     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>
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;
+    // Reset the offset to the end of the prologue to prevent the length check
+    // below from executing.
   };
   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;
   }
   return Error::success();

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

 # NONFATAL:      prologue_length: 0x00000013
 # NONFATAL-NOT:  include_directories
 # NONFATAL-NOT:  file_names
 # NONFATAL:      0x8877665544332211 {{.*}} end_sequence
 
 ## Prologue with length shorter than parsed.
 # NONFATAL:      debug_line[0x00000081]
 # NONFATAL-NEXT: Line table prologue
-# NONFATAL:      file_names[  2]:
-# NONFATAL-NEXT:            name: "file2"
-# NONFATAL-NEXT:       dir_index: 1
-# NONFATAL-NEXT:        mod_time: 0x00000002
-# NONFATAL-NEXT:          length: 0x00000006
+# NONFATAL:      file_names[  1]:
+# NONFATAL-NEXT:            name: "file1"
+# NONFATAL-NEXT:       dir_index: 0
+# NONFATAL-NEXT:        mod_time: 0x00000000
+# NONFATAL-NEXT:          length: 0x00000000
 # NONFATAL:      0x1122334455667788 {{.*}} 0 end_sequence{{$}}
 
 ## Prologue with length longer than parsed.
 # NONFATAL:      debug_line[0x000000c8]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      file_names[  2]:
 # NONFATAL-NEXT:            name: "file2"
 # NONFATAL-NEXT:       dir_index: 1

 
 # ALL-NOT:  warning:
 # ALL:      warning: parsing line table prologue at offset 0x00000048: unsupported version 0
 # ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e: 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: parsing line table prologue at 0x00000361 found an invalid directory or file table description at 0x00000383
 # 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: parsing line table prologue at 0x00000390 found an invalid directory or file table description at 0x000003bc
 # 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) {
   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.
-  Result.Prologue.PrologueLength += 2;
-  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
+  if (Version != 5) {
+    // Parsing will stop before reading a complete file entry.
+    ASSERT_EQ(Result.Prologue.IncludeDirectories.size(), 1u);
+    EXPECT_EQ(toStringRef(Result.Prologue.IncludeDirectories[0]), "a dir");
+    EXPECT_EQ(Result.Prologue.FileNames.size(), 0u);
+  } else {
+    // Parsing will continue past the presumed end of prologue.
+    ASSERT_EQ(Result.Prologue.FileNames.size(), 1u);
+    ASSERT_EQ(Result.Prologue.FileNames[0].Name.getForm(), DW_FORM_string);
+    ASSERT_EQ(Result.Prologue.FileNames[0].DirIdx, 0u);
+    EXPECT_EQ(toStringRef(Result.Prologue.FileNames[0].Name), "a file");
+  }
 
   uint64_t ExpectedEnd =
       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))
+         Twine::utohexstr(ExpectedEnd + 1))
             .str());
     Errs.emplace_back("file names table was not null terminated before the end "
                       "of the prologue");
-  }
+  } else {
     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(ActualEnd))
+         Twine::utohexstr(ExpectedEnd + 2))
             .str());
+  }
   EXPECT_THAT_ERROR(std::move(Recoverable),
                     FailedWithMessageArray(testing::ElementsAreArray(Errs)));
 }
 
 INSTANTIATE_TEST_CASE_P(
     LineTableTestParams, DebugLineParameterisedFixture,
     Values(std::make_pair(
                2, DWARF32), // Test lower-bound of v2-3 fields and DWARF32.