[llvm-objdump] Don't attempt to print lines beyond the end of file

[phosek]

Summary

This triggers a segfault in llvm-objdump.

[davide]

Please add a test, if you can craft one. Gosh, llvm-objdump code is not great.

[phosek]

Is it fine to check-in executable? I saw that there're couple already in Inputs for llvm-objdump tests. I'm struggling to reproduce this with just pure llvm-mc and llvm-objdump, but I have a minimal reproducer that works with Clang and produces tiny (1.6k) ELF binary. The problems is that lines in LineBuffer are stored in std::vector which is allocated in chunks, so even if the debug info points beyond the end of the file, say line 15 in a file which has 14 lines (that typically happens because of comments), this still wouldn't segfault because the vector will have 16 items, but if you can make it point to line 17, it'll segfault (which is what my reproducer does).

[davide]

In D32466#736301, @phosek wrote:

Is it fine to check-in executable? I saw that there're couple already in Inputs for llvm-objdump tests. I'm struggling to reproduce this with just pure llvm-mc and llvm-objdump, but I have a minimal reproducer that works with Clang and produces tiny (1.6k) ELF binary. The problems is that lines in LineBuffer are stored in std::vector which is allocated in chunks, so even if the debug info points beyond the end of the file, say line 15 in a file which has 14 lines (that typically happens because of comments), this still wouldn't segfault because the vector will have 16 items, but if you can make it point to line 17, it'll segfault (which is what my reproducer does).

As long as it's small, fine. Ideally you might want to not check in binaries, if possible.
If you're having hard time reducing with llvm-mc, you may want to consider using obj2yaml and yaml2obj (having a text representation is better than having a binary).
If the yaml file fails, then check in a binary, but please add instructions (maybe in a comment) on how to regenerate the file in future (in case we need to).
BTW, is this a valid input?

[phosek]

Yes, we ran into this issue in our codebase. I was looking into using yaml2obj, problem is that IIUC it only supports emitting DWARF info for MachO files at the moment so we would need to extend it first to also support ELF.

[davide]

test/tools/llvm-objdump/X86/Inputs/debug-info-fileinfo.S

1–15 ↗

(On Diff #96505)

Instead of having a separate file you can put this as a comment of test/tools/llvm-objdump/X86/debug-info-fileinfo.test, no?

test/tools/llvm-objdump/X86/debug-info-fileinfo.test

1 ↗

(On Diff #96505)

Nit: the permissions for the file are 0755, 0644 should be fine.

3 ↗

(On Diff #96505)

You can probably dump a little more.

[davide]

Do me a favour and check that the output is the same of GNU objdump (I can't easily check at the moment as I'm not on a Linux Workstation).
If that's the case, this LGTM and you can submit without another round trip. Thanks for your fix. At some point we should really take a step back and rewrite the disassembler.

[phosek]

Almost, there's a slight difference in the way GNU objdump and llvm-objdump format the function name. I've modified the test so it passes with both.

llvm/trunk/test/tools/llvm-objdump/X86/debug-info-fileinfo.test

+# Check that debug info line number pointing beyond the end of the file
+# doesn't cause a segmentation fault in llvm-objdump.
+
+# To rebuild the input: clang -g debug-info-fileinfo.S -o debug-info-fileinfo.exe -nostdlib
+
+# RUN: llvm-objdump -S %p/Inputs/debug-info-fileinfo.exe.elf-x86_64 | FileCheck %s
+
+# CHECK: Disassembly of section .text:
+# CHECK: _start
+# CHECK-NEXT:   400078:       c3      retq
+# CHECK: foo
+# CHECK-NEXT:   400079:       90      nop
+# CHECK-NEXT:   40007a:       90      nop
+# CHECK-NEXT:   40007b:       c3      retq
+
+.text
+
+.global _start; .type _start, STT_FUNC; _start:
+    ret
+
+.global foo; .type foo, STT_FUNC; foo:
+    # foo
+    # bar
+    # baz
+    nop
+    nop
+    ret

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

if (PrintLines)
     OS << Delimiter << LineInfo.FileName << ":" << LineInfo.Line << "\n";
   if (PrintSource) {
     if (SourceCache.find(LineInfo.FileName) == SourceCache.end())
       if (!cacheSource(LineInfo.FileName))
         return;
     auto FileBuffer = SourceCache.find(LineInfo.FileName);
     if (FileBuffer != SourceCache.end()) {
       auto LineBuffer = LineCache.find(LineInfo.FileName);
-      if (LineBuffer != LineCache.end())
+      if (LineBuffer != LineCache.end()) {
+        if (LineInfo.Line > LineBuffer->second.size())
+          return;
         // Vector begins at 0, line numbers are non-zero
         OS << Delimiter << LineBuffer->second[LineInfo.Line - 1].ltrim()
            << "\n";
       }
     }
+  }
   OldLineInfo = LineInfo;
 }
 
 static bool isArmElf(const ObjectFile *Obj) {
   return (Obj->isELF() &&
           (Obj->getArch() == Triple::aarch64 ||
            Obj->getArch() == Triple::aarch64_be ||
            Obj->getArch() == Triple::arm || Obj->getArch() == Triple::armeb ||