Workaround incorrect signal handler save/restore

Workaround incorrect signal handler save/restore

If a library saves the breakpad signal handler and then restores it with
signal instead of sigaction, the arguments to the breakpad signal
handler may be invalid. To work around this, the signal handler can
detect that it was installed incorrectly, reinstall itself with the
correct flags, and then return. On return, the signal will be
retriggered and the signal handler will then be called with valid
arguments.

BUG=

[cjhopman]

Mark-

Could you take a look and let me know what you think about this. This is an
issue that we ran into on Android, where a library was improperly
saving/restoring the breakpad signal handler.

As mentioned in my comment, I couldn't actually come up with a simple test case
that would trigger the failure, and the one I included here is only to show the
expected failure case (it might be best to not actually include that test, or
maybe mark it disabled/flaky).

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
File client/linux/handler/exception_handler_unittest.cc (right):

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
client/linux/handler/exception_handler_unittest.cc:384:
TEST(ExceptionHandlerTest, WorkaroundSignalSaveRestore) {
This is how I would expect to test this fix, however it is not this easy to
reproduce the failure. It seems that when ExceptionHandler::SignalHandler is
first called in this test case, even though SA_SIGINFO is not set, the arguments
are valid. I could not produce a simple test case that would pass invalid
arguments to the signal handler. Whether or not the arguments get set or
overwritten depends on the signal dispatching code. The main point is that if
SA_SIGINFO is not set, then it is not guaranteed that the arguments to the
signal handler are valid.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
File client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
client/linux/handler/exception_handler.cc:301: static const char
no_handler_msg[] =
static is not necessary or helpful here and on line 305.

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
client/linux/handler/exception_handler.cc:303: logger::write(no_handler_msg,
sizeof(no_handler_msg));
You use sizeof here but strlen on line 307. Shouldn’t this be strlen or sizeof -
1?

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
client/linux/handler/exception_handler.cc:313: // Mask all exception signals
when we're handling one of them.
Who are “we?” Don’t say “we” in comments.

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
File client/linux/handler/exception_handler_unittest.cc (right):

https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
client/linux/handler/exception_handler_unittest.cc:384:
TEST(ExceptionHandlerTest, WorkaroundSignalSaveRestore) {
I suppose it’s enough to just test that the handler routine is ultimately called
while SA_SIGINFO is set, which I guess this test does do. Although if you were
seeing valid arguments to the handler routine even without SA_SIGINFO set, I
wonder if the theory about the cause of the bug you’re seeing is correct.
Certainly this change is valid either way. I haven’t looked at the trampolines
or anything in the kernel recently, so I don’t know for sure either.

[cjhopman]

digit@ has a slightly cleaner patch for this at
http://breakpad.appspot.com/481002/ and so I'm closing this one.

On 2012/10/09 14:25:52, Mark Mentovai wrote:
> LGTM
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> File client/linux/handler/exception_handler.cc (right):
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> client/linux/handler/exception_handler.cc:301: static const char
> no_handler_msg[] =
> static is not necessary or helpful here and on line 305.
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> client/linux/handler/exception_handler.cc:303: logger::write(no_handler_msg,
> sizeof(no_handler_msg));
> You use sizeof here but strlen on line 307. Shouldn’t this be strlen or sizeof
-
> 1?
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> client/linux/handler/exception_handler.cc:313: // Mask all exception signals
> when we're handling one of them.
> Who are “we?” Don’t say “we” in comments.
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> File client/linux/handler/exception_handler_unittest.cc (right):
> 
>
https://breakpad.appspot.com/478005/diff/1/client/linux/handler/exception_han...
> client/linux/handler/exception_handler_unittest.cc:384:
> TEST(ExceptionHandlerTest, WorkaroundSignalSaveRestore) {
> I suppose it’s enough to just test that the handler routine is ultimately
called
> while SA_SIGINFO is set, which I guess this test does do. Although if you were
> seeing valid arguments to the handler routine even without SA_SIGINFO set, I
> wonder if the theory about the cause of the bug you’re seeing is correct.
> Certainly this change is valid either way. I haven’t looked at the trampolines
> or anything in the kernel recently, so I don’t know for sure either.