Page MenuHomePhabricator

[llvm-readelf] - --elf-hash-histogram: do not crash when the .gnu.hash goes past the EOF.
ClosedPublic

Authored by grimar on Tue, May 19, 7:38 AM.

Details

Summary

llvm-readelf might crash when the .gnu.hash table goes past the EOF.

This patch splits and updates the code of a helper function checkGNUHashTable,
which is similar to checkHashTable and fixes the issue.

Diff Detail

Event Timeline

grimar created this revision.Tue, May 19, 7:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Tue, May 19, 8:49 AM

LGTM

This revision is now accepted and ready to land.Tue, May 19, 8:49 AM
jhenderson added inline comments.Thu, May 21, 1:35 AM
llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
275

This and below includes reference to 2 variables (MASKWORDS and NBUCKETS) that don't appear to be used?

Should you also add something that shows that the hash histogram isn't printed in these cases?

297

Perhaps move this comment and the similar one below, to where they are actually used.

llvm/tools/llvm-readobj/ELFDumper.cpp
2683

Again, probably in a follow-up, perhaps this should be changed to reportUniqueWarning (and similar in checkHashTable). That way, if a user requested both the hash histogram and hash tables (which seems like a not unreasonable request), they'd only get the warning once.

2706–2707

I note that printHashTable doesn't print any information at all if the hash table is bad, but this prints the header information. Perhaps (in a follow-up), printHashTable could be changed to print the header information like here?

grimar marked an inline comment as done.Thu, May 21, 4:06 AM

This patch still depends on D80204, I'll address other comments after.

llvm/tools/llvm-readobj/ELFDumper.cpp
2706–2707

Yes, I've noticed this difference too when wrote this patch. I've addressed this and above comment here: D80373

grimar planned changes to this revision.Tue, May 26, 6:29 AM
grimar marked an inline comment as done.
grimar added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
2708–2719

It is incorrect to do this check so late I think. I'll move it earlier (https://reviews.llvm.org/D80373#inline-739398).

grimar updated this revision to Diff 266836.Thu, May 28, 6:12 AM
grimar marked 7 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Updated implementation.
  • Addressed review comments.
This revision is now accepted and ready to land.Thu, May 28, 6:12 AM
grimar added inline comments.Thu, May 28, 6:12 AM
llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
275

Fixed.

297

Moved right parts.

llvm/tools/llvm-readobj/ELFDumper.cpp
2683

Fixed in this patch, because I had to change the implementation of this method to support the case when the header can be read, but the rest of the table is broken.

grimar requested review of this revision.Thu, May 28, 6:12 AM
jhenderson accepted this revision.Fri, May 29, 2:25 AM

Looks good with one question.

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
273

Could you clarify why the no-op values have changed?

This revision is now accepted and ready to land.Fri, May 29, 2:25 AM
grimar marked an inline comment as done.Fri, May 29, 2:46 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
273

BloomFilter and HashBuckets both have one value (0x0) each:

## The number of words in the Bloom filter.
      MaskWords: [[MASKWORDS]]
## The number of hash buckets.
      NBuckets:  [[NBUCKETS]]
    BloomFilter: [ 0x0 ]
    HashBuckets: [ 0x0 ]

So previous values (2 and 3) were wrong. I guess I've took these arrays from another test,
simplified them, but forgot to change the no-op values previously...

This revision was automatically updated to reflect the committed changes.