Page MenuHomePhabricator

[Object/ELF.h] - Improve testing of the fields in ELFFile<ELFT>::sections().
ClosedPublic

Authored by grimar on Jul 19 2019, 5:42 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson added inline comments.Jul 19 2019, 6:04 AM
include/llvm/Object/ELF.h
522 ↗(On Diff #210806)

I think this error might be easier to understand with some rewording. How about:

"Invalid section header table offset (e_shoff = 0x1234) or invalid number of sections specified in the first section header's sh_size field (0x4321)."

525 ↗(On Diff #210806)

e_shoff should probably be printed in hex, since it's an offset.

test/Object/invalid.test
556 ↗(On Diff #210806)

an error

557 ↗(On Diff #210806)

How about == instead of =?

593 ↗(On Diff #210806)

I think there's one more case to test, if possible: a very high e_shoff value (e.g. 0xfffffffffffffff), combined with a small number of section headers (e.g. 1).

This assumes it's possible in yaml2obj to explicitly specify an e_shoff field without affecting the file size (I can't remember if you added that ability).

grimar updated this revision to Diff 211051.Jul 22 2019, 4:40 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
grimar added inline comments.Jul 22 2019, 4:40 AM
test/Object/invalid.test
593 ↗(On Diff #210806)

I had to fix the e_shoff in ELFYAML.h to make this test, it had uint16 type, but had to be uint_X

jhenderson accepted this revision.Jul 22 2019, 9:32 AM

LGTM, with the one suggestion.

include/llvm/Object/ELF.h
524 ↗(On Diff #211051)

Prefix these hex numbers with "0x" to make it obvious that they are hex (it's not always obvious).

This revision is now accepted and ready to land.Jul 22 2019, 9:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 4:39 AM