This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Teach tool to dump objects with >= SHN_LORESERVE of sections.
ClosedPublic

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

Details

Summary

http://www.sco.com/developers/gabi/2003-12-17/ch4.eheader.html

says that e_shnum and/or e_shstrndx may have special values if
"the number of sections is greater than or equal to SHN_LORESERVE" or
"the section name string table section index is greater than or equal to SHN_LORESERVE (0xff00)"

Previously llvm-readobj was unable to dump such files, patch changes that.

I had to add a precompiled test case because it does not seem possible to
prepare a test using yaml2obj or llvm-mc (not clear how to make .shstrtab
to have index >= SHN_LORESERVE).

Diff Detail

Event Timeline

grimar created this revision.Jul 16 2018, 6:01 AM
jhenderson requested changes to this revision.Jul 17 2018, 1:55 AM
jhenderson added inline comments.
test/tools/llvm-readobj/many-sections.s
2

Thanks for the descriptive comment! I've got a few grammar nits though.

that generated -> that was

3

When ELF -> When an ELF

4

more of sections -> more sections
it's -> its

5

used -> is used

6

If section... -> If the section

9

used -> is used

11–13

has little amount of sections -> has few sections
set in according to above -> set according to the above

tools/llvm-readobj/ELFDumper.cpp
2474

It may be worth giving a comment here and on the next function briefly explaining the rules, similar to what you did in the test.

2476

Personally, I'd be explicit here, and say ElfHeader->e_shnum > 0, but I don't mind that much, if you prefer it this way.

2478–2479

What about the situation where a file has no section headers? This will crash, I think. Please fix and add a test for this case. I think if e_shnum is zero, you need to check e_shoff to see if it has a non-zero value to decide if there are section headers.

2487

Again, what happens if there are no section headers here? In this case, the object is malformed, and we should probably report an error or warning, but still write the actual value in the field.

This revision now requires changes to proceed.Jul 17 2018, 1:55 AM
grimar updated this revision to Diff 155847.Jul 17 2018, 4:11 AM
grimar marked 7 inline comments as done.

Addressed review comments. (Thanks, James!)

tools/llvm-readobj/ELFDumper.cpp
2478–2479

It would report "Error reading file: invalid section index" actually (because 'unwrapOrError' catches and reports an error and exit() then). It still was an issue though. Fixed, added test case.

2487

I am printing "corrupt: out of range" here, it what readelf do.

jhenderson added inline comments.Jul 17 2018, 6:22 AM
test/tools/llvm-readobj/many-sections.s
25

I'd use the term "zero" rather than null here, since it's just a value.

tools/llvm-readobj/ELFDumper.cpp
2492–2493

I'm not seeing this behaviour in readelf? If there are no section headers, I just get a value of 0 for the section header string table index, and no warning. A non-zero value on the other hand, when there are no sections (or indeed any value outside that range) does give me that error.

grimar added inline comments.Jul 17 2018, 6:40 AM
tools/llvm-readobj/ELFDumper.cpp
2492–2493

I tested many-sections-stripped.elf-x86_64
(shared it here: https://drive.google.com/open?id=1HgUSb15a6WQY_lwMIUCDXBxD25VX8I-S)
which has e_shoff set to zero. And it has e_shstrndx == SHN_XINDEX.

It reports the following for me:

readelf -v
GNU readelf (GNU Binutils for Ubuntu) 2.28
...

readelf -a many-sections-stripped.elf-x86_64 
ELF Header:
...
  Number of section headers:         0
  Section header string table index: 65535 <corrupt: out of range>

I think when we have e_shstrndx == SHN_XINDEX, we expect that "The actual number of section header table entries is contained in the sh_size field of the section header at index 0". And since actual number is always > 0 ("If the number of sections is greater than or equal to SHN_LORESERVE...").
(https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/chapter6-43405/index.html),
we are fine reporting "out of range" here when having no sections. Isn't that look valid behavior?

jhenderson added inline comments.Jul 17 2018, 7:00 AM
tools/llvm-readobj/ELFDumper.cpp
2492–2493

I think we might be misunderstanding each other. I think these are the cases we need to be aware of:

  1. The regular case where 0 < e_shstrndx < SHN_XINDEX. Here, we should (and do already) print the correct number.
  2. The regular large number of sections case where e_shrstndx == SHN_XINDEX. Here we should look up the number and print it along with SHN_XINDEX. This is what your patch implements, and is tested by LLVM1 etc.
  3. The case where e_shstrndx is zero. This means that there is no section header string table. We should just print zero in this case.

In cases one and two, if the index is greater than the number of sections, we should print an error. In a stripped ELF, there is no reason for a section header string table, so the index should be 0, like case 3), and we should not print an error, even if there were many section headers previously. Currently you are printing "0 (corrupt: out of range)", even though the e_shstrndx field is non-zero. I would expect it to print "65535 (corrupt: out of range)" or similar. So I think the printing of '0' is the bad part, because it hides the actual value in the field.

Does that make sense?

grimar added inline comments.Jul 17 2018, 7:06 AM
tools/llvm-readobj/ELFDumper.cpp
2492–2493

So I think the printing of '0' is the bad part, because it hides the actual value in the field.

Ah, I missed that I am printing 0 (corrupt: out of range) now.
My intention sure was to print the value, so to print the 65535 (corrupt: out of range).

I'll update the patch.

grimar updated this revision to Diff 155884.Jul 17 2018, 7:19 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Jul 17 2018, 8:02 AM

LGTM, with a comment change in the test.

test/tools/llvm-readobj/many-sections.s
26

I suppose technically this is malformed, since if the section headers are stripped, the e_shstrndx should be zero. Maybe you should change the comment here to:

"... set to zero, but not e_shstrndx, to show that this corrupt case is handled correctly."

This revision is now accepted and ready to land.Jul 17 2018, 8:02 AM
This revision was automatically updated to reflect the committed changes.