Add an optional root prefix to Linux dumpers

Add an optional root prefix to Linux dumpers

The Linux dumpers use absolute paths for shared libraries referenced by
dumps, so they fail to locate them if the crash originated in a chroot.
This CL enables callers to specify a root prefix, which is prepended to
mapping paths before opening them.

BUG=chromium:591792
TEST=make check

[Dominik Laskowski, 2016-03-03 21:34:17]

domlaskowski@chromium.org changed reviewers:
+ lhchavez@chromium.org, vapier@chromium.org

[Luis Héctor Chávez, 2016-03-03 22:10:43]

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.cc:100: assert(root_prefix_);
Maybe also assert that my_strlen(root_prefix) < PATH_MAX?

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.cc:145: if
(!GetMappingAbsolutePath(mapping, filename))
This used to assert if the mapping name exceeded NAME_MAX. It's probably safer
if you preserve that also assert if the full |filename| exceeds PATH_MAX.

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/linux_dumper.h (right):

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.h:77: explicit LinuxDumper(pid_t
pid, const char* root_prefix = "");
The style guide seems to be slightly against default arguments (in favor of
overloading):
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[Dominik Laskowski, 2016-03-04 01:03:16]

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.cc:100: assert(root_prefix_);
On 2016/03/03 22:10:43, Luis Héctor Chávez wrote:
> Maybe also assert that my_strlen(root_prefix) < PATH_MAX?

Done.

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.cc:145: if
(!GetMappingAbsolutePath(mapping, filename))
On 2016/03/03 22:10:43, Luis Héctor Chávez wrote:
> This used to assert if the mapping name exceeded NAME_MAX. It's probably safer
> if you preserve that also assert if the full |filename| exceeds PATH_MAX.

Done.

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/linux_dumper.h (right):

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.h:77: explicit LinuxDumper(pid_t
pid, const char* root_prefix = "");
On 2016/03/03 22:10:43, Luis Héctor Chávez wrote:
> The style guide seems to be slightly against default arguments (in favor of
> overloading):
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

I think this is more readable, since overloads would be unnecessarily verbose
(especially without C++11 sugar).

[vapier, 2016-03-07 16:32:37]

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://codereview.chromium.org/1761023002/diff/1/src/client/linux/minidump_w...
src/client/linux/minidump_writer/linux_dumper.cc:145: if
(!GetMappingAbsolutePath(mapping, filename))
i don't think keeping that assert adds anything useful.  we return false in this
func when the sizes fail.

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:165:
assert(my_strlen(mapping.name) < NAME_MAX);
why bother ?  why does this matter at all ?

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:168: (assert(my_strlen(path) <
PATH_MAX), true);
i don't see the point in this assert.  we're already returning true/false based
on root_prefix_ & mapping.name fitting into the buffer.

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:255: if (mapping.exec &&
mapping.offset != 0) {
these braces aren't needed

[Dominik Laskowski, 2016-03-07 18:36:22]

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
File src/client/linux/minidump_writer/linux_dumper.cc (right):

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:165:
assert(my_strlen(mapping.name) < NAME_MAX);
On 2016/03/07 16:32:37, vapier wrote:
> why bother ?  why does this matter at all ?

It was just to preserve the assert, but I agree it's overkill. Removed.

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:168: (assert(my_strlen(path) <
PATH_MAX), true);
On 2016/03/07 16:32:36, vapier wrote:
> i don't see the point in this assert.  we're already returning true/false
based
> on root_prefix_ & mapping.name fitting into the buffer.

Removed. No real point other than paranoia about the correctness of the libc
substitute.

https://codereview.chromium.org/1761023002/diff/20001/src/client/linux/minidu...
src/client/linux/minidump_writer/linux_dumper.cc:255: if (mapping.exec &&
mapping.offset != 0) {
On 2016/03/07 16:32:37, vapier wrote:
> these braces aren't needed

True, but this file uses braces in most cases. See line 155. Also, the style
guide recommends braces for complex statements.

[vapier, 2016-03-07 23:20:41]

i won't bikeshed style too much ;)

lgtm but prob want to wait for lhchavez@ to make sure he's still happy

[elijahtaylor1, 2016-03-08 00:24:08]

elijahtaylor@chromium.org changed reviewers:
+ elijahtaylor@chromium.org

[elijahtaylor1, 2016-03-08 00:24:09]

lgtm, lhchavez@ is out for the week

[vapier, 2016-03-08 02:37:38]

pushed by hand because `git-cl` mangles things

https://chromium.googlesource.com/breakpad/breakpad/+/4d690507171bff0f29dcf38...