[DebugInfo] Do not hang when parsing a malformed .debug_pub* section.

[ikudrin]

Summary

The parsing method did not check reading errors and might easily fall into an infinite loop on an invalid input because of that.

[jhenderson]

lld/ELF/DWARF.h

59

Why did the types here need to change?

lld/ELF/SyntheticSections.cpp

2713

Could you put a comment here saying what the 0 means (i.e. /*Something=*/0), please?

lld/test/ELF/gdb-index-invalid-pubnames.s

5

It might make sense to extend this test to show that LLD doesn't use the warned-for pubnames section, if there's a way for that to make sense.

llvm/test/DebugInfo/X86/dwarfdump-debug-pubnames-invalid.s

1

I guess the pubnames section is fairly straightforward, but I personally feel like it would be best to write the testing for this and D83050 as gtest unit tests. That being said, a single test that shows how llvm-dwarfdump handles errors for malformed pubnames sections might make sense, although in that case I'd put it alongside the debug_line_invalid.test in the llvm-dwarfdump directory.

I would probably add basic coverage for the .debug_pubtypes and the GNU variant sections too, since they go through different code paths to get there.

[ikudrin]

lld/ELF/DWARF.h

59

This helps to have access to the sec field without casing. That is used in readPubNamesAndTypes().

lld/ELF/SyntheticSections.cpp

2713

OK, changed to config->wordsize to avoid uncertainties.

lld/test/ELF/gdb-index-invalid-pubnames.s

5

This test is solely about avoiding hanging in LLD; it should be as short and simple as possible. There are other tests for LLD to check that the information from .debug_gnu_pub* sections is passed to the index. And there are (or will be) tests for llvm-dwarfdump to check what is read from valid or malformed sections.

llvm/test/DebugInfo/X86/dwarfdump-debug-pubnames-invalid.s

1

I usually prefer lit tests, because they are much handier and easier to maintain, especially in cases like that, with just two line of source code and a few RUN/CHECK commands.

I'll move the test to the directory of llvm-dwarfdump tests and extend it to cover all four tables.

[jhenderson]

LGTM.

[dblaikie]

llvm/test/tools/llvm-dwarfdump/X86/debug_pub_tables_invalid.s

2–7 ↗

