[DebugInfo] Add error checking around data extraction in DWARFAbbreviationDeclaration::extract

[bulbazord]

Summary

In trying to hoist errors further up this callstack, I discovered that
if the data in the debug_abbrev section is invalid entirely, the code
that parses the debug_abbrev section may do strange or unpredictable
things. The underlying issue is that DataExtractor will return a value
of 0 when it encounters an error in extracting a LEB128 value. It's thus
difficult to determine if there was an error just by looking at the
return value. This patch aims to bail at the first sight of an error in
the debug_abbrev parsing code.

[jhenderson]

There are five new places that the function you've changed can bail out with an error, but you've only got one test. I think it makes sense to test the other new places too?

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

47

Might be worth adding an assert that Size is a multiple of 4?

129–130

Nit: it seems unnecessary to have this over two lines?

Is it also worth checking that Offset is some specific value?

[fdeazeve]

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

45

Small nit but it would be helpful for readers if we improve the naming here a bit. There is nothing inherently "Bad" or inherently "Abbreviation" in the function, this is a form of write_n algorithm.

Is it bad because we always write an odd number of elements? Or is it bad because we never have the proper zero terminators? Or because the contents of Data, when splat Size times, would fail to be decoded as an ULEB? I think the answer to this is related to @jhenderson's suggestion for different tests

In all those cases, my suggestion would be to rename this to writeValueNTimes and then on the callee side we name the data/size to BadData and BadSize and maybe add a comment on why it's bad

[bulbazord]

• Add more rigorous testing. Specifically, I'm testing every possible place where we we can bail early from DWARFAbbreviationDeclaration::extract.
• Replace the writeBadAbbreviationDeclarationData with more specific functions to handle both malformed input and input that is too large (both kinds of LEB128 extraction errors).

[fdeazeve]

The test looks great!

[bulbazord]

Looks like some lld-macho tests are failing... I'm looking at them and the debug_abbrev data in them looks malformed. I'll update those tests here as well.

[bulbazord]

Update lld-macho tests

[bulbazord]

@int3 Hey can you take a look at the lld tests to make sure I didn't make a mistake? The debug_abbrev sections were malformed, I've manually added the necessary terminating 0s into the tests. They do pass on my machine after the change.

[bulbazord]

Updating for pre-submission checks

[int3]

Seems reasonable, I probably deleted those bytes when trying to make a 'minimal' test case

[bulbazord]

Re-applied as 94935c0d9ac89263dd0d7ce6bfb6f700c0f10fa2

lld/test/MachO/stabs-dwarf5.s

Lsection_abbrev:
   .byte  6                       ## DW_FORM_data4
   .byte  3                       ## DW_AT_name
   .byte  37                      ## DW_FORM_strx1
   .byte  27                      ## DW_AT_comp_dir
   .byte  37                      ## DW_FORM_strx1
   .byte  114                     ## DW_AT_str_offsets_base
   .byte  23                      ## DW_FORM_sec_offset
   .byte  0                       ## EOM(1)
+  .byte  0                       ## EOM(2)
+  .byte  0                       ## EOM(3)
 .section  __DWARF,__debug_info,regular,debug
 .set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
   .long  Lset0
 Ldebug_info_start0:
   .short  5                       ## DWARF version number
   .byte   1                       ## DWARF Unit Type
   .byte   8                       ## Address Size (in bytes)
 .set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section

lld/test/MachO/stabs-icf.s

Lsection_abbrev:
   .byte  14                      ## DW_FORM_strp
   .byte  27                      ## DW_AT_comp_dir
   .byte  14                      ## DW_FORM_strp
   .byte  17                      ## DW_AT_low_pc
   .byte  1                       ## DW_FORM_addr
   .byte  18                      ## DW_AT_high_pc
   .byte  6                       ## DW_FORM_data4
   .byte  0                       ## EOM(1)
+  .byte  0                       ## EOM(2)
+  .byte  0                       ## EOM(3)
 .section  __DWARF,__debug_info,regular,debug
 .set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
   .long  Lset0
 Ldebug_info_start0:
   .short  4                       ## DWARF version number
 .set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
   .long  Lset1
   .byte  8                       ## Address Size (in bytes)
   .byte  1                       ## Abbrev [1] 0xb:0x48 DW_TAG_compile_unit
   .long  0                       ## DW_AT_name
   .long  9                       ## DW_AT_comp_dir
   .quad  Lfunc_begin0            ## DW_AT_low_pc
 .set Lset3, Lfunc_end0-Lfunc_begin0     ## DW_AT_high_pc
   .long  Lset3
   .byte  0                       ## End Of Children Mark
 Ldebug_info_end0:

lld/test/MachO/stabs.s

