Page MenuHomePhabricator

Fix for Bug 25338
ClosedPublic

Authored by ravitheja on Jan 12 2016, 3:04 AM.

Details

Summary

The issue arises because LLDB is not
able to read the vdso library correctly.
The fix adds memory allocation callbacks
to allocate sufficient memory in case the
requested offsets don't fit in the memory
buffer allocated for the ELF.

Diff Detail

Event Timeline

ravitheja updated this revision to Diff 44616.Jan 12 2016, 3:04 AM
ravitheja retitled this revision from to Fix for Bug 25338.
ravitheja updated this object.
labath edited edge metadata.Jan 12 2016, 4:11 AM

Could you explain what is the nature of the failure in the vdso parsing? Otherwise it's quite hard to see what's the relationship of your patch to it, as it seems that the ObjectFile should be able to handle files which are not fully contained within the initial read (comment on Module::GetMemoryObjectFile says that it's enough if size_to_read contains the file header).

Also, if this fixes a test (I think I remember us disabling a test because of this), can you please enable it again?

include/lldb/Target/Process.h
3145 ↗(On Diff #44616)

This definition sounds confusing to me. I would read this as the "start address of the last block ...", which is clearly not what you want. It's probably enough to change it to "*end* address of the last block".

source/Plugins/Process/Linux/NativeProcessLinux.cpp
2973 ↗(On Diff #44616)

This will be wrong if the file contains multiple entries for the same file (probably because different segments are mapped with different permissions). Doesn't happen for the vdso, but all other files have this. You should probably look for the *last* segment matching this file and get it's end address.

source/Utility/StringExtractorGDBRemote.cpp
149 ↗(On Diff #44616)

Could you also document this packet in docs/lldb-gdb-remote.txt?

Could you explain what is the nature of the failure in the vdso parsing?

Strike that. I see that you have linked the bug # in the title...

tberghammer edited edge metadata.Jan 12 2016, 5:06 AM

Can we calculate the size of the object file based on elf headers and the section headers instead of using the /proc file system?

I think a good algorithm would be to take the maximum of the following 2 expression:

Start of section header + Number of section headers x  Size of section headers
Max (for each section: section file address + section size)
ovyalov edited edge metadata.Jan 12 2016, 11:31 AM

vdso issue is related to ELF format and I'm wondering whether we can keep the fix contained in ObjectFileELF - for example, if a section that we're trying to read is beyond the boundaries of memory buffer we can read its content from inferior's memory using ObjectFile::m_process_wp.

Hi Oleyksiy, actually the problem is not with the Elf Format but its a problem with the ObjectFileElf itself which cannot handle reading from arbitrary offsets in the Elf. Now the ideal solution would be to fix that but as we had discussed last year we agreed to allocate more memory for the vdso. I could try to contain the complete fix in the ObjectFileElf but that may be counterproductive since it already buggy ?

Hi Tamas, a modified algorithm may work in this case for now, but this issue may still reoccur for some other Elf Format .

Based on your description the vdso file itself looks like a valid elf file to me. If the ObjectFileELF plugin is buggy (it is certainly is as it can't handle the vdso) then I think we should fix that instead of building up a new approach just to work around one of our own bug.

Yes the vdso is fine, last year I did discuss this and we here decided that at the moment we can only offer this solution.

If vdso bug pertains to ELF format then it looks reasonable to keep the fix within ObjectFileELF.
I experimented a while ago with making ObjectFileELF to read from arbitrary offsets - please see http://reviews.llvm.org/D16151 as reflection of this idea (patch is pretty old and cannot be applied as-is).
Please take a look into it and let us know whether such approach may solve vdso problem.

ravitheja updated this revision to Diff 45185.Jan 18 2016, 7:37 AM
ravitheja edited edge metadata.

Changes according to D16151

I think this looks *much* nicer than the previous version.

I personally would replace the std::function dance with something simpler such as bool use_memory_fallback, but I guess that's a matter of taste.

source/Plugins/ObjectFile/ELF/ObjectFileELF.h
13

Please put this right before <vector>

284

const non-reference arguments are somewhat pointless. Did you forget a reference here?

307

same here

ravitheja updated this object.Jan 19 2016, 2:31 AM
ravitheja updated this revision to Diff 45238.Jan 19 2016, 2:51 AM

correcting previous revision.

Looks good as far as I am concerned. @tberghammer, @ovyalov ?

tberghammer accepted this revision.Jan 19 2016, 4:57 AM
tberghammer edited edge metadata.

Looks good

This revision is now accepted and ready to land.Jan 19 2016, 4:57 AM
ravitheja closed this revision.Jan 19 2016, 4:59 AM
ldrumm added a subscriber: ldrumm.Jan 25 2016, 6:45 AM

This differential seems to have introduced a regression on x86_64 Android, where std::bad_alloc is thrown during process attach. I've reverted this locally, and filed a bug, but as this is code I'm not familiar with, I'd appreciate it if @ravitheja were able to take a look at this again.

Hello, Ok I will take a look.

ravitheja added a comment.EditedJan 28 2016, 2:51 AM

Hello @ldrumm,

I did investigate the issue and my finding is that incorrect values are being passed to the SetDataWithReadMemoryFallback function.
Now I suspect the ElfSectionHeader is wrongly parsed in your case and the size and offsets obtained are wrong. To talk briefly about r258122, it just tries to allocate enough memory for Elf files being loaded by lldb and earlier it would not have read the Elf file so you wouldnt have seen this issue because it would have just returned. Could you maybe check the ElfSectionHeader::Parse function, and see the values it obtains for this elf (mainly the sh_offset and sh_size fields), maybe print them in logs ? I currently don't have an Android x86_64 environment.