[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

Here, Archive::Child *C will be nullptr

2410

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

[Higuoxing]

tools/llvm-objdump/llvm-objdump.cpp

2408

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

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

Thanks a lot, I agree with you :)

[eush]

tools/llvm-objdump/llvm-objdump.cpp

2309

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

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

[eush]

tools/llvm-objdump/llvm-objdump.cpp

2309

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

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

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 :)

tools/llvm-objdump/llvm-objdump.cpp

static void printFileHeaders(const ObjectFile *o) {
 
   StringRef Fmt = o->getBytesInAddress() > 4 ? "%016" PRIx64 : "%08" PRIx64;
   uint64_t Address = StartAddrOrErr.get();
   outs() << "start address: "
          << "0x" << format(Fmt.data(), Address)
          << "\n";
 }
 
-static void printArchiveChild(StringRef Filename, const Archive::Child &C) {
-  Expected<sys::fs::perms> ModeOrErr = C.getAccessMode();
+static void printArchiveChild(StringRef Filename, const Archive::Child *C) {
+  if (C == nullptr) return;
+  Expected<sys::fs::perms> ModeOrErr = C->getAccessMode();
   if (!ModeOrErr) {
     errs() << "ill-formed archive entry.\n";
     consumeError(ModeOrErr.takeError());
     return;
   }
   sys::fs::perms Mode = ModeOrErr.get();
   outs() << ((Mode & sys::fs::owner_read) ? "r" : "-");
   outs() << ((Mode & sys::fs::owner_write) ? "w" : "-");
   outs() << ((Mode & sys::fs::owner_exe) ? "x" : "-");
   outs() << ((Mode & sys::fs::group_read) ? "r" : "-");
   outs() << ((Mode & sys::fs::group_write) ? "w" : "-");
   outs() << ((Mode & sys::fs::group_exe) ? "x" : "-");
   outs() << ((Mode & sys::fs::others_read) ? "r" : "-");
   outs() << ((Mode & sys::fs::others_write) ? "w" : "-");
   outs() << ((Mode & sys::fs::others_exe) ? "x" : "-");
 
   outs() << " ";
 
-  Expected<unsigned> UIDOrErr = C.getUID();
+  Expected<unsigned> UIDOrErr = C->getUID();
   if (!UIDOrErr)
     report_error(Filename, UIDOrErr.takeError());
   unsigned UID = UIDOrErr.get();
   outs() << format("%d/", UID);
 
-  Expected<unsigned> GIDOrErr = C.getGID();
+  Expected<unsigned> GIDOrErr = C->getGID();
   if (!GIDOrErr)
     report_error(Filename, GIDOrErr.takeError());
   unsigned GID = GIDOrErr.get();
   outs() << format("%-d ", GID);
 
-  Expected<uint64_t> Size = C.getRawSize();
+  Expected<uint64_t> Size = C->getRawSize();
   if (!Size)
     report_error(Filename, Size.takeError());
   outs() << format("%6" PRId64, Size.get()) << " ";
 
-  StringRef RawLastModified = C.getRawLastModified();
+  StringRef RawLastModified = C->getRawLastModified();
   unsigned Seconds;
   if (RawLastModified.getAsInteger(10, Seconds))
     outs() << "(date: \"" << RawLastModified
            << "\" contains non-decimal chars) ";
   else {
     // Since ctime(3) returns a 26 character string of the form:
     // "Sun Sep 16 01:03:52 1973\n\0"
     // just print 24 characters.
     time_t t = Seconds;
     outs() << format("%.24s ", ctime(&t));
   }
 
   StringRef Name = "";
-  Expected<StringRef> NameOrErr = C.getName();
+  Expected<StringRef> NameOrErr = C->getName();
   if (!NameOrErr) {
     consumeError(NameOrErr.takeError());
-    Expected<StringRef> RawNameOrErr = C.getRawName();
+    Expected<StringRef> RawNameOrErr = C->getRawName();
     if (!RawNameOrErr)
       report_error(Filename, NameOrErr.takeError());
     Name = RawNameOrErr.get();
   } else {
     Name = NameOrErr.get();
   }
   outs() << Name << "\n";
 }
 
 static void DumpObject(ObjectFile *o, const Archive *a = nullptr,
                        const Archive::Child *c = nullptr) {
   StringRef ArchiveName = a != nullptr ? a->getFileName() : "";
   // Avoid other output when using a raw option.
   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);
+    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);
+    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)) {
static void DumpInput(StringRef file) {
 
   // Attempt to open the binary.
   Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(file);
   if (!BinaryOrErr)
     report_error(file, BinaryOrErr.takeError());
   Binary &Binary = *BinaryOrErr.get().getBinary();
 
   if (Archive *a = dyn_cast<Archive>(&Binary))
     DumpArchive(a);
   else if (ObjectFile *o = dyn_cast<ObjectFile>(&Binary))
     DumpObject(o);
   else if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(&Binary))
     ParseInputMachO(UB);
   else
     report_error(file, object_error::invalid_file_type);
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);