[llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402)

[Higuoxing]

Summary

The crash was caused when dereferencing nullptr in DumpObject and printArchiveChild.

In DumpObject, we have

2311    StringRef ArchiveName = a != nullptr ? a->getFileName() : "";
...
2323      printArchiveChild(a->getFileName(), *c);

2368    StringRef ArchiveName = A ? A->getFileName() : "";
...
2378      printArchiveChild(A->getFileName(), *C);

Look, this will cause segment fault when we use -a option on Non-archive files.
Besides, in printArchiveChild

2236  static void printArchiveChild(StringRef Filename, const Archive::Child &C) {
2237    Expected<sys::fs::perms> ModeOrErr = C.getAccessMode();
...

This can also cause segment fault when calling C.getAccessMode() (&C can be nullptr).

I update the printArchiveChild argument by taking a pointer of Archive::Child and check whether it's a nullptr.

This is a draft, I will add some tests later.

Any suggestions are welcoming, thanks

Bugzilla – Bug 39402

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2408 ↗

(On Diff #171045)

Here, Archive::Child *C will be nullptr

2410 ↗

(On Diff #171045)

Here, Archive::Child *C will be nullptr as well

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2408 ↗

(On Diff #171045)

Sorry, here, Archive::Child *C will not be nullptr

[Higuoxing]

Add simple test which will cause crash in previous 'nullptr dereferencing' version

[MaskRay]

tools/llvm-objdump/llvm-objdump.cpp

2410 ↗

(On Diff #171045)

DumpObject has only two call sites. Do you think it is better deleting its default arguments and passing nullptr explicitly?

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2410 ↗

(On Diff #171045)

Thanks a lot, I agree with you :)

[eush]

tools/llvm-objdump/llvm-objdump.cpp

2309 ↗

(On Diff #171051)

It seems like you could just add && c here and in the second overload and the signature of printArchiveChild could stay the same.

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2309 ↗

(On Diff #171051)

Thanks a lot, but I think checking nullptr inside printArchiveChild will be more readable ?

[eush]

tools/llvm-objdump/llvm-objdump.cpp

2309 ↗

(On Diff #171051)

I disagree. DumpArchive, DumpObject, as well as other functions in this file are never called on nullptr, the pattern is to check first, then call. Why should printArchiveChild work differently?

Also, from code-complexity point of view, this patch introduces one new conditional statement where there could be none.

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2309 ↗

(On Diff #171051)

Sorry, I misunderstand your opinion. I just want to make a safe printArchiveName, it seems that not a good idea to introduce a new conditional statement ... thanks a lot

[jhenderson]

test/tools/llvm-objdump/non-archive-object.test

3–4 ↗

(On Diff #171460)

I don't think you need to worry about testing both switches in this test, as this is testing a specific aspect of the switch, rather than the generic behaviour. That will then allow you to write the llvm-objdump and FileCheck on the same line:

# RUN: llvm-objdump --archive-headers %t | FileCheck %s

tools/llvm-objdump/llvm-objdump.cpp

2309 ↗

(On Diff #171051)

Personally, I'm a big fan of being explicit in my checks against null, i.e. write c != nullptr instead of just c, but I'm not too bothered either way.

[jhenderson]

LGTM, with the nit.

test/tools/llvm-objdump/non-archive-object.test

4 ↗

(On Diff #171482)

Nit: "If this test has not crashed..."

[Higuoxing]

Fix typo. Thanks a lot for all of your help :)

[jhenderson]

Do you need help committing this again? If so, could you confirm how I should write your name in the attribution, please? (e.g. Xing Guo/Guo Xing etc)

[Higuoxing]

In D53690#1278729, @jhenderson wrote:

Do you need help committing this again? If so, could you confirm how I should write your name in the attribution, please? (e.g. Xing Guo/Guo Xing etc)

Thanks a lot, I don't have commit permission. Please help me commit it ... Thanks a lot

Xing GUO is ok :)

llvm/trunk/test/tools/llvm-objdump/non-archive-object.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump -a %t | FileCheck %s
+
+# If this test has not crashed, then this test passed.
+# CHECK: file format ELF64-x86-64
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000010
+    Size:            64
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "01234567"
+    Size:            4
+

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

if (!RawClangAST) {
     outs() << '\n';
     if (a)
       outs() << a->getFileName() << "(" << o->getFileName() << ")";
     else
       outs() << o->getFileName();
     outs() << ":\tfile format " << o->getFileFormatName() << "\n\n";
   }
 
-  if (ArchiveHeaders && !MachOOpt)
-    printArchiveChild(a->getFileName(), *c);
+  if (ArchiveHeaders && !MachOOpt && c)
+    printArchiveChild(ArchiveName, *c);
   if (Disassemble)
     DisassembleObject(o, Relocations);
   if (Relocations && !Disassemble)
     PrintRelocations(o);
   if (DynamicRelocations)
     PrintDynamicRelocations(o);
   if (SectionHeaders)
     PrintSectionHeaders(o);
static void DumpObject(const COFFImportFile *I, const Archive *A,
 
   // Avoid other output when using a raw option.
   if (!RawClangAST)
     outs() << '\n'
            << ArchiveName << "(" << I->getFileName() << ")"
            << ":\tfile format COFF-import-file"
            << "\n\n";
 
-  if (ArchiveHeaders && !MachOOpt)
-    printArchiveChild(A->getFileName(), *C);
+  if (ArchiveHeaders && !MachOOpt && C)
+    printArchiveChild(ArchiveName, *C);
   if (SymbolTable)
     printCOFFSymbolTable(I);
 }
 
 /// Dump each object file in \a a;
 static void DumpArchive(const Archive *a) {
   Error Err = Error::success();
   for (auto &C : a->children(Err)) {