This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Prevent crash in writing an .ARM.exidx sentinel entry.
ClosedPublic

Authored by ikudrin on Dec 11 2017, 9:49 PM.

Details

Summary

We might crash in ARMExidxSentinelSection::writeTo() because it expects that
the sentinel entry is put in the same InputSectionDescription as the last real entry,
but if a linker script has anything but an input section description entry at the end
of the output section description for .ARM.exidx, OutputSection::addSection()
puts the sentinel entry into a new bucket.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ikudrin created this revision.Dec 11 2017, 9:49 PM
ruiu edited edge metadata.Dec 11 2017, 9:54 PM

As the new code is not obvious, please write comment.

ikudrin updated this revision to Diff 126500.Dec 11 2017, 10:19 PM
  • Added a comment.
peter.smith edited edge metadata.Dec 12 2017, 2:09 AM

I think that this is the right thing to do for .ARM.exidx sections. I'm pretty sure the original implementation did do something like that, but it must have been refactored sometime later to use addSection(). I don't think that this is expected for orphans in general though. For example consider the silly example: SECTIONS { .text : { t.o(.text) foo = .; } }
If I provide an object t.o with a .text section and an object t2.o with a .text section, the t2.o will be an orphan that will match in .text. The bfd linker places this after the assignment to foo = .

.text          0x0000000000000000        0x4 t.o
                0x0000000000000000                _start
                0x0000000000000004                foo = .
 .text          0x0000000000000004        0x4 t2.o
                0x0000000000000004                func

At first glance it looks like the patch might change this behaviour for orphan sections? This might lead to incompatibility problems with bfd. If I'm right perhaps addSection can be made to optionally choose to find the last InputSectionDescription for cases like .ARM.exidx. Alternatively some custom code could be used for .ARM.exidx as that case is much more specialised.

ikudrin planned changes to this revision.Dec 12 2017, 3:15 AM

Thanks for the example, Peter! It looks like I have to think on these corner cases a bit more.

ikudrin updated this revision to Diff 126704.Dec 13 2017, 2:53 AM
ikudrin retitled this revision from [ELF] Fix placement of a sentinel entry in the .ARM.exidx section. to [ELF] Prevent crash in writing an .ARM.exidx sentinel entry..
ikudrin edited the summary of this revision. (Show Details)
  • Limited the patch to fix only the crash in ARMExidxSentinelSection::writeTo().

That looks correct to me. In the test I think it would be useful to check the contents of the table llvm-objdump -s or llvm-readobj -u to check that the sentinel is written in the correct place.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2017, 10:24 PM
This revision was automatically updated to reflect the committed changes.