This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - implement support of extended length field for CIE/FDE records of eh_frame.
ClosedPublic

Authored by grimar on Dec 15 2015, 8:49 AM.

Details

Summary

Problem here in the next part of code:

void EHOutputSection<ELFT>::addSectionAux(
    EHInputSection<ELFT> *S,
    iterator_range<const Elf_Rel_Impl<ELFT, IsRela> *> Rels) {
...
    uint32_t Length = read32<E>(D.data());
    Length += 4;
...

Ian Lance Taylor writes: "Read 4 bytes. If they are not 0xffffffff, they are the length of the CIE or FDE record. Otherwise the next 64 bits holds the length, and this is a 64-bit DWARF format. This is like .debug_frame." (http://www.airs.com/blog/archives/460), that also consistent with spec (https://refspecs.linuxfoundation.org/LSB_3.0.0/LSB-PDA/LSB-PDA/ehframechpt.html).

When length was 0xffffffff overflow happened and code executed forward without any error here, failing much later.
Patch implements support of described extended length field and also adds few more checks for safety.

Diff Detail

Event Timeline

grimar updated this revision to Diff 42863.Dec 15 2015, 8:49 AM
grimar retitled this revision from to [ELF] - implement support of extended length field for CIE/FDE records of eh_frame..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added a subscriber: emaste.Dec 15 2015, 10:01 AM

Looks reasonable. How about a test for successful Is64Dwarf though?

ruiu added inline comments.Dec 15 2015, 11:54 AM
ELF/OutputSections.cpp
1001–1002

Can you return early if it's 32 bit?

uint64_t Len = read32<E>(D.data());
if (Len < UINT32_MAX) {
  if (Len > D.size())
    error("CIE/FIE ends past the end of the section");
  return Len + 4;
}
if (D.size() < 12)
  error("Truncated CIE/FDE length");
Len = read64<E>(D.data() + 4);
if (Len > D.size())
  error("CIE/FIE ends past the end of the section");
return Len + 12;
ELF/OutputSections.h
304

ArrayRef is usually supposed to be passed by value as it's basically a pointer to an array.

grimar updated this revision to Diff 42969.Dec 16 2015, 1:11 AM
  • Review commend addressed.
  • Added test for valid 64 bit CIE/FDE record size.
  • Fixed invalid-cie-length5.s test.

Looks reasonable. How about a test for successful Is64Dwarf though?

Added test.

ELF/OutputSections.cpp
1001–1002

Done.

ELF/OutputSections.h
304

Ok, removed &.

ruiu accepted this revision.Dec 16 2015, 10:50 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 16 2015, 10:50 AM
This revision was automatically updated to reflect the committed changes.