Allow Linux dumper to work on PTRACE-hardened kernels

This patch is from Chris Coulson <chris.coulson@canonical.com>. While working on
enabling crash reporting for Ubuntu's Firefox builds, he found that Ubuntu's
PTRACE-hardened kernel was preventing Linux dumping from functioning, since it
relies on clone()ing a child process and then dumping the parent from there.
This patch uses prctl to indicate that the child is permitted to dump the
parent.

See
https://wiki.ubuntu.com/SecurityTeam/Roadmap/KernelHardening#PTRACE%20Protection
for details about the kernel modifications.

[mochalatte]

http://breakpad.appspot.com/166001/diff/1/2
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/166001/diff/1/2#newcode356
Line 356: static const char str = 'a';
please name this better - how about okToContinueMessage or something indicative
of what the message means?

http://breakpad.appspot.com/166001/diff/1/2#newcode372
Line 372: // until after we've called prctl
i understand the comment, but it seems like it's intended to communicate with
gdb.  also adding an explanation of why we need to do this (e.g. like you
mentioned to me, some kernels will disable ptracing unless this is done) will be
helpful to future contributors.

http://breakpad.appspot.com/166001/diff/1/2#newcode381
Line 381: }
how about creating the pipe in the constructor to minimize what's done at
post-crash time?

http://breakpad.appspot.com/166001/diff/1/2#newcode391
Line 391: } while (r == -1 && errno == EINTR);
Can you abstract both this and the child process code behind two methods named
something like "NotifyChildItsOkToContinue and WaitForOkToContinueNotification"
?  I'm not requesting those names in particular as much as a separate function
for each that indicates what's going on.

[evan]

+markus FYI

[Chris Coulson]

Thanks for the review.

On 2010/08/24 20:38:13, mochalatte wrote:
> http://breakpad.appspot.com/166001/diff/1/2
> File src/client/linux/handler/exception_handler.cc (right):
> 
> http://breakpad.appspot.com/166001/diff/1/2#newcode356
> Line 356: static const char str = 'a';
> please name this better - how about okToContinueMessage or something
indicative
> of what the message means?
> 

Agreed, I've renamed this now.

> http://breakpad.appspot.com/166001/diff/1/2#newcode372
> Line 372: // until after we've called prctl
> i understand the comment, but it seems like it's intended to communicate with
> gdb.  also adding an explanation of why we need to do this (e.g. like you
> mentioned to me, some kernels will disable ptracing unless this is done) will
be
> helpful to future contributors.
> 

I've updated the comment now so that it makes more sense and explains the change
better

> http://breakpad.appspot.com/166001/diff/1/2#newcode381
> Line 381: }
> how about creating the pipe in the constructor to minimize what's done at
> post-crash time?
> 

I wasn't sure whether to do this or not. The reason I chose not to do it in the
constructor was to avoid wasting a couple of FD's. If that's not a problem, then
I'm happy to move it to the constructor instead.

> http://breakpad.appspot.com/166001/diff/1/2#newcode391
> Line 391: } while (r == -1 && errno == EINTR);
> Can you abstract both this and the child process code behind two methods named
> something like "NotifyChildItsOkToContinue and
WaitForOkToContinueNotification"
> ?  I'm not requesting those names in particular as much as a separate function
> for each that indicates what's going on.

Makes sense, I've done that now as well (and also fixed a bug in the read() call
which made it fail with EFAULT when data appeared in the pipe)

Now, I'm not sure whether I can attach the updated patch...

[mochalatte]

http://breakpad.appspot.com/166001/diff/1/3
File src/client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/166001/diff/1/3#newcode228
Line 228: };
missed this file - please add comment to member variable

[Chris Coulson]

On 2010/08/25 15:25:46, mochalatte wrote:
> http://breakpad.appspot.com/166001/diff/1/3
> File src/client/linux/handler/exception_handler.h (right):
> 
> http://breakpad.appspot.com/166001/diff/1/3#newcode228
> Line 228: };
> missed this file - please add comment to member variable

Hi,

Ted already uploaded the updated patch. Do you want to comment on that one too
before I update it to add the extra comment?

Thanks
Chris

[mochalatte]

just some nits...otherwise LGTM..thanks for the patch!

sorry to keep harassing you on some details..this stuff just ends up being such
a pain to debug when it fails, so having extra logging & reusing our existing
macros helps a great deal.

http://breakpad.appspot.com/166001/diff/5001/6001
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/166001/diff/5001/6001#newcode375
Line 375: static const char no_pipe_msg[] = "ExceptionHandler::GenerateDump
sys_pipe failed:";
80 col limit

http://breakpad.appspot.com/166001/diff/5001/6001#newcode415
Line 415: do {
sorry i should have mentioned this in my last review, but we have a macro to do
system call restarts:

http://www.google.com/codesearch/p?hl=en#I3UcDWKuRYs/trunk/src/common/linux/e...

http://breakpad.appspot.com/166001/diff/5001/6001#newcode417
Line 417: } while (r == -1 && errno == EINTR);
can you add logging indicating if errno != EINTR ? that way in situations where
a dump was failed to be generated we can at least see that it was because this
write failed for some random reason

http://breakpad.appspot.com/166001/diff/5001/6001#newcode427
Line 427: } while (r == -1 && errno == EINTR);
same log comment