Stricter validation checks for symbol parser functions.

Adding stricter validation checks to various symbol parser functions.

More specifically, the validation of the following record types is improved:
 - FILE records
 - FUNC records
 - Line record
 - PUBLIC records

Adding unittests.
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1217

[Mark Mentovai]

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
File src/google_breakpad/processor/basic_source_line_resolver.h (right):

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
The comment should say which of these are “in” and which are “out.” The normal
way to figure out which are “in,” by looking for constness, doesn’t apply here.
If you could make file_line, function_line, etc. into pointers to const char
data, that would resolve the problem.

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
This leaves me wondering what the memory management strategy for filename is.
Does the callee take ownership of a newly-allocated string? Does it just point
to something in the file_line buffer, so the callee doesn’t take ownership of
anything new, but the lifetime of filename becomes the same as the lifetime of
file_line?

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
Probably something should also say that these modify the input line (which is
why they can’t be const.) That’s non-obvious and frankly kind of sucky for a
general-purpose API, but somewhat sensible given the usage.

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

https://breakpad.appspot.com/632003/diff/2001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:476: if
(!IsValidAfterNumber(after_number) || *index < 0 || *index == LONG_MAX) {
Prefer to #include <limits> and use std::numeric_limits<long>::max(). Similar
comments apply to other use of <limits.h> macros.

[Ivan Penkov]

Done.

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
File src/google_breakpad/processor/basic_source_line_resolver.h (right):

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
On 2013/09/23 16:51:25, Mark Mentovai wrote:
> Probably something should also say that these modify the input line (which is
> why they can’t be const.) That’s non-obvious and frankly kind of sucky for a
> general-purpose API, but somewhat sensible given the usage.

Done.

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
On 2013/09/23 16:51:25, Mark Mentovai wrote:
> The comment should say which of these are “in” and which are “out.” The normal
> way to figure out which are “in,” by looking for constness, doesn’t apply
here.
> If you could make file_line, function_line, etc. into pointers to const char
> data, that would resolve the problem.

Done.

https://breakpad.appspot.com/632003/diff/2001/src/google_breakpad/processor/b...
src/google_breakpad/processor/basic_source_line_resolver.h:88: static bool
ParseFile(char *file_line, int *index, char **filename);
On 2013/09/23 16:51:25, Mark Mentovai wrote:
> This leaves me wondering what the memory management strategy for filename is.
> Does the callee take ownership of a newly-allocated string? Does it just point
> to something in the file_line buffer, so the callee doesn’t take ownership of
> anything new, but the lifetime of filename becomes the same as the lifetime of
> file_line?

Done.

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

https://breakpad.appspot.com/632003/diff/2001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:476: if
(!IsValidAfterNumber(after_number) || *index < 0 || *index == LONG_MAX) {
On 2013/09/23 16:51:25, Mark Mentovai wrote:
> Prefer to #include <limits> and use std::numeric_limits<long>::max(). Similar
> comments apply to other use of <limits.h> macros.

Done.

[Mark Mentovai]

LGTM

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

https://breakpad.appspot.com/632003/diff/7001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:467: file_line += 5;  // skip prefix
You could add an assertion to check that the prefix really is “FILE ”. Same for
FUNC and PUBLIC below.

[Ivan Penkov]

Done.

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

https://breakpad.appspot.com/632003/diff/7001/src/processor/basic_source_line...
src/processor/basic_source_line_resolver.cc:467: file_line += 5;  // skip prefix
On 2013/09/24 21:08:39, Mark Mentovai wrote:
> You could add an assertion to check that the prefix really is “FILE ”. Same
for
> FUNC and PUBLIC below.

Done.