Breakpad Linux: Fix flag value delimiter for ChromeOS

Breakpad Linux: Fix flag value delimiter for ChromeOS

Fix command line flag passing to crash_reporter to be of the form
"--key=value" instead of "--key value", as crash_reporter now uses
base::CommandLine to do parsing, which doesn't support the latter
format.

BUG=422094
Committed: https://crrev.com/06147cb9165bf57aa6e0a6ad526866342dfb5c58
Cr-Commit-Position: refs/heads/master@{#299429}

[stevefung, 2014-10-09 22:48:48]

stevefung@google.com changed reviewers:
+ stevefung@google.com, thestig@chromium.org, vapier@chromium.org

[Lei Zhang, 2014-10-09 23:02:31]

Why did the crash_reporter behavior change?

You can't use base::StringPrintf(). The comment on line 560 applies here.

[stevefung, 2014-10-10 06:44:45]

On 2014/10/09 23:02:31, Lei Zhang wrote:
> Why did the crash_reporter behavior change?
> 
> You can't use base::StringPrintf(). The comment on line 560 applies here.

The crash_reporter behavior changed, because as part of our minimization effort,
we're removing gflags from chromeos and switching to base::CommandLine instead. 
But base::CommandLine's parsing does not work with "--key value" pairs, only
with "--key=value" delimiters.

[Lei Zhang, 2014-10-10 19:11:47]

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
components/crash/app/breakpad_linux.cc:976: my_strlcat(chrome_flag, "--chrome=",
9);
I think strlcat takes the size of the dest buffer, not the source buffer.

[vapier, 2014-10-13 19:50:51]

i might be missing something obvious, but why don't we update the libchromeos
flags wrapper to handle "--foo bar" ?

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
components/crash/app/breakpad_linux.cc:975: char* chrome_flag =
reinterpret_cast<char*>(allocator->Alloc(buf_len + 10));
this is C++ code, so why not just use std::string and not deal with all the
allocation yourself ?  this is not a critical fast path.

[stevefung, 2014-10-13 20:15:14]

The libchromeos flags wrapper doesn't do any actual parsing of command line
flags itself.  It leaves that up to base::CommandLine, and accesses the flags
through its API.  base::CommandLine however doesn't handle "--foo bar", and I
don't believe they'd be willing to add support for it, since being able to parse
"--foo bar" as foo=bar does take prior knowledge about foo's type (i.e. that its
not a boolean flag), and they've already told us they did not want to add flag
definition/declaration to their API.  We could add our own flag parsing on top
of it in the wrapper, but I did not think that would be a good idea, as it adds
yet another divergent flag parsing path, as well as increased binary size.

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
components/crash/app/breakpad_linux.cc:975: char* chrome_flag =
reinterpret_cast<char*>(allocator->Alloc(buf_len + 10));
On 2014/10/13 19:50:50, vapier wrote:
> this is C++ code, so why not just use std::string and not deal with all the
> allocation yourself ?  this is not a critical fast path.

I believe we can't use it, because this code runs in a compromised context. 
See: https://codereview.chromium.org/633833002/

[stevefung, 2014-10-13 22:28:40]

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/70001/components/crash/app/bre...
components/crash/app/breakpad_linux.cc:976: my_strlcat(chrome_flag, "--chrome=",
9);
On 2014/10/10 19:11:47, Lei Zhang wrote:
> I think strlcat takes the size of the dest buffer, not the source buffer.

Done.

[vapier, 2014-10-13 22:37:55]

https://codereview.chromium.org/649443002/diff/160001/components/crash/app/br...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/160001/components/crash/app/br...
components/crash/app/breakpad_linux.cc:974: size_t buf_len = my_strlen(dumpfile)
+ 10;
alternative:

 const char kChromeFlag[] = "--chrome=";
 size_t buf_len = my_strlen(dumpfile) + sizeof(kChromeFlag);
 char* chrome_flag = reinterpret_cast<char*>(allocator->Alloc(buf_len));
 chrome_flag[0] = '\0';
 my_strlcat(chrome_flag, kChromeFlag, buf_len);
 my_strlcat(chrome_flag, dumpfile, buf_len);

this way you don't have to hardcode the size constants yourself

[stevefung, 2014-10-13 23:51:55]

https://codereview.chromium.org/649443002/diff/160001/components/crash/app/br...
File components/crash/app/breakpad_linux.cc (right):

https://codereview.chromium.org/649443002/diff/160001/components/crash/app/br...
components/crash/app/breakpad_linux.cc:974: size_t buf_len = my_strlen(dumpfile)
+ 10;
On 2014/10/13 22:37:55, vapier wrote:
> alternative:
> 
>  const char kChromeFlag[] = "--chrome=";
>  size_t buf_len = my_strlen(dumpfile) + sizeof(kChromeFlag);
>  char* chrome_flag = reinterpret_cast<char*>(allocator->Alloc(buf_len));
>  chrome_flag[0] = '\0';
>  my_strlcat(chrome_flag, kChromeFlag, buf_len);
>  my_strlcat(chrome_flag, dumpfile, buf_len);
> 
> this way you don't have to hardcode the size constants yourself

Done.

[Lei Zhang, 2014-10-14 00:10:15]

lgtm

[vapier, 2014-10-14 00:22:25]

lgtm

[stevefung, 2014-10-14 03:43:24]

The CQ bit was checked by stevefung@google.com

[commit-bot: I haz the power, 2014-10-14 03:44:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/649443002/220001

[commit-bot: I haz the power, 2014-10-14 04:22:50]

Committed patchset #4 (id:220001)

[commit-bot: I haz the power, 2014-10-14 04:23:33]

Patchset 4 (id:??) landed as
https://crrev.com/06147cb9165bf57aa6e0a6ad526866342dfb5c58
Cr-Commit-Position: refs/heads/master@{#299429}