Page MenuHomePhabricator

[ELF] Fix a null pointer dereference when --emit-relocs and --strip-debug are used together
ClosedPublic

Authored by MaskRay on Wed, Feb 12, 2:55 PM.

Details

Summary

Fixes https://bugs.llvm.org//show_bug.cgi?id=44878

When --strip-debug is specified, .debug* are removed from inputSections
while .rel[a].debug* (incorrectly) remain.

LinkerScript::addOrphanSections() requires the output section of a relocated
InputSectionBase to be created first.

.debug* are not in inputSections -> output sections .debug* are not
created -> getOutputSectionName(.rel[a].debug*) dereferences a null
pointer.

Fix the null pointer dereference by deleting .rel[a].debug* from inputSections as well.

Diff Detail

Event Timeline

MaskRay created this revision.Wed, Feb 12, 2:55 PM
MaskRay marked an inline comment as done.Wed, Feb 12, 2:56 PM
MaskRay added inline comments.
lld/test/ELF/emit-relocs-debug.s
4

I'm a bit reluctant to add another test for the obsoleted feature .zdebug*

nickdesaulniers added inline comments.
lld/ELF/Driver.cpp
1947

will this fit on one line?

if (isDebugSection(isec->getRelocationSection())`
MaskRay marked 2 inline comments as done.Wed, Feb 12, 4:17 PM
MaskRay added inline comments.
lld/ELF/Driver.cpp
1947

rel may be nullptr, if sh_info=0. Maybe I should make isDebugSection take a reference to emphasize that the argument cannot be nullptr.

MaskRay updated this revision to Diff 244302.Wed, Feb 12, 4:20 PM
MaskRay marked an inline comment as done.

Change isDebugSection() to take a const reference.

nickdesaulniers accepted this revision.Wed, Feb 12, 4:32 PM
This revision is now accepted and ready to land.Wed, Feb 12, 4:32 PM
grimar accepted this revision.Thu, Feb 13, 12:18 AM

LGTM.

lld/test/ELF/emit-relocs-debug.s
4

We support it though. I have no strong opinion on this as I do not know if it is used in the wild.
Covering all the features can be good when collecting the code coverage. I had experiments
with it before: http://lists.llvm.org/pipermail/llvm-dev/2018-April/122782.html

May be we might think about reporting an error if an object has .zdebug and see if people will complain?
We have some places in the linker where we had to introduce some sort of temporary hacks.
E.g:

// The linkonce feature is a sort of proto-comdat. Some glibc i386 object
// files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
// sections. Drop those sections to avoid duplicate symbol errors.
// FIXME: This is glibc PR20543, we should remove this hack once that has been
// fixed for a while.
if (name == ".gnu.linkonce.t.__x86.get_pc_thunk.bx" ||
    name == ".gnu.linkonce.t.__i686.get_pc_thunk.bx")
  return &InputSection::discarded;

I guess one day we might want to start removing such things. Perhaps .zdebug is a good candidate
for one of the next LLD releases.

This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.Thu, Feb 13, 9:18 AM
MaskRay added inline comments.
lld/test/ELF/emit-relocs-debug.s
4

My clang D61689 was reverted. I think our internal toolchain is still not ready (-no-integrated-as uses an old GNU as). I will try fixing the remaining issues so that our internal toolchain will not block .zdebug removal. I will start a llvm-dev notification when I want to delete .zdebug input support.

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

Removing support for .gnu.linkonce will break module loading in the Linux kernel.
See also: https://github.com/ClangBuiltLinux/linux/issues/432

MaskRay added a comment.EditedThu, Feb 13, 9:33 AM

Regarding .gnu.linkonce*: I also want to delete them. I left a comment on https://sourceware.org/bugzilla/show_bug.cgi?id=20543 half a year ago.

Removing support for .gnu.linkonce will break module loading in the Linux kernel.
See also: https://github.com/ClangBuiltLinux/linux/issues/432

What lld does is to simply discard .gnu.linkonce.t.__x86.get_pc_thunk.bx and .gnu.linkonce.t.__i386.get_pc_thunk.bx input sections from object files.
It never supports the semantics of .gnu.linkonce. Other .gnu.linkonce. prefixed section names (e.g. .gnu.link_once.this_module) are not recognized by lld.
So if something currently works, after removing the code below, it will continue working.

// The linkonce feature is a sort of proto-comdat. Some glibc i386 object
// files contain definitions of symbol "__x86.get_pc_thunk.bx" in linkonce
// sections. Drop those sections to avoid duplicate symbol errors.
// FIXME: This is glibc PR20543, we should remove this hack once that has been
// fixed for a while.
if (name == ".gnu.linkonce.t.__x86.get_pc_thunk.bx" ||
    name == ".gnu.linkonce.t.__i686.get_pc_thunk.bx")
  return &InputSection::discarded;
kees added a comment.Mon, Feb 17, 10:57 AM

Thank you for the quick fix! I can confirm my builds with --string-debug work now. :)