637316 - Minidumps don't contain all mappings for elfhacked libraries

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Mike Hommey [:glandium]]

With elfhack, there are two PT_LOAD segments to map the executable code instead of one on standard binaries. More precisely, there is one for the headers, dynamic symbols, etc. up to relocations, and one for the code.

Minidumps only contain information for the first one (the least relevant), and this leads to crash dump bustages (see bug 637243).

Comment 1

[Mike Hommey [:glandium]]

This is where the problem is:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc#828

Comment 2

[Mike Hommey [:glandium]]

(In reply to comment #1)
> http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc#828

Removing this line results in the proper information being written in the minidump, but stackwalk can't make sense off it.

Comment 3

[Mike Hommey [:glandium]]

While working on programs for bug 637680, I figured what needed to be done for this bug. We actually don't need to have breakpad supporting multiple mappings for a module. Reality is that even when there are several PT_LOADs (which is the common case, since there are usually 2), the dynamic linker allocates a single memory range and then maps each part corresponding to the PT_LOAD data inside that memory range. This is why there are private mappings with no permissions between each mapping on the same file for loaded libraries.

So what we really need to do is to reconstruct this memory range. This is actually very trivial on Android (pushed something to try that I hope will work), and shouldn't be very hard in minidump_writer for linux.

Note that at the moment, Android minidumps do include several mappings for each module, and that doesn't work properly, because the base address for the second mapping on a same file is wrong, which is why it so blatantly fail with elfhack on android, but differently than on linux. It hadn't made any problem so far because the second mapping is data, and we don't have functions there.

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 1 - Report whole memory mapping to crash reporter for libraries on Android

See comment 3.

Comment 5

[Mike Hommey [:glandium]]

Attachment: part 2 - Merge adjacent mappings with the same name into one module in crash reporter

Tested on Linux. I'm going to test if it changes anything on Android (it really shouldn't)

Comment 6

[Mike Hommey [:glandium]]

> Tested on Linux. I'm going to test if it changes anything on Android (it really
> shouldn't)

... and it doesn't break anything on Android.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 516242 [details] [diff] [review]
part 1 - Report whole memory mapping to crash reporter for libraries on Android

I am punting this to mwu.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 516242 [details] [diff] [review]
part 1 - Report whole memory mapping to crash reporter for libraries on Android

>+    report_mapping(si->name, si->base, (si->size + PAGE_MASK) & (~PAGE_MASK), 0);
>+

si->size is already rounded up to the nearest page. (calculated in get_lib_extents)

Were you able to verify that you could crash and get a stack? The previous mapping reporting gave direct mappings between addresses and offsets within the library file. With a single mapping, I'm not sure this is necessarily the case except for PT_LOAD sections where offset == vaddr. I'm guessing that the crash reporter stuff doesn't actually care about the offset within the library file so this is all fine.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> Were you able to verify that you could crash and get a stack? The previous
> mapping reporting gave direct mappings between addresses and offsets within the
> library file. With a single mapping, I'm not sure this is necessarily the case
> except for PT_LOAD sections where offset == vaddr. I'm guessing that the crash
> reporter stuff doesn't actually care about the offset within the library file
> so this is all fine.

I did test. Note the offset is actually very much irrelevant as it was reporting the file offset, and not the vaddr offset. Breakpad doesn't use that anyways. The good thing with having only one mapping for the whole module is that vaddrs *are* right for all the PT_LOADed segments, which was not the case before for all but the first segment.

Comment 10

[Michael Wu [:mwu]]

(In reply to comment #9)
> I did test. Note the offset is actually very much irrelevant as it was
> reporting the file offset, and not the vaddr offset. Breakpad doesn't use that
> anyways. The good thing with having only one mapping for the whole module is
> that vaddrs *are* right for all the PT_LOADed segments, which was not the case
> before for all but the first segment.

Mmm ok, sounds about right.

Guess we can also kill the offset arg in report_mapping.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 516278 [details] [diff] [review]
part 2 - Merge adjacent mappings with the same name into one module in crash reporter

This needs to wind up upstream, but I can land it there for you, assuming it applies cleanly to Breakpad SVN.

>diff --git a/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc b/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
>--- a/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
>+++ b/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
>@@ -319,35 +319,46 @@ LinuxDumper::EnumerateMappings(wasteful_
>     uintptr_t start_addr, end_addr, offset;
> 
>     const char* i1 = my_read_hex_ptr(&start_addr, line);
>     if (*i1 == '-') {
>       const char* i2 = my_read_hex_ptr(&end_addr, i1 + 1);
>       if (*i2 == ' ') {
>         const char* i3 = my_read_hex_ptr(&offset, i2 + 6 /* skip ' rwxp ' */);
>         if (*i3 == ' ') {
>+          const char* name = NULL;
>+          // Only copy name if the name is a valid path name, or if
>+          // we've found the VDSO image

Can you fix this comment while you're here? Apparently Google style is to not use first-person pronouns, but there's a bit of inconsistency. You might say "or if this is the VDSO image"

>+          // Merge adjacent mappings with the same name into one module,
>+          // assuming they're a single library mapped by the dynamic linker
>+          if (name && result->size()) {
>+            MappingInfo* module = (*result)[result->size() - 1];
>+            if ((start_addr == module->start_addr + module->size) &&
>+                (my_strlen(name) == my_strlen(module->name)) &&
>+                (memcmp(name, module->name, my_strlen(name)) == 0)) {

memcmp is usually an intrinsic, right? This file otherwise tries to avoid using libc or syscalls directly, to avoid entering the dynamic linker.

Looks fine otherwise.

Comment 12

[Mike Hommey [:glandium]]

(In reply to comment #11)
> Comment on attachment 516278 [details] [diff] [review]
> part 2 - Merge adjacent mappings with the same name into one module in crash
> reporter
> 
> This needs to wind up upstream, but I can land it there for you, assuming it
> applies cleanly to Breakpad SVN.

Interestingly, it doesn't. I'll check the differences.
 
> memcmp is usually an intrinsic, right? This file otherwise tries to avoid using
> libc or syscalls directly, to avoid entering the dynamic linker.
> 
> Looks fine otherwise.

According to gcc documentation, it is not an intrisic, but it is a builtin. OTOH, there is my_strncmp().

Comment 13

[Mike Hommey [:glandium]]

Attachment: part 2 - Merge adjacent mappings with the same name into one module in crash reporter

Carrying over r+.

Same patch, that applies on breakpad trunk (only difference was in the comment you mentioned, which you already fixed upstream), using my_strncmp instead of memcmp.

Comment 14

[Mike Hommey [:glandium]]

http://hg.mozilla.org/projects/build-system/rev/d5bb42e60c93
http://hg.mozilla.org/projects/build-system/rev/0615993bf269

And a forgotten piece:
http://hg.mozilla.org/projects/build-system/rev/e7ba53628e3e

Comment 15

[Mike Hommey [:glandium]]

Attachment: part 2 - Merge adjacent mappings with the same name into one module in crash reporter, as landed

As landed.

Comment 16

[(not currently active) Ted Mielczarek]

Landed upstream after I wrote a unittest:
http://code.google.com/p/google-breakpad/source/detail?r=781

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/d5bb42e60c93
http://hg.mozilla.org/mozilla-central/rev/0615993bf269
http://hg.mozilla.org/mozilla-central/rev/e7ba53628e3e