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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/DWARF.h:59
 
-  const llvm::DWARFSection &getGnuPubnamesSection() const override {
+  const LLDDWARFSection &getGnuPubnamesSection() const override {
     return gnuPubnamesSection;
----------------
jhenderson wrote:
> Why did the types here need to change?
This helps to have access to the `sec` field without casing. That is used in `readPubNamesAndTypes()`.


================
Comment at: lld/ELF/SyntheticSections.cpp:2713
+  for (const LLDDWARFSection *pub : {&pubNames, &pubTypes}) {
+    DWARFDataExtractor data(obj, *pub, config->isLE, 0);
+    DWARFDebugPubTable table;
----------------
jhenderson wrote:
> Could you put a comment here saying what the `0` means (i.e. `/*Something=*/0`), please?
OK, changed to `config->wordsize` to avoid uncertainties.


================
Comment at: lld/test/ELF/gdb-index-invalid-pubnames.s:5
+
+# CHECK: warning: {{.*}}(.debug_gnu_pubnames): unexpected end of data at offset 0x1 while reading [0x0, 0x4)
+
----------------
jhenderson wrote:
> 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.
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.


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-debug-pubnames-invalid.s:1
+# RUN: llvm-mc -triple x86_64 %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-pubnames %t 2>&1 | FileCheck %s
----------------
jhenderson wrote:
> 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.
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.


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

https://reviews.llvm.org/D83049