Allow Linux dumper to work on PTRACE-hardened kernels

[ted.mie...@gmail.com]

Reviewers: mochalatte,

Description:
This patch is from Chris Coulson <chris....@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.

Please review this at http://breakpad.appspot.com/166001

Affected files:
M src/client/linux/handler/exception_handler.cc
M src/client/linux/handler/exception_handler.h

[nea...@gmail.com]

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.

http://breakpad.appspot.com/166001

[nea...@gmail.com]

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

http://breakpad.appspot.com/166001

[Neal Sidhwaney]

I think the owner has to update the patches in the code review issue, so the updated one would have to go through Ted.

Markus - do you have any input?

Thanks,

Neal

On Tue, Aug 24, 2010 at 6:13 PM, <chris....@canonical.com> wrote:

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...

http://breakpad.appspot.com/166001

[nea...@gmail.com]

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/eintr_wrapper.h&q=handle_eintr%20package:http://google-breakpad%5C.googlecode%5C.com&sa=N&cd=1&ct=rc

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

http://breakpad.appspot.com/166001