This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not produce broken output when amount of sections is > ~65k
ClosedPublic

Authored by grimar on Jul 16 2018, 6:08 AM.

Details

Summary

This is https://bugs.llvm.org//show_bug.cgi?id=38119

We produce broken ELF header now when the number of output sections is >= SHN_LORESERVE (0xff00).

ELF spec says (http://www.sco.com/developers/gabi/2003-12-17/ch4.eheader.html):

e_shnum:
If the number of sections is greater than or equal to SHN_LORESERVE (0xff00), this member has the value zero
and the actual number of section header table entries is contained in the sh_size field of the section header at index 0.
(Otherwise, the sh_size member of the initial entry contains 0.)

e_shstrndx
If the section name string table section index is greater than or equal to SHN_LORESERVE (0xff00), this member has the
value SHN_XINDEX (0xffff) and the actual index of the section name string table section is contained in the sh_link field of
the section header at index 0. (Otherwise, the sh_link member of the initial entry contains 0.)

We did not set these fields correctly earlier. The patch fixes the issue.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 16 2018, 6:08 AM

This looks correct to me. I've got a few small suggestions.

ELF/Writer.cpp
2267 ↗(On Diff #155657)

I suggest combining the two comments for SHN_LORESERVE as they are closely related. For example:

The ELF header can only store numbers up to SHN_LORESERVE in the e_shnum and e_shstrndx fields. When the value of one of these fields exceeds SHN_LORESERVE ELF requires us to put sentinel values in
the ELF header and use fields in the section header at index 0 to store the value. The sentinel values and fields are:
e_shnum = 0, SHdrs[0].sh_size = number of sections.
e_shstrndx = SHN_XINDEX, SHdrs[0].sh_link = strtab section index.
2271 ↗(On Diff #155657)

Is it worth something like NumElfSections = OutputSections.size() + 1; as we use it 3 times in a row, and it makes the +1 for the 0th Section Header a bit more obvious.

2272 ↗(On Diff #155657)

Should we explicitly set EHdr->e_shnum to 0 here as we've called it out in the comment.

grimar updated this revision to Diff 155859.Jul 17 2018, 5:37 AM
grimar marked 2 inline comments as done.
grimar removed a reviewer: espindola.

Addressed comments. Thanks, Peter!

grimar added inline comments.Jul 17 2018, 5:37 AM
ELF/Writer.cpp
2272 ↗(On Diff #155657)

Above (line 2246) we have a code that does not do that, assuming fields are zero by default:

if (!Config->Relocatable) {
  EHdr->e_phoff = sizeof(Elf_Ehdr);
  EHdr->e_phentsize = sizeof(Elf_Phdr);
}

So to be consistent, I would not do that probably. Though both ways would be OK to me.

peter.smith accepted this revision.Jul 17 2018, 5:43 AM

Looks good to me thanks.

This revision is now accepted and ready to land.Jul 17 2018, 5:43 AM
This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Jul 18 2018, 9:54 AM

Do you really need this? That the spec says something doesn't always mean that we need it. I'm afraid that all these nitpicky details would accumulate and make the entire program hard to read. Even though I'm not against "doing it right", I really want you to show some examples to convince others that we need it. Also, please wait for me for a few days (I believe you've got my vacation responder email) to review your change. In the meantime, could you please revert this change?

ruiu added a comment.Jul 18 2018, 10:19 AM

Hm, maybe this particular patch is too easy that it doesn't have to be reverted, but please allow me time to review your patches. I believe there's no need to hurry to submit these kind of changes.

Do you really need this? That the spec says something doesn't always mean that we need it. I'm afraid that all these nitpicky details would accumulate and make the entire program hard to read. Even though I'm not against "doing it right", I really want you to show some examples to convince others that we need it.

Hi Rui, this was https://bugs.llvm.org//show_bug.cgi?id=38119. We produced broken output previously and user faced it.
Tools did not allow to dump the content correctly and things looked like some kind of memory corruption.

Hm, maybe this particular patch is too easy that it doesn't have to be reverted, but please allow me time to review your patches.
I believe there's no need to hurry to submit these kind of changes.

Sure. Thanks.

(I believe you've got my vacation responder email.)

(FYI I did not get any responder emails from you, I guess because using phab and not direct mails may be. Not sure)