Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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
71

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
58–59

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
71

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
71

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
240

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

242

And this with SHN_UNDEF?

244

and this with ELF::SHN_XINDEX.

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

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
240

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
240

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.