(On Diff #275403)

Might be nice if you could test all these in one go? (so it's one invocation of llvm-dwarfdump rather than 4 - save on process overhead, etc)

[ikudrin]

• Updated the test to check everything in one run.

lld/ELF/DWARF.h

public:
   const llvm::DWARFSection &getLineSection() const override {
     return lineSection;
   }
 
   const llvm::DWARFSection &getAddrSection() const override {
     return addrSection;
   }
 
-  const llvm::DWARFSection &getGnuPubnamesSection() const override {
+  const LLDDWARFSection &getGnuPubnamesSection() const override {
     return gnuPubnamesSection;
   }
 
-  const llvm::DWARFSection &getGnuPubtypesSection() const override {
+  const LLDDWARFSection &getGnuPubtypesSection() const override {
     return gnuPubtypesSection;
   }
 
   StringRef getFileName() const override { return ""; }
   StringRef getAbbrevSection() const override { return abbrevSection; }
   StringRef getStrSection() const override { return strSection; }
   StringRef getLineStrSection() const override { return lineStrSection; }
 

lld/ELF/SyntheticSections.cpp

readAddressAreas(DWARFContext &dwarf, InputSection *sec) {
 
   return ret;
 }
 
 template <class ELFT>
 static std::vector<GdbIndexSection::NameAttrEntry>
 readPubNamesAndTypes(const LLDDwarfObj<ELFT> &obj,
                      const std::vector<GdbIndexSection::CuEntry> &cus) {
-  const DWARFSection &pubNames = obj.getGnuPubnamesSection();
-  const DWARFSection &pubTypes = obj.getGnuPubtypesSection();
+  const LLDDWARFSection &pubNames = obj.getGnuPubnamesSection();
+  const LLDDWARFSection &pubTypes = obj.getGnuPubtypesSection();
 
   std::vector<GdbIndexSection::NameAttrEntry> ret;
-  for (const DWARFSection *pub : {&pubNames, &pubTypes}) {
-    DWARFDebugPubTable table(obj, *pub, config->isLE, true);
+  for (const LLDDWARFSection *pub : {&pubNames, &pubTypes}) {
+    DWARFDataExtractor data(obj, *pub, config->isLE, 0);
+    DWARFDebugPubTable table;
+    if (Error e = table.extract(data, /*GnuStyle=*/true))
+      warn(toString(pub->sec) + ": " + toString(std::move(e)));
     for (const DWARFDebugPubTable::Set &set : table.getData()) {
       // The value written into the constant pool is kind << 24 | cuIndex. As we
       // don't know how many compilation units precede this object to compute
       // cuIndex, we compute (kind << 24 | cuIndexInThisObject) instead, and add
       // the number of preceding compilation units later.
       uint32_t i = llvm::partition_point(cus,
                                          [&](GdbIndexSection::CuEntry cu) {
                                            return cu.cuOffset < set.Offset;

lld/test/ELF/gdb-index-invalid-pubnames.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: ld.lld --gdb-index %t -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK: warning: {{.*}}(.debug_gnu_pubnames): unexpected end of data at offset 0x1 while reading [0x0, 0x4)
+
+    .section .debug_abbrev,"",@progbits
+    .byte 1                         # Abbreviation Code
+    .byte 17                        # DW_TAG_compile_unit
+    .byte 0                         # DW_CHILDREN_no
+    .byte 0                         # EOM(1)
+    .byte 0                         # EOM(2)
+    .byte 0                         # EOM(3)
+
+    .section .debug_info,"",@progbits
+.LCUBegin:
+    .long .LUnitEnd-.LUnitBegin     # Length of Unit
+.LUnitBegin:
+    .short 4                        # DWARF version number
+    .long .debug_abbrev             # Offset Into Abbrev. Section
+    .byte 8                         # Address Size (in bytes)
+    .byte 1                         # Abbrev [1] DW_TAG_compile_unit
+.LUnitEnd:
+
+    .section .debug_gnu_pubnames,"",@progbits
+    .byte 0

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h

 //===- DWARFDebugPubTable.h -------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_DWARF_DWARFDEBUGPUBTABLE_H
 #define LLVM_DEBUGINFO_DWARF_DWARFDEBUGPUBTABLE_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
 #include "llvm/DebugInfo/DWARF/DWARFObject.h"
 #include <cstdint>
 #include <vector>
 
 namespace llvm {
 
 class raw_ostream;
 
struct Set {
     std::vector<Entry> Entries;
   };
 
 private:
   std::vector<Set> Sets;
 
   /// gnu styled tables contains additional information.
   /// This flag determines whether or not section we parse is debug_gnu* table.
-  bool GnuStyle;
+  bool GnuStyle = false;
 
 public:
-  DWARFDebugPubTable(const DWARFObject &Obj, const DWARFSection &Sec,
-                     bool LittleEndian, bool GnuStyle);
+  DWARFDebugPubTable() = default;
+
+  Error extract(DWARFDataExtractor Data, bool GnuStyle);
 
   void dump(raw_ostream &OS) const;
 
   ArrayRef<Set> getData() { return Sets; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGPUBTABLE_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

if (DumpOffset) {
       }
     } else {
       Loc.dumpRange(Offset, EndOffset - Offset, OS, MRI, Obj, DumpOpts);
     }
     Offset = EndOffset;
   }
 }
 
+static void dumpPubTableSection(raw_ostream &OS, DIDumpOptions DumpOpts,
+                                DWARFDataExtractor Data, bool GnuStyle) {
+  DWARFDebugPubTable Table;
+  if (Error E = Table.extract(Data, GnuStyle))
+    DumpOpts.RecoverableErrorHandler(std::move(E));
+  Table.dump(OS);
+}
+
 void DWARFContext::dump(
     raw_ostream &OS, DIDumpOptions DumpOpts,
     std::array<Optional<uint64_t>, DIDT_ID_Count> DumpOffsets) {
   uint64_t DumpType = DumpOpts.DumpType;
 
   StringRef Extension = sys::path::extension(DObj->getFileName());
   bool IsDWO = (Extension == ".dwo") || (Extension == ".dwp");
 
void DWARFContext::dump(
   if (shouldDump(ExplicitDWO, ".debug_rnglists.dwo", DIDT_ID_DebugRnglists,
                  DObj->getRnglistsDWOSection().Data)) {
     DWARFDataExtractor RnglistData(*DObj, DObj->getRnglistsDWOSection(),
                                    isLittleEndian(), 0);
     dumpRnglistsSection(OS, RnglistData, LookupPooledAddress, DumpOpts);
   }
 
   if (shouldDump(Explicit, ".debug_pubnames", DIDT_ID_DebugPubnames,
-                 DObj->getPubnamesSection().Data))
-    DWARFDebugPubTable(*DObj, DObj->getPubnamesSection(), isLittleEndian(), false)
-        .dump(OS);
+                 DObj->getPubnamesSection().Data)) {
+    DWARFDataExtractor PubTableData(*DObj, DObj->getPubnamesSection(),
+                                    isLittleEndian(), 0);
+    dumpPubTableSection(OS, DumpOpts, PubTableData, /*GnuStyle=*/false);
+  }
 
   if (shouldDump(Explicit, ".debug_pubtypes", DIDT_ID_DebugPubtypes,
-                 DObj->getPubtypesSection().Data))
-    DWARFDebugPubTable(*DObj, DObj->getPubtypesSection(), isLittleEndian(), false)
-        .dump(OS);
+                 DObj->getPubtypesSection().Data)) {
+    DWARFDataExtractor PubTableData(*DObj, DObj->getPubtypesSection(),
+                                    isLittleEndian(), 0);
+    dumpPubTableSection(OS, DumpOpts, PubTableData, /*GnuStyle=*/false);
+  }
 
   if (shouldDump(Explicit, ".debug_gnu_pubnames", DIDT_ID_DebugGnuPubnames,
-                 DObj->getGnuPubnamesSection().Data))
-    DWARFDebugPubTable(*DObj, DObj->getGnuPubnamesSection(), isLittleEndian(),
-                       true /* GnuStyle */)
-        .dump(OS);
+                 DObj->getGnuPubnamesSection().Data)) {
+    DWARFDataExtractor PubTableData(*DObj, DObj->getGnuPubnamesSection(),
+                                    isLittleEndian(), 0);
+    dumpPubTableSection(OS, DumpOpts, PubTableData, /*GnuStyle=*/true);
+  }
 
   if (shouldDump(Explicit, ".debug_gnu_pubtypes", DIDT_ID_DebugGnuPubtypes,
-                 DObj->getGnuPubtypesSection().Data))
-    DWARFDebugPubTable(*DObj, DObj->getGnuPubtypesSection(), isLittleEndian(),
-                       true /* GnuStyle */)
-        .dump(OS);
+                 DObj->getGnuPubtypesSection().Data)) {
+    DWARFDataExtractor PubTableData(*DObj, DObj->getGnuPubtypesSection(),
+                                    isLittleEndian(), 0);
+    dumpPubTableSection(OS, DumpOpts, PubTableData, /*GnuStyle=*/true);
+  }
 
   if (shouldDump(Explicit, ".debug_str_offsets", DIDT_ID_DebugStrOffsets,
                  DObj->getStrOffsetsSection().Data))
     dumpStringOffsetsSection(
         OS, DumpOpts, "debug_str_offsets", *DObj, DObj->getStrOffsetsSection(),
         DObj->getStrSection(), normal_units(), isLittleEndian());
   if (shouldDump(ExplicitDWO, ".debug_str_offsets.dwo", DIDT_ID_DebugStrOffsets,
                  DObj->getStrOffsetsDWOSection().Data))

llvm/lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp

 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 
 using namespace llvm;
 using namespace dwarf;
 
-DWARFDebugPubTable::DWARFDebugPubTable(const DWARFObject &Obj,
-                                       const DWARFSection &Sec,
-                                       bool LittleEndian, bool GnuStyle)
-    : GnuStyle(GnuStyle) {
-  DWARFDataExtractor PubNames(Obj, Sec, LittleEndian, 0);
-  uint64_t Offset = 0;
-  while (PubNames.isValidOffset(Offset)) {
+Error DWARFDebugPubTable::extract(DWARFDataExtractor Data, bool GnuStyle) {
+  this->GnuStyle = GnuStyle;
+  Sets.clear();
+  DataExtractor::Cursor C(0);
+  while (C && Data.isValidOffset(C.tell())) {
     Sets.push_back({});
     Set &SetData = Sets.back();
 
-    std::tie(SetData.Length, SetData.Format) =
-        PubNames.getInitialLength(&Offset);
+    std::tie(SetData.Length, SetData.Format) = Data.getInitialLength(C);
     const unsigned OffsetSize = dwarf::getDwarfOffsetByteSize(SetData.Format);
 
-    SetData.Version = PubNames.getU16(&Offset);
-    SetData.Offset = PubNames.getRelocatedValue(OffsetSize, &Offset);
-    SetData.Size = PubNames.getUnsigned(&Offset, OffsetSize);
+    SetData.Version = Data.getU16(C);
+    SetData.Offset = Data.getRelocatedValue(C, OffsetSize);
+    SetData.Size = Data.getUnsigned(C, OffsetSize);
 
-    while (Offset < Sec.Data.size()) {
-      uint64_t DieRef = PubNames.getUnsigned(&Offset, OffsetSize);
+    while (C) {
+      uint64_t DieRef = Data.getUnsigned(C, OffsetSize);
       if (DieRef == 0)
         break;
-      uint8_t IndexEntryValue = GnuStyle ? PubNames.getU8(&Offset) : 0;
-      StringRef Name = PubNames.getCStrRef(&Offset);
+      uint8_t IndexEntryValue = GnuStyle ? Data.getU8(C) : 0;
+      StringRef Name = Data.getCStrRef(C);
       SetData.Entries.push_back(
           {DieRef, PubIndexEntryDescriptor(IndexEntryValue), Name});
     }
   }
+  return C.takeError();
 }
 
 void DWARFDebugPubTable::dump(raw_ostream &OS) const {
   for (const Set &S : Sets) {
     int OffsetDumpWidth = 2 * dwarf::getDwarfOffsetByteSize(S.Format);
     OS << "length = " << format("0x%0*" PRIx64, OffsetDumpWidth, S.Length);
     OS << ", format = " << dwarf::FormatString(S.Format);
     OS << ", version = " << format("0x%04x", S.Version);

llvm/test/DebugInfo/X86/dwarfdump-debug-pubnames-invalid.s

+# RUN: llvm-mc -triple x86_64 %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-pubnames %t 2>&1 | FileCheck %s
+
+# CHECK: error: unexpected end of data at offset 0x1 while reading [0x0, 0x4)
+
+    .section .debug_pubnames,"",@progbits
+    .byte 0