689184 - Minidumps never contain MD_LINUX_DSO_DEBUG info

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Mike Hommey [:glandium]]

MD_LINUX_DSO_DEBUG info is gotten from DT_DEBUG field in the dynamic section. Except the minidump writer takes it from _DYNAMIC, which corresponds to the dynamic section of where the minidump writer is linked, which happens to be libxul.so. In practice, the DT_DEBUG value is only available in the dynamic section of executables, which means we're effectively never finding a DT_DEBUG value in the libxul.so dynamic section, and thus never get that info.

Comment 1

[Mike Hommey [:glandium]]

Attachment: part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 2 - Use the program PT_DYNAMIC segment for the MD_LINUX_DSO_DEBUG stream

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 562739 [details] [diff] [review]
part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Review of attachment 562739 [details] [diff] [review]:
-----------------------------------------------------------------

This is generally ok, but r- for the use of std::map.

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
@@ +301,5 @@
>    // load location and special case it's entry when creating the list
>    // of mappings.
> +  // We use the AT_SYSINFO_EHDR entry for linux-gate.so
> +  // See http://www.trilithium.com/johan/2005/08/linux-gate/ for more
> +  // information.

Comments in Breakpad code are supposed to be all third-person (so avoid I and we). I realize the existing comment here uses first-person, so maybe you can change that too? If you need wording suggestions I can probably provide them

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.h
@@ +131,5 @@
>  bool GetThreadRegisters(ThreadInfo* info);
>  
>  class LinuxDumper {
>   public:
> +  typedef std::map<unsigned, elf_aux_val_t> auxv_map;

I don't think you can use std::map here, since this code runs in the compromised process. Note everything is using wasteful_vector<T> below.

@@ +185,5 @@
>  
>    bool threads_suspended_;
>    wasteful_vector<pid_t> threads_;  // the ids of all the threads
>    wasteful_vector<MappingInfo*> mappings_;  // info from /proc/<pid>/maps
> +  auxv_map auxv_; // info from /proc/<pid>/auxv

Super-nit: Google style says two spaces after the last character before the //.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #3)
> >  class LinuxDumper {
> >   public:
> > +  typedef std::map<unsigned, elf_aux_val_t> auxv_map;
> 
> I don't think you can use std::map here, since this code runs in the
> compromised process. Note everything is using wasteful_vector<T> below.

Mmmm this runs in the forked process, but maybe there are modes where there's no forked process?

Comment 5

[(not currently active) Ted Mielczarek]

No, there's always a forked process:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/linux/handler/exception_handler.cc#398

but it's cloned from the signal handler, so it shares the memory space of the (already crashed) process, so allocating memory is bad, AIUI.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #5)
> No, there's always a forked process:
> http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/
> linux/handler/exception_handler.cc#398
> 
> but it's cloned from the signal handler, so it shares the memory space of
> the (already crashed) process, so allocating memory is bad, AIUI.

We could get off the signal handler, then, but that's a different story. Not using std::map should be easy enough.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 562741 [details] [diff] [review]
part 2 - Use the program PT_DYNAMIC segment for the MD_LINUX_DSO_DEBUG stream

Review of attachment 562741 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc
@@ +1025,5 @@
> +    if (!phnum || !phdr)
> +      return false;
> +
> +    // Assume the program base is at the beginning of the same page as the PHDR
> +    base = (char *) ((uintptr_t)phdr & ~0xfff);

You're inconsistent about what type of casts you use. You should probably use reinterpret_cast throughout.

@@ +1046,5 @@
> +    ElfW(Dyn) *dynamic = (ElfW(Dyn) *) (dyn_addr + base);
> +
> +    // The dynamic linker makes information available that helps gdb find all
> +    // DSOs loaded into the program. If we can access this information, we dump
> +    // it to a MD_LINUX_DSO_DEBUG stream.

I always get chided to not use first person pronouns in Breakpad comments, so try to avoid them. (They do creep in regardless.)

@@ +1058,5 @@
> +      if (dyn.d_tag == DT_DEBUG) {
> +        r_debug = (struct r_debug*)dyn.d_un.d_ptr;
> +        continue;
> +      } else if (dyn.d_tag == DT_NULL)
> +        break;

Brace this else if block, please.

Comment 8

[Mike Hommey [:glandium]]

Attachment: part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Comment 9

[Mike Hommey [:glandium]]

Attachment: part 2 - Use the program PT_DYNAMIC segment for the MD_LINUX_DSO_DEBUG stream

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 564177 [details] [diff] [review]
part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Review of attachment 564177 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a unit test for this somewhere in here:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/linux/minidump_writer/linux_dumper_unittest.cc ?

We do have a test there that linux-gate.so is present, maybe that's sufficient?

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
@@ +166,4 @@
>  }
>  
>  bool LinuxDumper::Init() {
> +  return ReadAuxv(&auxv_) &&

If auxv_ is already a member, why bother passing it as a parameter? (I guess we already do the same silly thing with EnumerateThreads/EnumerateMappings...)

Comment 11

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #10)
> >  bool LinuxDumper::Init() {
> > +  return ReadAuxv(&auxv_) &&
> 
> If auxv_ is already a member, why bother passing it as a parameter? (I guess
> we already do the same silly thing with
> EnumerateThreads/EnumerateMappings...)

Yeah, I used the same idiom.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 564179 [details] [diff] [review]
part 2 - Use the program PT_DYNAMIC segment for the MD_LINUX_DSO_DEBUG stream

Review of attachment 564179 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a unit test for this? Considering it's been broken for quite a while it'd be nice to make sure it works properly!
http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/linux/minidump_writer/minidump_writer_unittest.cc

Other tests in that file write minidumps, then read them in with the Minidump class. It's probably enough to do that and verify that the MD_LINUX_DSO_DEBUG stream exists.

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/minidump_writer.cc
@@ +1045,5 @@
> +
> +    ElfW(Dyn) *dynamic = reinterpret_cast<ElfW(Dyn) *>(dyn_addr + base);
> +
> +    // The dynamic linker makes information available that helps gdb find all
> +    // DSOs loaded into the program. If this information is indeed avaible,

"available"

Comment 13

[Mike Hommey [:glandium]]

Attachment: part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Refreshed to actually work as intended, and while at it, removed the argument to ReadAuxv, which fits better on upstream trunk.
The patch against upstream trunk is slightly different, but essentially the same.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 643884 [details] [diff] [review]
part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Review of attachment 643884 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/linux_dumper.cc
@@ +304,5 @@
>    BuildProcPath(maps_path, pid_, "maps");
>  
>    // linux_gate_loc is the beginning of the kernel's mapping of
>    // linux-gate.so in the process.  It doesn't actually show up in the
> +  // maps list as a filename, so we use the AT_SYSINFO_EHDR aux vector

Can you reword this to get rid of the "we" while you're here?

Perhaps something like:
"It doesn't actually show up in the maps list as a filename, but it can be found using the AT_SYSINFO_EHDR aux vector entry, which gives the information necessary to special case its entry when creating the list of mappings."

Comment 15

[Mike Hommey [:glandium]]

Attachment: part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Refreshed with review comment

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 645244 [details] [diff] [review]
part 1 - Keep ELF auxiliary vectors values within LinuxDumper

Carrying r+ over.

Comment 17

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8db1068216cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd0bf639650

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8db1068216cc
https://hg.mozilla.org/mozilla-central/rev/5dd0bf639650