This is an archive of the discontinued LLVM Phabricator instance.

Open ELF core dumps with more than 64K sections
ClosedPublic

Authored by EugeneBi on Jan 24 2017, 12:56 PM.

Details

Summary

Problem:

There are three filelds in the ELF header - e_phnum, e_shnum, and e_shstrndx -
that could be bigger than 64K and therefore do not fit in 16 bits reserved for
them in the header. If this happens, pretty often there is a special section at
index 0 which contains their real values for these fields in the section header
in the fields sh_info, sh_size, and sh_link respectively.

Fix:

  • Rename original fields in the header declaration. We want to have them around

just in case.

  • Reintroduce these fields as 32-bit members at the end of the header. By default

they are initialized from the header in Parse() method.

  • In Parse(), detect the situation when the header might have been extended into

section info #0 and try to read it from the same data source.

  • ObjectFileELF::GetModuleSpecifications accesses some of these fields but the

original parse uses too small data source. Re-parse the header if necessary
using bigger data source.

  • ProcessElfCore::CreateInstance uses header with potentially sentinel values,

but it does not access these fields, so a comment here is enough.

Diff Detail

Event Timeline

EugeneBi created this revision.Jan 24 2017, 12:56 PM
EugeneBi added a reviewer: labath.
labath edited edge metadata.Jan 25 2017, 3:09 AM

The changes themselves look fine. However:

  • this diff seems to be based on some very old snapshot, as it still uses the old 4-char indentation and everything. Please rebase the change on current master.
  • have given any thoughts on how to test this? It sounds like testing the whole pipeline would be quite hard. But we could at least add a unit test for the ELFHeader::Parse function. It seems quite doable - just create some elf-looking data in memory and pass it to this function. Currently there is no unittests/ObjectFile/ELF folder, so please create one and put the test there.
source/Plugins/ObjectFile/ELF/ELFHeader.h
56–57

I am wondering whether these is any use in keeping these old values. It sounds like it's a recipe for someone getting confused and using the wrong ones. What do you think about just deleting these?

  1. Yes, this is based on release_39 branch. OK, I'll rebase it on current master.
  2. Sorry, I am not familiar with LLDB testing and unfortunately I do not have time to do it. I am testing it on the core dumps I have. If this is not acceptable, I'll keep using my private copy and wait until somebody with more resources to spend on LLDB picks it up.
  3. I thought about keeping vs. discarding values from the original header. I slightly inclined to keep them because:
  • They are not necessary if everything works, but if something goes wrong it could be useful to see in the debugger what was the original header
  • They are used in HasHeaderExtension method which is quite simple right now, I do not want it get complicated.
  • To avoid confusion, I added "_hdr" suffix to the original values. Well, we can invent better name - maybe "_orig" or what?
EugeneBi updated this revision to Diff 85827.Jan 25 2017, 3:43 PM

Rebased to recent master

EugeneBi abandoned this revision.Jan 25 2017, 3:49 PM

Sorry, messed up.
Will upload correct change soon

Hmm... This fix does not work with the recent sources :(
I'll debug some more tomorrow.

source/Plugins/ObjectFile/ELF/ELFHeader.h
67

Accidentally deleted this line

EugeneBi updated this revision to Diff 85931.Jan 26 2017, 10:10 AM

This compiles, builds, and run.

Unfortunately, unlike release_39 branch, I cannot open my core dump, but the problem seems unrelated.
So I think I want to push it through and continue investigation what is not working this time.

EugeneBi added inline comments.Jan 26 2017, 10:13 AM
source/Plugins/ObjectFile/ELF/ELFHeader.h
56–57

I will leave it to you - just tell me what you prefer. I see both pros and cons of my current code.

labath updated this revision to Diff 86048.Jan 27 2017, 7:16 AM
labath edited the summary of this revision. (Show Details)

Add a unittest for the header parse function

labath accepted this revision.Jan 27 2017, 7:25 AM
labath added a subscriber: lldb-commits.

I've added a quick test to demonstrate what I had in mind.

Unfortunately, unlike release_39 branch, I cannot open my core dump, but the problem seems unrelated. So I think I want to push it through and continue investigation what is not working this time.

That's kinda the reason I wanted to add tests. :)
This test would not have actually caught your new problem, but it could add at least some protection for the future, as I doubt many people will run into real files with this many sections.

I think this should be ok to put in if there are no objections from anyone else.

source/Plugins/ObjectFile/ELF/ELFHeader.h
56–57

Ok, let's leave it this way then.

This revision is now accepted and ready to land.Jan 27 2017, 7:25 AM
davidb added a subscriber: davidb.Jan 27 2017, 7:54 AM
davidb added inline comments.
source/Plugins/ObjectFile/ELF/ELFHeader.cpp
108

Would this make more sense to compare against a named constant ELF::PN_XNUM?

110

And this with SHN_UNDEF?

112

and this with ELF::SHN_XINDEX.

EugeneBi added inline comments.Jan 27 2017, 4:55 PM
source/Plugins/ObjectFile/ELF/ELFHeader.cpp
108

Would be nice, but - where is it defined? I tried ELF::PN_XNUM, elf::PN_XNUM, PN_XNUM - compiler barks anyway.

EugeneBi added inline comments.Jan 27 2017, 5:15 PM
source/Plugins/ObjectFile/ELF/ELFHeader.cpp
108

OK, I see the other two are defined in Support/ELF.h, I'll put PN_XNUM there then.

EugeneBi added inline comments.Jan 27 2017, 5:22 PM
source/Plugins/ObjectFile/ELF/ELFHeader.cpp
108

Oh, this ELF.h is a part of a different git repo - it lives in llvm. I am not sure what to do about it.

EugeneBi updated this revision to Diff 86148.Jan 27 2017, 5:51 PM

Used named constants for SHN_UNDEF and SHN_XINDEX sentinels.
Unfortunately ELF.h lacks definition of PN_XNUM, so left it as a comment.

So, what's now? Can somebody commit it, please?
I do not see any option to do it myself.

This revision was automatically updated to reflect the committed changes.

Sorry, I have been a bit busy.

I've committed this in now. Thank you for the patch.