Add support for shared libraries in archives.

Chrome on Android now supports loading the shared library directly from the APK
file.

This patch makes two changes to breakpad to enable crash reporting to work
correctly when the library is inside another file (an archive):

- Do not filter mappings which map an executable at a non-zero offset.
- If such an executable is mapped look in the ELF information for the 
shared object name and use that name in the minidump.

Note this change doesn't care about the archive format and isn't Android
specific (though loading the shared library this way is currently only done on
Android).

BUG=390618

[Lei Zhang (chromium)]

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:141: bool
LinuxDumper::ElfFileSoNameFromMappedFile(
nit: add a "// static" comment

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:165: const ElfW(Dyn) *dynamic =
static_cast<const ElfW(Dyn)*>(segment_start);
nit: " *dynamic" -> "* dynamic"

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:185: bool
LinuxDumper::ElfFileSoName(
nit: add a "// static" comment

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:194: assert(filename_len <
NAME_MAX);
make this an assert(false) in the if block below so you don't have to specify
the same condition twice?

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
File client/linux/minidump_writer/linux_dumper.h (right):

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.h:110: bool exec;   // true if the
mapping has the execute bit set.
nit: 2 spaces

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.h:185: static bool
ElfFileSoNameFromMappedFile(
This can probably be moved into linux_dumper.cc in an anonymous namespace?

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
File common/linux/memory_mapped_file.cc (right):

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:79: if (st.st_size == 0 || offset >=
st.st_size) {
Can you update the comment?

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:87: if ((offset & 4095) != 0) {
Do you need this? Would mmap2() just fail otherwise and then line 95 can handle
this?

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:89: return false;
If you keep this block, you need to sys_close(fd);

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset = 0);
no default argument:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

[anton]

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:141: bool
LinuxDumper::ElfFileSoNameFromMappedFile(
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> nit: add a "// static" comment

Done.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:165: const ElfW(Dyn) *dynamic =
static_cast<const ElfW(Dyn)*>(segment_start);
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> nit: " *dynamic" -> "* dynamic"

Done.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:165: const ElfW(Dyn) *dynamic =
static_cast<const ElfW(Dyn)*>(segment_start);
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> nit: " *dynamic" -> "* dynamic"

Done.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:185: bool
LinuxDumper::ElfFileSoName(
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> nit: add a "// static" comment

Done.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.cc:194: assert(filename_len <
NAME_MAX);
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> make this an assert(false) in the if block below so you don't have to specify
> the same condition twice?

Done. Also made the same change in the existing code at L120.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
File client/linux/minidump_writer/linux_dumper.h (right):

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.h:110: bool exec;   // true if the
mapping has the execute bit set.
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> nit: 2 spaces

Done.

https://breakpad.appspot.com/7684002/diff/90001/client/linux/minidump_writer/...
client/linux/minidump_writer/linux_dumper.h:185: static bool
ElfFileSoNameFromMappedFile(
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> This can probably be moved into linux_dumper.cc in an anonymous namespace?

Done.

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
File common/linux/memory_mapped_file.cc (right):

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:79: if (st.st_size == 0 || offset >=
st.st_size) {
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> Can you update the comment?

Done.

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:87: if ((offset & 4095) != 0) {
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> Do you need this? Would mmap2() just fail otherwise and then line 95 can
handle
> this?

The shift at L92 is discarding the bottom 12 bits, so I check that they are zero
first.

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.cc:89: return false;
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> If you keep this block, you need to sys_close(fd);

Done.

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/90001/common/linux/memory_mapped_fi...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset = 0);
On 2014/07/16 23:28:25, Lei Zhang (chromium) wrote:
> no default argument:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

Done.

[Lei Zhang (chromium)]

Generally looking good.

In the CL description, please change BUG= to chromium:NNN.

Can you add some unit tests for MemoryMappedFile and ElfFileSoName() ?

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
client/linux/minidump_writer/linux_dumper.cc:186: }  // unnamed namespace
s/unnamed //

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset);
no longer explicit

[Lei Zhang (chromium)]

I recommend getting a real Breakpad checkout and running make && make check.
Several unit tests as well as minidump-2-core.cc need updating.

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
client/linux/minidump_writer/linux_dumper.cc:172: if (dyn->d_un.d_val >=
dynstr_size) {
|dynstr_size| should to be a size_t. Let me whip up a CL to make this work for
you.

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset);
|offset| needs to be off_t, or you'll get a sign/unsigned mismatch. Same with
Map(). You'll need to #include <sys/types.h>.

[anton]

On 2014/07/17 17:45:40, Lei Zhang (chromium) wrote:
> I recommend getting a real Breakpad checkout and running make && make check.
> Several unit tests as well as minidump-2-core.cc need updating.

I found the google-breakpad site, but didn't find instructions for doing this.
Please send me a link.

>
https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
> File client/linux/minidump_writer/linux_dumper.cc (right):
> 
>
https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
> client/linux/minidump_writer/linux_dumper.cc:172: if (dyn->d_un.d_val >=
> dynstr_size) {
> |dynstr_size| should to be a size_t. Let me whip up a CL to make this work for
> you.
> 
>
https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
> File common/linux/memory_mapped_file.h (right):
> 
>
https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
> common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
> path, size_t offset);
> |offset| needs to be off_t, or you'll get a sign/unsigned mismatch. Same with
> Map(). You'll need to #include <sys/types.h>.

I can make these type changes, but need to understand more about what you want
here. These changes create ripples, so I am not sure you will like the result.

[anton]

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
client/linux/minidump_writer/linux_dumper.cc:172: if (dyn->d_un.d_val >=
dynstr_size) {
On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> |dynstr_size| should to be a size_t. Let me whip up a CL to make this work for
> you.

Not sure of the motivation behind this one.  The int is coming from
FindElfSection(). But we are not expecting a negative value nor a value greater
than 2^31.

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
client/linux/minidump_writer/linux_dumper.cc:186: }  // unnamed namespace
On 2014/07/17 17:27:26, Lei Zhang (chromium) wrote:
> s/unnamed //

Done.

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset);
On 2014/07/17 17:27:26, Lei Zhang (chromium) wrote:
> no longer explicit

Done.

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset);
On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> |offset| needs to be off_t, or you'll get a sign/unsigned mismatch. Same with
> Map(). You'll need to #include <sys/types.h>.

The size_t is coming from MappingInfo offset. There is no signed/unsigned issue
here both size_t and off_t are unsigned.

[Lei Zhang (chromium)]

See https://code.google.com/p/google-breakpad/source/checkout for how to get the
source for Breakpad proper.

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
File client/linux/minidump_writer/linux_dumper.cc (right):

https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
client/linux/minidump_writer/linux_dumper.cc:172: if (dyn->d_un.d_val >=
dynstr_size) {
On 2014/07/18 11:10:00, anton wrote:
> On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> > |dynstr_size| should to be a size_t. Let me whip up a CL to make this work
for
> > you.
> 
> Not sure of the motivation behind this one.  The int is coming from
> FindElfSection(). But we are not expecting a negative value nor a value
greater
> than 2^31.

Otherwise you'd get signed/unsigned comparison warnings and need to cast. I
already committed to ToT Breakpad.

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
File common/linux/memory_mapped_file.h (right):

https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
path, size_t offset);
On 2014/07/18 11:10:00, anton wrote:
> On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> > |offset| needs to be off_t, or you'll get a sign/unsigned mismatch. Same
with
> > Map(). You'll need to #include <sys/types.h>.
> 
> The size_t is coming from MappingInfo offset. There is no signed/unsigned
issue
> here both size_t and off_t are unsigned. 

Last I checked, off_t is signed.

[anton]

On 2014/07/18 20:29:00, Lei Zhang (chromium) wrote:
> See https://code.google.com/p/google-breakpad/source/checkout for how to get
the
> source for Breakpad proper.
> 
>
https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
> File client/linux/minidump_writer/linux_dumper.cc (right):
> 
>
https://breakpad.appspot.com/7684002/diff/160001/client/linux/minidump_writer...
> client/linux/minidump_writer/linux_dumper.cc:172: if (dyn->d_un.d_val >=
> dynstr_size) {
> On 2014/07/18 11:10:00, anton wrote:
> > On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> > > |dynstr_size| should to be a size_t. Let me whip up a CL to make this work
> for
> > > you.
> > 
> > Not sure of the motivation behind this one.  The int is coming from
> > FindElfSection(). But we are not expecting a negative value nor a value
> greater
> > than 2^31.
> 
> Otherwise you'd get signed/unsigned comparison warnings and need to cast. I
> already committed to ToT Breakpad.

Done. Thanks.

> 
>
https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
> File common/linux/memory_mapped_file.h (right):
> 
>
https://breakpad.appspot.com/7684002/diff/160001/common/linux/memory_mapped_f...
> common/linux/memory_mapped_file.h:50: explicit MemoryMappedFile(const char*
> path, size_t offset);
> On 2014/07/18 11:10:00, anton wrote:
> > On 2014/07/17 17:45:41, Lei Zhang (chromium) wrote:
> > > |offset| needs to be off_t, or you'll get a sign/unsigned mismatch. Same
> with
> > > Map(). You'll need to #include <sys/types.h>.
> > 
> > The size_t is coming from MappingInfo offset. There is no signed/unsigned
> issue
> > here both size_t and off_t are unsigned. 
> 
> Last I checked, off_t is signed.

Yes, you are right, bit surprising. I just added an explicit cast to handle
this. There is already a check for non-negative, so this should be safe.

[Lei Zhang (chromium)]

lgtm

[rmcilroy]

On 2014/07/21 21:13:54, Lei Zhang (chromium) wrote:
> lgtm

Landed as r1355