Page MenuHomePhabricator

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

Authored by Higuoxing on Oct 25 2018, 1:13 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

Higuoxing created this revision.Oct 25 2018, 1:13 AM
Higuoxing added inline comments.Oct 25 2018, 1:23 AM
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 added inline comments.Oct 25 2018, 1:32 AM
tools/llvm-objdump/llvm-objdump.cpp
2408 ↗(On Diff #171045)

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

Higuoxing updated this revision to Diff 171051.Oct 25 2018, 1:52 AM

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

MaskRay added inline comments.Oct 27 2018, 4:40 PM
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 added inline comments.Oct 27 2018, 9:25 PM
tools/llvm-objdump/llvm-objdump.cpp
2410 ↗(On Diff #171045)

Thanks a lot, I agree with you :)

eush added a subscriber: eush.Oct 28 2018, 3:15 AM
eush added inline comments.
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 updated this revision to Diff 171444.Oct 28 2018, 6:05 PM
Higuoxing marked an inline comment as done.Oct 28 2018, 6:08 PM
Higuoxing added inline comments.
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 added inline comments.Oct 28 2018, 11:01 PM
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 marked an inline comment as done.Oct 29 2018, 1:01 AM
Higuoxing added inline comments.
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 added inline comments.Oct 29 2018, 4:10 AM
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.

Higuoxing updated this revision to Diff 171482.Oct 29 2018, 4:19 AM
Higuoxing marked an inline comment as done.
jhenderson accepted this revision.Oct 29 2018, 4:41 AM

LGTM, with the nit.

test/tools/llvm-objdump/non-archive-object.test
4 ↗(On Diff #171482)

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

This revision is now accepted and ready to land.Oct 29 2018, 4:41 AM
Higuoxing updated this revision to Diff 171484.Oct 29 2018, 4:54 AM

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

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)

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

This revision was automatically updated to reflect the committed changes.