This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fixed assert fail when symbol table has zero sh_info value.
ClosedPublic

Authored by grimar on Sep 28 2016, 5:37 AM.

Diff Detail

Event Timeline

grimar updated this revision to Diff 72808.Sep 28 2016, 5:37 AM
grimar retitled this revision from to [ELF] - Fixed assert fail when symbol table has invalid sh_info value..
grimar updated this object.
grimar added reviewers: ruiu, rafael, davide.
grimar added subscribers: llvm-commits, grimar, evgeny777.
grimar updated this revision to Diff 72809.Sep 28 2016, 5:47 AM
  • Restored line removed by mistake at last minute.
grimar updated this revision to Diff 72816.Sep 28 2016, 6:30 AM
  • Added testcase for sh_info == 0.
  • Do verification check earlier.
ruiu edited edge metadata.Sep 28 2016, 10:33 AM

Out of curiosity, why do you want to do this? Did you find an object file broken this way?

ELF/InputFiles.cpp
0–1

Probably FirstNonLocal == 0 is better as we all know that 0 is not a valid symbol index.

davide added inline comments.Sep 28 2016, 12:06 PM
ELF/InputFiles.cpp
0–1

Agree.

rafael added inline comments.Sep 28 2016, 1:08 PM
ELF/InputFiles.cpp
0–1

Do you need this check in here to avoid the crash? How was it crashing before getting to the other check you added in Writer?

grimar updated this revision to Diff 73006.Sep 30 2016, 1:51 AM
grimar edited edge metadata.
  • Addressed review comments.
  • Updated symtab-sh_info3.elf binary input.
ELF/InputFiles.cpp
0–1

So already answered in mails, repeat in short here for patch history:

Current invalid-symtab-sh_info3.elf demonstrates the issue.

That happens because of getLocalSymbols() implementation:
ArrayRef<SymbolBody *> elf::ObjectFile<ELFT>::getLocalSymbols() {
.....

uint32_t FirstNonLocal = this->Symtab->sh_info;
return makeArrayRef(this->SymbolBodies).slice(1, FirstNonLocal - 1);

}

When FirstNonLocal is 0, returned array has invalid 0xFFFFFFFF length and
accessing memory our of array bounds just may crash.

rafael accepted this revision.Oct 7 2016, 6:59 AM
rafael edited edge metadata.

The FirstNonLocal == 0 check LGTM, but please split the other check to another patch.

ELF/InputFiles.cpp
85

You don't need the extra ()

ELF/Writer.cpp
385

Please split this to another patch.

This revision is now accepted and ready to land.Oct 7 2016, 6:59 AM
grimar retitled this revision from [ELF] - Fixed assert fail when symbol table has invalid sh_info value. to [ELF] - Fixed assert fail when symbol table has zero sh_info value..Oct 7 2016, 8:24 AM
grimar updated this object.
grimar edited edge metadata.
grimar closed this revision.Oct 7 2016, 8:27 AM

r283562