Modify minidump_stackwalk to be more tolerant of overlapping ranges.

Modify minidump_stackwalk to be more tolerant of overlapping ranges.

These ranges can be seen in some Android minidumps.

BUG=chromium:439531
R=mark@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1412

[Will Harris]

add mark

[Mark Mentovai]

The reason that this was fatal is that it’s impossible to decide which module
contributed the conflicting pages to a process’ memory map. This change states
that the first module that appears wins all of the pages, and subsequent ones
won’t be reachable at all, for conflicting and non-conflicting pages alike.

When you’re seeing these overlapping ranges, what does the geometry of the
modules in the minidump actually look like? Where did the dump come from, and
what wrote it? How were the modules actually loaded in the process’ address
space?

[Will Harris]

there's more details in the linked bug (command #47)

but the modules look like

61318000 61418000   /dev/ashmem/dalvik-jit-code-cache (deleted)
6133e000 61418000   /dev/ashmem/dalvik-jit-code-cache (deleted)

the dump is from a Chrome on Android crash.

[Will Harris]

>and subsequent ones won’t be reachable at all

Also, I understand we might end up in a non-deterministic state if we do not
abort on overlapping modules, but currently we abort the module loading
processing completely, meaning that minidump_stackwalk produces even more
invalid stacks than if the processing were to just continue.  It's just
different degrees of invalidity.

Another option here would be to take the larger of the two overlapping regions,
or ignore the error, if the name of the loaded module is the same (e.g. in this
case).

[Mark Mentovai]

My preference would be to:

1. make sure we only tolerate this situation on dumps coming from Android.
2. kick both modules out of the map, and poison the address space that would
have been used by both modules so that a third module that shows up with a
collision will also get kicked out.

If it’s only happening for /dev/ashmem/dalvik-jit-code-cache, then we should
make sure that we only tolerate it and kick the modules out if the module name
matches.

[Will Harris]

I now ignore duplicate entries if they start with "/dev/ashmem".  If you would
prefer I can just throw out all entries that match
"/dev/ashmem/dalvik-jit-code-cache".

[Mark Mentovai]

This is why the dump-writer should suspend the crashy process while it’s doing
its thing.

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2551: // /dev/ashmem can contain duplicate entries for
JITted code.
This comment only makes it half-obvious that this was done for Android. It
should make it fully obvious.

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2552: const string kDevAshmem("/dev/ashmem");
Anchor it with a / on the other side too.

[Will Harris]

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2551: // /dev/ashmem can contain duplicate entries for
JITted code.
On 2014/12/19 14:58:10, Mark Mentovai wrote:
> This comment only makes it half-obvious that this was done for Android. It
> should make it fully obvious.

Done.

https://breakpad.appspot.com/9744002/diff/120001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2552: const string kDevAshmem("/dev/ashmem");
On 2014/12/19 14:58:10, Mark Mentovai wrote:
> Anchor it with a / on the other side too.

Done.

[Mark Mentovai]

https://breakpad.appspot.com/9744002/diff/170001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/9744002/diff/170001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2552: // duplicate entries for JITted code, so ignore
these.
Leave a link to the bug too and LGTM.

[Will Harris]

thanks for the review.

https://breakpad.appspot.com/9744002/diff/170001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/9744002/diff/170001/src/processor/minidump.cc#ne...
src/processor/minidump.cc:2552: // duplicate entries for JITted code, so ignore
these.
On 2014/12/19 20:39:27, Mark Mentovai wrote:
> Leave a link to the bug too and LGTM.

Done.

[Will Harris]

Committed patchset #5 (id:210001) manually as r1412 (presubmit successful).