This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Fix crashes and misbehaviors when reading strings from broken string tables.
ClosedPublic

Authored by grimar on Apr 1 2020, 7:27 AM.

Details

Summary

There are cases when we either might print garbage or crash when
reading strings for dumping dynamic tags.

For example when a string table is not null-terminated or goes past the EOF.
This patch fixes issues mentioned.

Diff Detail

Event Timeline

grimar created this revision.Apr 1 2020, 7:27 AM

Hmmm... my first instinct here is that we should print a single warning out-of-line, and then use the "<?>" pattern in place of the invalid strings. That probably goes for the existing messages too. What do you think about that idea?

grimar added a comment.Apr 1 2020, 7:57 AM

Hmmm... my first instinct here is that we should print a single warning out-of-line, and then use the "<?>" pattern in place of the invalid strings. That probably goes for the existing messages too. What do you think about that idea?

Yeah, I've though about that we might want to change the code to this behavior as it would be more consistent with all kinds of other errors we have.
I'll prepare a patch to change the existent behavior first then.

grimar planned changes to this revision.Apr 1 2020, 8:00 AM
grimar updated this revision to Diff 256015.Apr 8 2020, 7:12 AM
  • Updated after recent changes done by D77399.
MaskRay added inline comments.Apr 11 2020, 9:41 PM
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
336

Not terminated by a NUL.

or

Not null terminated

364

This can be omitted

grimar updated this revision to Diff 256956.Apr 13 2020, 3:40 AM
  • Addressed review comments.
MaskRay accepted this revision.Apr 13 2020, 9:05 AM

Thanks! Please wait a bit for @jhenderson's opinions (I think today is a public holiday (Easter Monday))

This revision is now accepted and ready to land.Apr 13 2020, 9:05 AM
jhenderson added inline comments.Apr 14 2020, 1:58 AM
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
289

--implicit-check-not=warning: is probably better to avoid weird failures with path names involving "warning" (there's bound to be somebody out there...).

Same below.

293

I might be inclined to move these checks to below all the RUN lines for the different cases, as the use of NOT-TERMINATED-LESS is a little confusing when nothing up to that point in the test file had actually speciifed it.

384

strings are dumped because the file ends with the zero byte

What guarantees this? Should we add something to either a) validate it (to make it easier to identify the problem if it doesn't end with a zero byte) or b) force it to always be zero?

385

Delete one of the "as a".

llvm/tools/llvm-readobj/ELFDumper.cpp
2576–2578

I think you can make the message slightly more concise:

"string table at offset 0x12345678 with size 0x87654321 goes past the end of the file (0x22222222)"

2585

Unrelated I know, but this should be ':' not ',' for more correct grammar.

2592

I'd change ',' to ':' here.

grimar marked an inline comment as done.Apr 14 2020, 2:12 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
384

But should we care/do something more? The intention of this test is to document the behavior when "the value of DT_STRSZ tag is set so that the string table ends right before the EOF". In this case dumping of strings is a side effect, so I've put a "Note". If the file had no zero byte at the end, we would dump "<?>", but what we really want to check is that we do not crash here and in the "Case B" below.

grimar marked an inline comment as done.Apr 14 2020, 2:14 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
375

I guess this is misleading. What about I change this to:

"Document the behaviort when the dynamic string table ends past the end of the file. Check we do not crash."

grimar updated this revision to Diff 257293.Apr 14 2020, 5:05 AM
grimar marked 8 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
2585

Fixed (to be consistent with new messages around).

jhenderson added inline comments.Apr 16 2020, 1:35 AM
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
375

Maybe this could be "Check that we emit an appropriate warning when the dynamic string table ends past the end of the file."

377

Add "No warning should be emitted." here, I think.

384–385

I think if we rewrite this note slightly, we could clarify more that it's not important what the final byte is:

"Note: The code reads the data in [DT_STRTAB, DT_STRTAB + DT_STRSZ] as the string table as normal. Since the file ends with a zero byte, strings are dumped, but if it didn't, we'd get <?> printed instead. The important bit is that we don't get the past the end warning."

grimar updated this revision to Diff 258291.Apr 17 2020, 4:42 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test
384–385

Reads better, thanks.

MaskRay accepted this revision.Apr 17 2020, 7:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 4:48 AM