Page MenuHomePhabricator

[LLDB] [PECOFF] Don't crash in ReadImageDataByRVA for addresses out of range
ClosedPublic

Authored by mstorsjo on Oct 28 2019, 4:23 AM.

Details

Summary

This can happen e.g. when unwinding doesn't work perfectly.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 28 2019, 4:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 4:23 AM
aleksandr.urakov accepted this revision.Oct 28 2019, 7:27 AM

LGTM, thank you! Can you send me an example of binary on which unwinding fails with a crash, please? It is a very interesting case...

This revision is now accepted and ready to land.Oct 28 2019, 7:27 AM

LGTM, thank you! Can you send me an example of binary on which unwinding fails with a crash, please? It is a very interesting case...

It is on arm64, so it's not something that is expected to be flawless yet (even if it generally works, sometimes).

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

Ok, I'll look at it later to see if I can make some broken file to trigger this condition.

Any way to get a test for this? Maybe creating a object file with a bogus unwind RVA via yaml2obj ?

Do we have a suitable test as basis for it? I'm not quite sure which way is the most compact way of achieving that. A small couple function exe with SEH or dwarf (eh_frame) unwind info, without debug info, with a crash/int3 in a nested function? Or just some image unwind commands so it doesn't need executing?

We have some files that might be usable as a basis for this, but I don't know which one would be the best, as I don't know what you need here. What do you need to do in order to reproduce the crash? Would it be possible to just set the export table RVA to some bogus value? That should be trigerred by just constructing the module symbol table...

Ok, I'll look at it later to see if I can make some broken file to trigger this condition.

I gave this some amount of tries, but my files with broken unwind info doesn't trigger it. It happened originally unreliably on arm64.

Ok to proceed with it without a testcase?

I did test crafting a file with a bogus export table RVA as well, and that crashes lldb elsewhere, due to an unchecked Expected<> which contained an error. Will try to look into that one separately later...

Try this for patch for size:

. It needs to me moved into a separate test case, and cleaned up a bit, but I believe it should trigger the bug you're fixing here.

The tricky part about unwind info is that you currently need a Process instance around even to just inspect (= parse) it. I have ideas how to fix that, but no time to implement them :(. Until then we need to create a process somehow to test unwind info parsing.

Fortunately, these days we have a minidump files and their yaml representation, which gives us a relatively easy (though there definitely are ways to improve that too) method to create Processes from core files. I consider something like this to be the future of testing unwinding, because it's usually very hard to reliably drive a real process to a point which will exercise some specific unwind corner case. OTOH, with a minidump(core) file, you can just freeze a process in the state that you're intersted in, and the test will always test the exact same thing.

mstorsjo updated this revision to Diff 227183.Oct 30 2019, 2:52 PM

Added a testcase based on @labath 's patch. (Thanks! That managed to trigger the condition!)

labath accepted this revision.Oct 31 2019, 1:26 AM
labath added inline comments.
lldb/test/Shell/Minidump/Windows/Inputs/broken-unwind.exe.yaml
30

Maybe add a comment that this is the thing which points outside the .pdata (or any other) section.

This revision was automatically updated to reflect the committed changes.