Linux: Attempt to generate an ELF identifier for deleted running binaries.

Linux: Attempt to generate an ELF identifier for deleted running binaries.

r733

[Lei Zhang (chromium)]

When an user is running Google Chrome version N, and then an upgrade changes the
on-disk copy to version N+1, the entry for /opt/google/chrome/chrome in
/proc/[pid]/maps and /proc/[pid]/exe changes to "/opt/google/chrome/chrome
(deleted)".

If the running copy encounters a crash, the crash handler will read the maps
file and then try to generate the ELF identifier for "/opt/google/chrome/chrome
(deleted)", which does not exist. Thus on the crash server we get crash reports
that we cannot symbolize.

With this CL, if we assume nobody ever names their program "foo (deleted)", we
check entries in maps for " (deleted)" at the end and check for a match against
/proc/[pid]/exe. Upon a successful match, generate the ELF identifier from
/proc/[pid]/exe instead, and modify the map to remove the " (deleted)" suffix.

http://breakpad.appspot.com/228001/diff/1/2
File src/client/linux/minidump_writer/linux_dumper.cc (left):

http://breakpad.appspot.com/228001/diff/1/2#oldcode39
Line 39: #include <elf.h>
Removing headers already included in linux_dumper.h.

[kmixter]

Just a quick drive-by comment: doesn't this mean that the the resulting minidump
will be symbolized against Chrome N+1 when it's really a crash in Chrome N, so
it will have inaccurate line numbers, etc?

[Lei Zhang (chromium)]

On 2010/11/11 02:08:41, kmixter wrote:
> Just a quick drive-by comment: doesn't this mean that the the resulting
minidump
> will be symbolized against Chrome N+1 when it's really a crash in Chrome N, so
> it will have inaccurate line numbers, etc?

No, even though Chrome N has been deleted off the disk, it's still accessible
via /proc/[pid]/exe. We read that 'file' and not  /opt/google/chrome/chrome.
(version N+1)

[agl]

LGTM

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

http://breakpad.appspot.com/228001/diff/1/2#newcode1
Line 1: // Copyright (c) 2010, Google Inc.
(Typically I don't think we bump copyright dates, but it doesn't matter either
way.)

http://breakpad.appspot.com/228001/diff/1/2#newcode62
Line 62: static const char* kDeletedSuffix = " (deleted)";
Should use kDeletedSuffix[] to save the relocation record.

http://breakpad.appspot.com/228001/diff/1/2#newcode243
Line 243: mapping->name[filename_len - my_strlen(kDeletedSuffix)] = '\0';
If you switch to using [] with kDeleteSuffix, above, then this can be
"filename_len - sizeof(kDeletedSuffix) - 1"

http://breakpad.appspot.com/228001/diff/1/2#newcode503
Line 503: bool LinuxDumper::HandleDeletedFileInMapping(char* path) {
Needs a comment block explaining the reasoning behind the logic here.

http://breakpad.appspot.com/228001/diff/1/2#newcode504
Line 504: static const size_t kDeletedSuffixLen = my_strlen(kDeletedSuffix);
I think this needs to be sizeof(kDeletedSuffix) - 1 otherwise GCC will emit a
threadsafe wrapping around this. (Well, not in Chromium code because we pass the
command line flags to switch it off, but we shouldn't depend on that for correct
operation.)

[Lei Zhang (chromium)]

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

http://breakpad.appspot.com/228001/diff/1/2#newcode243
Line 243: mapping->name[filename_len - my_strlen(kDeletedSuffix)] = '\0';
On 2010/11/11 14:39:58, agl wrote:
> If you switch to using [] with kDeleteSuffix, above, then this can be
> "filename_len - sizeof(kDeletedSuffix) - 1"

Done. filename_len - sizeof(kDeletedSuffix) + 1, rather.

http://breakpad.appspot.com/228001/diff/1/2#newcode503
Line 503: bool LinuxDumper::HandleDeletedFileInMapping(char* path) {
On 2010/11/11 14:39:58, agl wrote:
> Needs a comment block explaining the reasoning behind the logic here.

There's a small paragraph in the header file. I added more comments inside the
function as well.

http://breakpad.appspot.com/228001/diff/1/2#newcode504
Line 504: static const size_t kDeletedSuffixLen = my_strlen(kDeletedSuffix);
On 2010/11/11 14:39:58, agl wrote:
> I think this needs to be sizeof(kDeletedSuffix) - 1 otherwise GCC will emit a
> threadsafe wrapping around this. (Well, not in Chromium code because we pass
the
> command line flags to switch it off, but we shouldn't depend on that for
correct
> operation.)

Done.

[agl]

LGTM