Lsection_abbrev:
   .byte  14                      ## DW_FORM_strp
   .byte  27                      ## DW_AT_comp_dir
   .byte  14                      ## DW_FORM_strp
   .byte  17                      ## DW_AT_low_pc
   .byte  1                       ## DW_FORM_addr
   .byte  18                      ## DW_AT_high_pc
   .byte  6                       ## DW_FORM_data4
   .byte  0                       ## EOM(1)
+  .byte  0                       ## EOM(2)
+  .byte  0                       ## EOM(3)
 .section  __DWARF,__debug_info,regular,debug
 .set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
   .long  Lset0
 Ldebug_info_start0:
   .short  4                       ## DWARF version number
 .set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
   .long  Lset1
   .byte  8                       ## Address Size (in bytes)
Lsection_abbrev:
   .byte  14                      ## DW_FORM_strp
   .byte  27                      ## DW_AT_comp_dir
   .byte  14                      ## DW_FORM_strp
   .byte  17                      ## DW_AT_low_pc
   .byte  1                       ## DW_FORM_addr
   .byte  18                      ## DW_AT_high_pc
   .byte  6                       ## DW_FORM_data4
   .byte  0                       ## EOM(1)
+  .byte  0                       ## EOM(2)
+  .byte  0                       ## EOM(3)
 .section  __DWARF,__debug_info,regular,debug
 .set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
   .long  Lset0
 Ldebug_info_start0:
   .short  4                       ## DWARF version number
 .set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
   .long  Lset1
   .byte  8                       ## Address Size (in bytes)

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

 DWARFAbbreviationDeclaration::DWARFAbbreviationDeclaration() {
   clear();
 }
 
 llvm::Expected<DWARFAbbreviationDeclaration::ExtractState>
 DWARFAbbreviationDeclaration::extract(DataExtractor Data, uint64_t *OffsetPtr) {
   clear();
   const uint64_t Offset = *OffsetPtr;
-  Code = Data.getULEB128(OffsetPtr);
+  Error Err = Error::success();
+  Code = Data.getULEB128(OffsetPtr, &Err);
+  if (Err)
+    return Err;
+
   if (Code == 0)
     return ExtractState::Complete;
 
   CodeByteSize = *OffsetPtr - Offset;
-  Tag = static_cast<llvm::dwarf::Tag>(Data.getULEB128(OffsetPtr));
+  Tag = static_cast<llvm::dwarf::Tag>(Data.getULEB128(OffsetPtr, &Err));
+  if (Err)
+    return Err;
+
   if (Tag == DW_TAG_null) {
     clear();
     return make_error<llvm::object::GenericBinaryError>(
         "abbreviation declaration requires a non-null tag");
   }
-  uint8_t ChildrenByte = Data.getU8(OffsetPtr);
+  uint8_t ChildrenByte = Data.getU8(OffsetPtr, &Err);
+  if (Err)
+    return Err;
+
   HasChildren = (ChildrenByte == DW_CHILDREN_yes);
   // Assign a value to our optional FixedAttributeSize member variable. If
   // this member variable still has a value after the while loop below, then
   // all attribute data in this abbreviation declaration has a fixed byte size.
   FixedAttributeSize = FixedSizeInfo();
 
   // Read all of the abbreviation attributes and forms.
   while (Data.isValidOffset(*OffsetPtr)) {
-    auto A = static_cast<Attribute>(Data.getULEB128(OffsetPtr));
-    auto F = static_cast<Form>(Data.getULEB128(OffsetPtr));
+    auto A = static_cast<Attribute>(Data.getULEB128(OffsetPtr, &Err));
+    if (Err)
+      return Err;
+
+    auto F = static_cast<Form>(Data.getULEB128(OffsetPtr, &Err));
+    if (Err)
+      return Err;
 
     // We successfully reached the end of this abbreviation declaration
     // since both attribute and form are zero. There may be more abbreviation
     // declarations afterwards.
     if (!A && !F)
       return ExtractState::MoreItems;
 
     if (!A || !F) {

llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp

void writeAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
   encodeULEB128(DW_TAG_subprogram, OS);
   OS << static_cast<uint8_t>(DW_CHILDREN_no);
   encodeULEB128(DW_AT_name, OS);
   encodeULEB128(DW_FORM_strp, OS);
   encodeULEB128(0, OS);
   encodeULEB128(0, OS);
 }
 
+void writeMalformedULEB128Value(raw_ostream &OS) {
+  OS << static_cast<uint8_t>(0x80);
+}
+
+void writeULEB128LargerThan64Bits(raw_ostream &OS) {
+  static constexpr llvm::StringRef LargeULEB128 =
+      "\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x01";
+  OS << LargeULEB128;
+}
+
 TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccess) {
   SmallString<64> RawData;
   raw_svector_ostream OS(RawData);
   uint32_t FirstCode = 5;
 
   writeAbbreviationDeclarations(OS, FirstCode, InOrder);
   encodeULEB128(0, OS);
 
TEST(DWARFDebugAbbrevTest, DWARFAbbrevDeclSetExtractSuccessOutOfOrder) {
 
   const DWARFAbbreviationDeclaration *Abbrev1 =
       AbbrevSet.getAbbreviationDeclaration(FirstCode - 1);
   ASSERT_TRUE(Abbrev1);
   EXPECT_EQ(Abbrev1->getTag(), DW_TAG_subprogram);
   EXPECT_FALSE(Abbrev1->hasChildren());
   EXPECT_EQ(Abbrev1->getNumAttributes(), 1u);
 }
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbreviationDeclSetCodeExtractionError) {
+  SmallString<64> RawData;
+
+  // Check for malformed ULEB128.
+  {
+    raw_svector_ostream OS(RawData);
+    writeMalformedULEB128Value(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_FALSE(AbbrevSet.extract(Data, &Offset));
+    EXPECT_EQ(Offset, 0u);
+  }
+
+  RawData.clear();
+  // Check for ULEB128 too large to fit into a uin64_t.
+  {
+    raw_svector_ostream OS(RawData);
+    writeULEB128LargerThan64Bits(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_FALSE(AbbrevSet.extract(Data, &Offset));
+    EXPECT_EQ(Offset, 0u);
+  }
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbreviationDeclSetTagExtractionError) {
+  SmallString<64> RawData;
+  const uint32_t Code = 1;
+
+  // Check for malformed ULEB128.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    writeMalformedULEB128Value(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // Only the code was extracted correctly.
+    EXPECT_EQ(Offset, 1u);
+  }
+
+  RawData.clear();
+  // Check for ULEB128 too large to fit into a uint64_t.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    writeULEB128LargerThan64Bits(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // Only the code was extracted correctly.
+    EXPECT_EQ(Offset, 1u);
+  }
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbreviatioDeclSetChildExtractionError) {
+  SmallString<64> RawData;
+  const uint32_t Code = 1;
+  const dwarf::Tag Tag = DW_TAG_compile_unit;
+
+  // We want to make sure that we fail if we reach the end of the stream before
+  // reading the 'children' byte.
+  raw_svector_ostream OS(RawData);
+  encodeULEB128(Code, OS);
+  encodeULEB128(Tag, OS);
+
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
+  EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+  // The code and the tag were extracted correctly.
+  EXPECT_EQ(Offset, 2u);
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbreviationDeclSetAttributeExtractionError) {
+  SmallString<64> RawData;
+  const uint32_t Code = 1;
+  const dwarf::Tag Tag = DW_TAG_compile_unit;
+  const uint8_t Children = DW_CHILDREN_yes;
+
+  // Check for malformed ULEB128.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    encodeULEB128(Tag, OS);
+    OS << Children;
+    writeMalformedULEB128Value(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // The code, tag, and child byte were extracted correctly.
+    EXPECT_EQ(Offset, 3u);
+  }
+
+  RawData.clear();
+  // Check for ULEB128 too large to fit into a uint64_t.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    encodeULEB128(Tag, OS);
+    OS << Children;
+    writeULEB128LargerThan64Bits(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // The code, tag, and child byte were extracted correctly.
+    EXPECT_EQ(Offset, 3u);
+  }
+}
+
+TEST(DWARFDebugAbbrevTest, DWARFAbbreviationDeclSetFormExtractionError) {
+  SmallString<64> RawData;
+  const uint32_t Code = 1;
+  const dwarf::Tag Tag = DW_TAG_compile_unit;
+  const uint8_t Children = DW_CHILDREN_yes;
+  const dwarf::Attribute Attr = DW_AT_name;
+
+  // Check for malformed ULEB128.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    encodeULEB128(Tag, OS);
+    OS << Children;
+    encodeULEB128(Attr, OS);
+    writeMalformedULEB128Value(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // The code, tag, child byte, and first attribute were extracted correctly.
+    EXPECT_EQ(Offset, 4u);
+  }
+
+  RawData.clear();
+  // Check for ULEB128 too large to fit into a uint64_t.
+  {
+    raw_svector_ostream OS(RawData);
+    encodeULEB128(Code, OS);
+    encodeULEB128(Tag, OS);
+    OS << Children;
+    encodeULEB128(Attr, OS);
+    writeULEB128LargerThan64Bits(OS);
+
+    uint64_t Offset = 0;
+    DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(uint64_t));
+    DWARFAbbreviationDeclarationSet AbbrevSet;
+    EXPECT_TRUE(AbbrevSet.extract(Data, &Offset));
+    // The code, tag, child byte, and first attribute were extracted correctly.
+    EXPECT_EQ(Offset, 4u);
+  }
+}