Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb][llvm] Return an error instead of crashing when parsing a line table prologue. #80769

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

clayborg
Copy link
Collaborator

@clayborg clayborg commented Feb 6, 2024

We recently ran into some bad DWARF where the DW_AT_stmt_list of many compile units was randomly set to invalid values and was causing LLDB to crash due to an assertion about address sizes not matching. Instead of asserting, we should return an appropriate recoverable llvm::Error.

…gue.

We recently ran into some bad DWARF where the DW_AT_stmt_list of many compile units was randomly set to invalid values and was causing LLDB to crash due to an assertion. Instead of asserting, we should return an appropriate llvm::Error.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 6, 2024

@llvm/pr-subscribers-debuginfo

Author: Greg Clayton (clayborg)

Changes

We recently ran into some bad DWARF where the DW_AT_stmt_list of many compile units was randomly set to invalid values and was causing LLDB to crash due to an assertion about address sizes not matching. Instead of asserting, we should return an appropriate llvm::Error. There was also a complication as the address size might be invalid or the cursor might have ran into the end of the .debug_line section. To handle this I added a new lambda that will return the correct error if the cursor is still in a valid state, or return the cursor error if the cursor has one.


Full diff: https://github.com/llvm/llvm-project/pull/80769.diff

2 Files Affected:

  • (modified) llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp (+37-7)
  • (modified) llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp (+4-9)
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
index 28f05644a3aa1..60513f09936aa 100644
--- a/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
+++ b/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
@@ -372,26 +372,56 @@ Error DWARFDebugLine::Prologue::parse(
   std::tie(TotalLength, FormParams.Format) =
       DebugLineData.getInitialLength(Cursor);
 
+  // Report an error, but also make sure the cursor is not in a bad state. If
+  // the cursor is in a bad state, return its error instead. Many times during
+  // parsing we might end up bailing if we don't like what we see, but often
+  // times when we decode items with a bad cursor, we will get zero back which
+  // might not be valid data to extract.
+  auto ReportErrorOrCursor = [&](llvm::Error Error) -> llvm::Error {
+    if (Cursor)
+      return Error;
+    llvm::consumeError(std::move(Error));
+    *OffsetPtr = Cursor.tell();
+    std::string CursorErrorStr = llvm::toString(Cursor.takeError());
+    return createStringError(
+        errc::not_supported,
+        "parsing line table prologue at offset 0x%8.8" PRIx64 ": %s",
+        PrologueOffset, CursorErrorStr.c_str());
+  };
+
   DebugLineData =
       DWARFDataExtractor(DebugLineData, Cursor.tell() + TotalLength);
   FormParams.Version = DebugLineData.getU16(Cursor);
-  if (Cursor && !versionIsSupported(getVersion())) {
+  if (!versionIsSupported(getVersion())) {
     // Treat this error as unrecoverable - we cannot be sure what any of
     // the data represents including the length field, so cannot skip it or make
     // any reasonable assumptions.
-    *OffsetPtr = Cursor.tell();
-    return createStringError(
+    return ReportErrorOrCursor(createStringError(
         errc::not_supported,
         "parsing line table prologue at offset 0x%8.8" PRIx64
         ": unsupported version %" PRIu16,
-        PrologueOffset, getVersion());
+        PrologueOffset, getVersion()));
   }
 
   if (getVersion() >= 5) {
     FormParams.AddrSize = DebugLineData.getU8(Cursor);
-    assert((!Cursor || DebugLineData.getAddressSize() == 0 ||
-            DebugLineData.getAddressSize() == getAddressSize()) &&
-           "Line table header and data extractor disagree");
+    const uint8_t DataAddrSize = DebugLineData.getAddressSize();
+    const uint8_t PrologueAddrSize = FormParams.AddrSize;
+    if (DataAddrSize == 0) {
+      if (PrologueAddrSize != 4 && PrologueAddrSize != 8) {
+        return ReportErrorOrCursor(createStringError(
+            errc::not_supported,
+            "parsing line table prologue at offset 0x%8.8" PRIx64
+            ": invalid address size %" PRIu8, PrologueOffset,
+            PrologueAddrSize));
+      }
+    } else if (DataAddrSize != getAddressSize()) {
+      return ReportErrorOrCursor(createStringError(
+          errc::not_supported,
+          "parsing line table prologue at offset 0x%8.8" PRIx64 ": address "
+          "size %" PRIu8 " doesn't match architecture address size %" PRIu8,
+          PrologueOffset, PrologueAddrSize, DataAddrSize));
+    }
     SegSelectorSize = DebugLineData.getU8(Cursor);
   }
 
diff --git a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
index d42a626fa9c1c..d7d04312a1c4c 100644
--- a/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ b/llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -821,15 +821,10 @@ TEST_F(DebugLineBasicFixture, ErrorForUnsupportedAddressSizeDefinedInHeader) {
 
   auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
                                                     nullptr, RecordRecoverable);
-  EXPECT_THAT_ERROR(
-      std::move(Recoverable),
-      FailedWithMessage("address size 0x09 of DW_LNE_set_address opcode at "
-                        "offset 0x00000038 is unsupported"));
-  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
-  ASSERT_EQ((*ExpectedLineTable)->Rows.size(), 3u);
-  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1u);
-  // Show that the set address opcode is ignored in this case.
-  EXPECT_EQ((*ExpectedLineTable)->Rows[0].Address.Address, 0u);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable,
+                       FailedWithMessage("parsing line table prologue at "
+                                         "offset 0x00000000: invalid address "
+                                         "size 9"));
 }
 
 TEST_F(DebugLineBasicFixture, CallbackUsedForUnterminatedSequence) {

Copy link

github-actions bot commented Feb 6, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment on lines 406 to 426
if (getVersion() >= 5) {
FormParams.AddrSize = DebugLineData.getU8(Cursor);
assert((!Cursor || DebugLineData.getAddressSize() == 0 ||
DebugLineData.getAddressSize() == getAddressSize()) &&
"Line table header and data extractor disagree");
const uint8_t DataAddrSize = DebugLineData.getAddressSize();
const uint8_t PrologueAddrSize = FormParams.AddrSize;
if (DataAddrSize == 0) {
if (PrologueAddrSize != 4 && PrologueAddrSize != 8) {
return ReportErrorOrCursor(createStringError(
errc::not_supported,
"parsing line table prologue at offset 0x%8.8" PRIx64
": invalid address size %" PRIu8, PrologueOffset,
PrologueAddrSize));
}
} else if (DataAddrSize != getAddressSize()) {
return ReportErrorOrCursor(createStringError(
errc::not_supported,
"parsing line table prologue at offset 0x%8.8" PRIx64 ": address "
"size %" PRIu8 " doesn't match architecture address size %" PRIu8,
PrologueOffset, PrologueAddrSize, DataAddrSize));
}
SegSelectorSize = DebugLineData.getU8(Cursor);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be recoverable errors, because we can skip this line table and read the next one.

@clayborg
Copy link
Collaborator Author

I have made the prologue errors recoverable. Anything else needed here?

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@clayborg clayborg merged commit a23d4ce into llvm:main Feb 22, 2024
3 of 4 checks passed
@clayborg clayborg deleted the invalid-stmt branch February 22, 2024 18:25
PrologueOffset, PrologueAddrSize));
}
} else if (DataAddrSize != PrologueAddrSize) {
RecoverableErrorHandler(createStringError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this path isn't tested? Could you add a test?

@@ -823,7 +823,9 @@ TEST_F(DebugLineBasicFixture, ErrorForUnsupportedAddressSizeDefinedInHeader) {
nullptr, RecordRecoverable);
EXPECT_THAT_ERROR(
std::move(Recoverable),
FailedWithMessage("address size 0x09 of DW_LNE_set_address opcode at "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this test change removed coverage for a different error message, by failing earlier?

This test should probably be restored by modifying it to be reachable, or the original code's error handling should be turned into an assertion if the diagnostic is now an invariant for the code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants