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

[Pavel Labath via Phabricator via llvm-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:173-192
+  Error Err = Error::success();
+  while (true) {
+    StringRef S = DebugLineData.getCStrRef(OffsetPtr, &Err);
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     if (S.empty()) {
       Terminated = true;
----------------
jhenderson wrote:
> dblaikie wrote:
> > 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)
> 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.
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:207-209
+    FileEntry.DirIdx = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.ModTime = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.Length = DebugLineData.getULEB128(OffsetPtr, &Err);
----------------
jhenderson wrote:
> Are there separate test cases for each of these being truncated?
There are now.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:427-429
+    // Reset the offset to the end of the prologue to prevent the length check
+    // below from executing.
+    *OffsetPtr = EndPrologueOffset;
----------------
dblaikie wrote:
> 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?
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:441
 
   if (*OffsetPtr != EndPrologueOffset) {
     RecoverableErrorHandler(createStringError(
----------------
dblaikie wrote:
> 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.
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.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:80-84
+# NONFATAL:      file_names[  1]:
+# NONFATAL-NEXT:            name: "file1"
+# NONFATAL-NEXT:       dir_index: 0
+# NONFATAL-NEXT:        mod_time: 0x00000000
+# NONFATAL-NEXT:          length: 0x00000000
----------------
jhenderson wrote:
> Why did this change?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77555/new/

https://reviews.llvm.org/D77555