Detect corrupt symbol files during minidump processing. Recover from the errors and use the good d…

Detect corrupt symbol files during minidump processing.  Recover from the errors
and use the good data if possible.

More specifically:
 - Detect corrupt symbols during minidump processing and provide the list of
modules with corrupt symbols in the ProcessState.  This will allow listing the
corrupt symbol files in the final crash report.
 - Skip and recover from symbol data parse errors - don't give up until 100
parse errors are seen.
 - In order to recover from '\0' (null terminator) in the middle of a symbol
file, a couple of methods have to be updated to require both buffer pointer and
length.  Previously they required only a buffer pointer (char *) and the size of
the buffer was evaluated using strlen which is not reliable when the data is
corrupt.  Most of the changes are due to these signature updates.
 - Added and updated unittests.

Also, updated minidump_stackwalk to show a WARNING for corrupt symbols.  Output
looks like this:
...
Loaded modules:
0x000da000 - 0x000dafff  Google Chrome Canary  ???  (main)
0x000e0000 - 0x0417dfff  Google Chrome Framework  0.1500.0.3  (WARNING: Corrupt
symbols, Google Chrome Framework, 4682A6B4136436C4BFECEB62D498020E0)
0x044a8000 - 0x04571fff  IOBluetooth  0.1.0.0
...
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1200

[Ivan Penkov]

.

[Mark Mentovai]

Good idea.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
File src/processor/basic_source_line_resolver.cc (right):

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:85: uint64_t last_null_terminator =
memory_buffer_size - 1;
Seems like last_null_terminator, memory_buffer_size, and i (line 96) are really
size_t, not uint64_t.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:100: BPLOG_IF(ERROR, num_errors <=
kMaxErrorsPrinted)
num_errors++; BPLOG_IF(ERROR, num_errors <= kMaxErrorsPrinted)

repeated a ton of times. This suggests that you really want to be calling

NEW_THING(&num_errors) << "blah";

or something else involving a refactoring so that it doesn’t duplicate itself so
much.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:115: << "ParseFile on buffer failed
at :" << line_number;
Get rid of the colon here and in the other messages in this file. Replace them
with the word “line.” People aren’t necessarily going to know what “failed at 7”
means (byte 7? 7 o'clock?) but “line” is unambiguous.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:177: is_corrupt_ = (num_errors > 0);
(parens unnecessary)

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
File src/processor/basic_source_line_resolver_types.h (right):

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver_types.h:82: // Does NOT have ownership
of memory_buffer.
Clarify here that it may modify the buffer, otherwise someone might assume that
it makes its modifications on its own copy of the buffer.

https://breakpad.appspot.com/613002/diff/1001/src/processor/simple_serializer...
File src/processor/simple_serializer-inl.h (right):

https://breakpad.appspot.com/613002/diff/1001/src/processor/simple_serializer...
src/processor/simple_serializer-inl.h:65: *value = ((*source) == 0 ? false :
true);
Parentheses unnecesssary.

[Ivan Penkov]

Done.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
File src/processor/basic_source_line_resolver.cc (right):

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:85: uint64_t last_null_terminator =
memory_buffer_size - 1;
On 2013/07/03 14:24:23, Mark Mentovai wrote:
> Seems like last_null_terminator, memory_buffer_size, and i (line 96) are
really
> size_t, not uint64_t.

Good catch. Done.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:100: BPLOG_IF(ERROR, num_errors <=
kMaxErrorsPrinted)
On 2013/07/03 14:24:23, Mark Mentovai wrote:
> num_errors++; BPLOG_IF(ERROR, num_errors <= kMaxErrorsPrinted)
> 
> repeated a ton of times. This suggests that you really want to be calling
> 
> NEW_THING(&num_errors) << "blah";
> 
> or something else involving a refactoring so that it doesn’t duplicate itself
so
> much.

Done.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:115: << "ParseFile on buffer failed
at :" << line_number;
On 2013/07/03 14:24:23, Mark Mentovai wrote:
> Get rid of the colon here and in the other messages in this file. Replace them
> with the word “line.” People aren’t necessarily going to know what “failed at
7”
> means (byte 7? 7 o'clock?) but “line”&nbsp;is unambiguous.

Done.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:177: is_corrupt_ = (num_errors > 0);
On 2013/07/03 14:24:23, Mark Mentovai wrote:
> (parens unnecessary)

Done.

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
File src/processor/basic_source_line_resolver_types.h (right):

https://breakpad.appspot.com/613002/diff/1001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver_types.h:82: // Does NOT have ownership
of memory_buffer.
On 2013/07/03 14:24:23, Mark Mentovai wrote:
> Clarify here that it may modify the buffer, otherwise someone might assume
that
> it makes its modifications on its own copy of the buffer.

Done.

[Mark Mentovai]

LGTM

Seems sound from my perspective, but this touches lots of things, so maybe Ted
or someone else wants to weigh in too. Your call if you want to get someone
else’s opinion.

[Ivan Penkov]

On 2013/07/04 01:32:47, Mark Mentovai wrote:
> LGTM
> 
> Seems sound from my perspective, but this touches lots of things, so maybe Ted
> or someone else wants to weigh in too. Your call if you want to get someone
> else’s opinion.

Sure, I can wait.  I was hoping to land this by the end of next week.