Make the Linux CrashGenerationClient an interface.

Make the Linux CrashGenerationClient an interface.

Also allow it to be set on the ExceptionHandler. This will allow Chromium's
implementation to be properly treated as an out-of-process handler.

BUG=https://code.google.com/p/chromium/issues/detail?id=349600
R=mark@chromium.org

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

[Mark Mentovai]

LG otherwise

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:49:
CrashGenerationClientImpl(int server_fd) : server_fd_(server_fd) {}
explicit

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:57: struct
kernel_msghdr msg;
Don’t declare it ’til you use it.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:57: struct
kernel_msghdr msg;
= { NULL }; to 0-initialize the whole thing. Or, if you don’t like that…

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:58: my_memset(&msg,
0, sizeof(struct kernel_msghdr));
…at least use sizeof(variable) instead of sizeof(type) here.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:59: struct
kernel_iovec iov[1];
Why bother with the [1] at all?

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:66: my_memset(cmsg,
0, kControlMsgSize);
Same preference for losing the memset here, if you initialize cmsg[] to "".

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:84:
IGNORE_RET(HANDLE_EINTR(sys_read(fds[0], &b, 1)));
Is anyone ever gonna close fds[0]?

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:99: if (server_fd
<= 0)
< 0, not <= 0.

Technically line 79 should be < 0 too, although the <= is harmless there.

[rsesek]

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:49:
CrashGenerationClientImpl(int server_fd) : server_fd_(server_fd) {}
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> explicit

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:57: struct
kernel_msghdr msg;
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> Don’t declare it ’til you use it.

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:57: struct
kernel_msghdr msg;
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> = { NULL }; to 0-initialize the whole thing. Or, if you don’t like that…

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:58: my_memset(&msg,
0, sizeof(struct kernel_msghdr));
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> …at least use sizeof(variable) instead of sizeof(type) here.

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:59: struct
kernel_iovec iov[1];
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> Why bother with the [1] at all?

In the Chromium code from which this was copied, the iovec is more than one
item.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:66: my_memset(cmsg,
0, kControlMsgSize);
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> Same preference for losing the memset here, if you initialize cmsg[] to "".

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:84:
IGNORE_RET(HANDLE_EINTR(sys_read(fds[0], &b, 1)));
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> Is anyone ever gonna close fds[0]?

I don't know. I think we're supposed to crash after this.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:99: if (server_fd
<= 0)
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> < 0, not <= 0.
> 
> Technically line 79 should be < 0 too, although the <= is harmless there.

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:99: if (server_fd
<= 0)
On 2014/05/05 19:28:41, Mark Mentovai wrote:
> < 0, not <= 0.
> 
> Technically line 79 should be < 0 too, although the <= is harmless there.

Done.

[Mark Mentovai]

You might want to test socketpair’s return value and return false there too if
it fails. LGTM otherwise.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:84:
IGNORE_RET(HANDLE_EINTR(sys_read(fds[0], &b, 1)));
rsesek wrote:
> On 2014/05/05 19:28:41, Mark Mentovai wrote:
> > Is anyone ever gonna close fds[0]?
> 
> I don't know. I think we're supposed to crash after this.

Let’s close it, since it’s not clear that there will be an immediate exit, and
if there isn’t, it’s a leak.

https://breakpad.appspot.com/2664002/diff/50001/src/client/linux/crash_genera...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/50001/src/client/linux/crash_genera...
src/client/linux/crash_generation/crash_generation_client.cc:63: char
cmsg[kControlMsgSize] = { 0 };
Some compiler may get upset that you’ve written { 0 } instead of "" or { '\0' }.
I don’t think any compiler in its right mind actually should get upset at valid
code like that, but I’m pretty sure I’ve seen it happen.

[rsesek]

Good call on socketpair(), too.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:84:
IGNORE_RET(HANDLE_EINTR(sys_read(fds[0], &b, 1)));
On 2014/05/05 20:22:57, Mark Mentovai wrote:
> rsesek wrote:
> > On 2014/05/05 19:28:41, Mark Mentovai wrote:
> > > Is anyone ever gonna close fds[0]?
> > 
> > I don't know. I think we're supposed to crash after this.
> 
> Let’s close it, since it’s not clear that there will be an immediate exit, and
> if there isn’t, it’s a leak.

Done.

https://breakpad.appspot.com/2664002/diff/1/src/client/linux/crash_generation...
src/client/linux/crash_generation/crash_generation_client.cc:84:
IGNORE_RET(HANDLE_EINTR(sys_read(fds[0], &b, 1)));
On 2014/05/05 20:22:57, Mark Mentovai wrote:
> rsesek wrote:
> > On 2014/05/05 19:28:41, Mark Mentovai wrote:
> > > Is anyone ever gonna close fds[0]?
> > 
> > I don't know. I think we're supposed to crash after this.
> 
> Let’s close it, since it’s not clear that there will be an immediate exit, and
> if there isn’t, it’s a leak.

Done.

https://breakpad.appspot.com/2664002/diff/50001/src/client/linux/crash_genera...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/50001/src/client/linux/crash_genera...
src/client/linux/crash_generation/crash_generation_client.cc:63: char
cmsg[kControlMsgSize] = { 0 };
On 2014/05/05 20:22:57, Mark Mentovai wrote:
> Some compiler may get upset that you’ve written { 0 } instead of "" or { '\0'
}.
> I don’t think any compiler in its right mind actually should get upset at
valid
> code like that, but I’m pretty sure I’ve seen it happen.

Done.

[Mark Mentovai]

LGTM. Another potential hole avoided.

[rsesek]

Committed patchset #3 manually as r1324 (presubmit successful).

[mdempsky]

https://breakpad.appspot.com/2664002/diff/80002/src/client/linux/crash_genera...
File src/client/linux/crash_generation/crash_generation_client.cc (right):

https://breakpad.appspot.com/2664002/diff/80002/src/client/linux/crash_genera...
src/client/linux/crash_generation/crash_generation_client.cc:78: return false;
This predates your CL, but this return path leaks fds[0].