Issue 357: New Linux file_id code doesn't persist across strip

The Linux client rewrite changed the file id to be a 16-byte XOR of the first
page of the binary. This doesn't really work very well for Breakpad, since
generally you'll dump the symbols, then strip the binaries, which will result in
different IDs for stripped and unstripped binaries. I've revived some bits of
the old code, which locates the .text section, but changed it to XOR the first
4k of that data instead of generating an MD5 hash.

I've attempted to follow the guidelines for code that runs within the
compromised process, the new code in file_id.cc that's shared uses the libc and
syscall helpers, and doesn't do any allocation. The only thing I was unsure of
was calling sys_mmap2 in LinuxDumper::ElfFileIdentifierForMapping. Is that ok?
There isn't any other way to easily figure out where the text section starts,
AFAICT.

One other caveat is that you can't use this code as written to calculate a file
ID of a binary of different word size than the binary you're running, so you
can't use a 64-bit dump_syms to dump 32-bit binaries, for example. I don't have
any pressing need for that particular configuration, but I thought I'd mention
it.

[zhanglei]

FYI, for Google Chrome, we currently dump the symbol, strip the binary, then
take the XOR signature of the stripped binary and put that into the first line
of the symbol file. See chromium's src/build/linux/dump*.

[Ted Mielczarek]

Yeah, I asked Neal and he confirmed that that's what was happening. No offense,
but I think that's a pretty crummy solution, since it means that the code in the
Breakpad tree doesn't actually work as expected, and requires you to jump
through hoops to get sane behavior.

[agl]

Generally LGTM, however, nealsid should have the final say.

http://breakpad.appspot.com/49008/diff/1/7
File src/client/linux/minidump_writer/linux_dumper.cc (right):

http://breakpad.appspot.com/49008/diff/1/7#newcode166
Line 166: uint8_t identifier[sizeof(MDGUID)])
should be on the previous line.

http://breakpad.appspot.com/49008/diff/1/7#newcode175
Line 175: return false;
sys_close(fd)

http://breakpad.appspot.com/49008/diff/1/10
File src/client/linux/minidump_writer/linux_dumper_unittest.cc (right):

http://breakpad.appspot.com/49008/diff/1/10#newcode137
Line 137: ssize_t len = readlink("/proc/self/exe", exe_name, PATH_MAX);
PATH_MAX - 1

http://breakpad.appspot.com/49008/diff/1/9
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/49008/diff/1/9#newcode552
Line 552: dumper_.ElfFileIdentifierForMapping(i, signature);
output arguments should come first.

http://breakpad.appspot.com/49008/diff/1/3
File src/common/linux/file_id.cc (right):

http://breakpad.appspot.com/49008/diff/1/3#newcode59
Line 59: static bool FindElfTextSection(const void *elf_mapped_base,
output arguments should come first.

http://breakpad.appspot.com/49008/diff/1/2
File src/tools/linux/dump_syms/Makefile (right):

http://breakpad.appspot.com/49008/diff/1/2#newcode71
Line 71: 
This looks like a mistaken change.

[mochalatte]

LGTM with some minor nits in addition to agl's feedback
Thanks, Ted.

Neal

http://breakpad.appspot.com/49008/diff/1/3
File src/common/linux/file_id.cc (right):

http://breakpad.appspot.com/49008/diff/1/3#newcode66
Line 66: const char *elf_base =
star on left

http://breakpad.appspot.com/49008/diff/1/3#newcode90
Line 90: const char *section_name = (char*)(elf_base +
* on the left here and elsewhere in file for consistency with below

http://breakpad.appspot.com/49008/diff/1/3#newcode100
Line 100: int text_section_size = text_section->sh_size;
seems like you could get rid of this temporary by assigning directly to the
output parameter

[Ted Mielczarek]

http://breakpad.appspot.com/49008/diff/1/7
File src/client/linux/minidump_writer/linux_dumper.cc (right):

http://breakpad.appspot.com/49008/diff/1/7#newcode166
Line 166: uint8_t identifier[sizeof(MDGUID)])
On 2009/12/18 23:07:29, agl wrote:
> should be on the previous line.

Sorry, what should be? The line would go over 80 characters if I put both
parameters on the same line.

http://breakpad.appspot.com/49008/diff/1/9
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/49008/diff/1/9#newcode552
Line 552: dumper_.ElfFileIdentifierForMapping(i, signature);
On 2009/12/18 23:07:29, agl wrote:
> output arguments should come first.

This actually violates the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Are you sure?

[agl]

http://breakpad.appspot.com/49008/diff/1/9
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/49008/diff/1/9#newcode552
Line 552: dumper_.ElfFileIdentifierForMapping(i, signature);
On 2009/12/23 16:50:12, Ted Mielczarek wrote:
> This actually violates the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

You can leave it as is. The style guide is flat out wrong here, but I'm too old
to care these days.