Page MenuHomePhabricator

llvm-objdump can error out with an unexpected EOF error if .bss is larger than the file being dumped.
ClosedPublic

Authored by sidneym on Oct 18 2019, 1:41 PM.

Details

Summary

llvm-objdump -D this file:

int a[100000];

int
main() {
  return 0;
}

Will produce an error, "The end of the file was unexpectedly encountered"

This happens because of a check in Binary.h checkOffset. (Addr + Size > M.getBufferEnd()).

Since the .bss section doesn't occupy space in the file this check fails when the above program is dumped with -D.

This change avoids the reading .bss sections.

I added a test for X86_64 but this isn't target specific.

Diff Detail

Event Timeline

sidneym created this revision.Oct 18 2019, 1:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 1:41 PM
MaskRay added a comment.EditedOct 18 2019, 7:54 PM

inline Expected<StringRef> SectionRef::getContents() const { is called 4 times in llvm-objdump.cpp. They probably should all be replaced. The method should special case SHT_NOBITS - the sh_offset field of a SHT_NOBITS section is ignored. For a GNU ld linked binary, sh_offset may be greater than the file size (lld does not do this).

Please don't check in a prebuilt binary. I think we can construct a test with yaml2obj.

grimar added inline comments.Oct 21 2019, 2:08 AM
llvm/test/tools/llvm-objdump/X86/bss.test
1 ↗(On Diff #225683)

Use ## for comments (this is inline with most of the new tests we introduce for llvm tools like this one).

sidneym updated this revision to Diff 225935.Oct 21 2019, 12:22 PM

Changed getContents to return a zero-length array when the section type was NOBITS.

I looked at the other locations in that file and they all seemed to be set up to avoid dumping bss but not the potential of a non-bss/nobits section. Adding this to getContents should address those sections and usages by other tools.

I've never used yaml before so this might not be the perfect test but it sure feels better than adding binaries to the test tree.

The code change looks good to me.
Comments/suggestions are below.

llvm/test/tools/llvm-objdump/X86/bss.test
1 ↗(On Diff #225935)

"bss.test" is probably not a goodname.
I'd suggest "disassemble-bss.test" (it is similar to other disassemble-*.test we have).

4 ↗(On Diff #225935)

I'd probably change this. Instead of testing the error message which might change
at any time, you can check that .bss is disassembled:

# CHECK: Disassembly of section .bss:
# CHECK: 0000000000000000 .bss:
# CHECK-NEXT:		...

I'd also don't optimize the invocations that much and use files:

yaml2obj %s -o %t
llvm-objdump -D %t | FileCheck %s

It helps to debug tests that fails. With that it is easy to copy paste the
invocation line from the output and have a file created for a futher inspection.

5 ↗(On Diff #225935)

nit: add an empty line between check and yaml.

25 ↗(On Diff #225935)

You can simplify to:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
Sections:
  - Name:  .bss
    Type:  SHT_NOBITS
    Flags: [ SHF_WRITE, SHF_ALLOC ]
    Size:  0x0000000000001000
sidneym updated this revision to Diff 226073.Oct 22 2019, 12:32 PM

Rename and update test.

MaskRay added inline comments.Oct 23 2019, 4:11 PM
llvm/include/llvm/Object/ELFObjectFile.h
727

Delete + sh_offset to solve the problem I described in a previous comment.

grimar added inline comments.Oct 24 2019, 12:59 AM
llvm/include/llvm/Object/ELFObjectFile.h
727

It is strange to know that sh_offset can be greater than the file size.

If we want to have this change and not error out, I'd add a comment and a one more test case for
this (yaml2obj allows overriding the sh_offset with use of "ShOffset" field).

llvm/test/tools/llvm-objdump/X86/elf-disassemble-bss.test
17

As I mentioned earlier, you do not need this, Address and AddressAlign fields below.

sidneym marked an inline comment as done.Oct 24 2019, 6:40 AM
sidneym added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
727

I don't think sh_offset will be larger than the file size but sh_offset+sh_size can be and that would cause checkOffset to fail. I guess one could argue that if sh_offset is outside the scope of the file something is really bogus about the file but if the section NOBITS maybe it doesn't matter.

LG after grimar's comment is addressed.

llvm/include/llvm/Object/ELFObjectFile.h
727

It can be larger than the file size with some usage of objcopy. We can proceed with this change for now. I'll come up a test later.

sidneym updated this revision to Diff 226453.Oct 25 2019, 10:19 AM

Remove +sh_offset update testcase to include a test for ShOffset > file size.

sidneym updated this revision to Diff 226454.Oct 25 2019, 10:24 AM

Forgot to remove Address/AddressAlign

MaskRay accepted this revision.Oct 25 2019, 10:41 AM

Thanks for the update. sh_offset > file size is what GNU objcopy may do. LG but please wait for others' opinions.

This revision is now accepted and ready to land.Oct 25 2019, 10:41 AM

Thanks for the update. sh_offset > file size is what GNU objcopy may do. LG but please wait for others' opinions.

LGTM

grimar accepted this revision.Oct 28 2019, 1:49 AM

LGTM.

MaskRay added inline comments.Oct 28 2019, 11:25 AM
llvm/test/tools/llvm-objdump/X86/elf-disassemble-bss.test
33

Delete Entry

sidneym updated this revision to Diff 226771.Oct 28 2019, 3:34 PM

Remove Entry: from testcase

Can one of the reviewers push this? I lost my commit access, waiting for it to be restored.

Thanks.

Can one of the reviewers push this? I lost my commit access, waiting for it to be restored.

Thanks.

Will do that shortly.

This revision was automatically updated to reflect the committed changes.