Properly redeliver (or don't) signals to the previous handlers.

Properly redeliver (or don't) signals to the previous handlers.

If none of the installed ExceptionHandlers handle a signal (their
FilterCallbacks or HandlerCallbacks all return false), then the signal
should be delivered to the signal handlers that were previously
installed.

This requires that old_handlers_ become a static vector so that we can
restore the handlers in the static HandleSignal.

Currently it is also restoring signals in ~ExceptionHandler (if there
are no others). This should not be required since our documentation
states that a process can only have one ExceptionHandler for which
install_handlers is true (and so we get the correct behavior if we
simply leave our handlers installed forever), but even the tests
themselves violate that.

BUG=

[Mark Mentovai]

Is anyone actually using stacked handlers? They tend to be sort of broken. If
you don’t actually need the feature, I’d just as soon not invest any energy in
fixing it up.

If you do actually need the feature, we can investigate this. In that case, I’d
like digit to take a first look for review.

[cjhopman]

On 2012/08/23 18:44:18, Mark Mentovai wrote:
> Is anyone actually using stacked handlers? They tend to be sort of broken. If
> you don’t actually need the feature, I’d just as soon not invest any energy in
> fixing it up.
> 
> If you do actually need the feature, we can investigate this. In that case,
I’d
> like digit to take a first look for review.

The Android system installs a handler to drop a stacktrace and information to
the log. On Chrome for Android we would like to be able to have more control
over whether we generate a minidump or that Android tombstone (or possibly even
both), but without this CL we cannot do that because breakpad doesn't restore
the old handler as expected (as claimed in exception_handler.h).

[Mark Mentovai]

That’s reasonable. David, you’re up first.

[digit]

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:158: if (handler_stack_->empty()) {
Since the code below is modifying global variables, I think it'd be better if
this was implemented in a static method instead.

I.e. can you keep UninstallHandlers()? or UninstallHandlersLocked()?

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:192: old_handlers_ = new
std::vector<struct sigaction*>;
I think there are several issues with this code:

1/ You changed old_handlers_ from a per-object field to a global variable. That
looks fine to me, but this means you'll have to add locking any time you want to
act on it to avoid race conditions.

2/ The line above actually overwrites a global variable each time a new
ExceptionHandler is created with install_handler=true. It's a memory leak if
more than one EH object is created.

I'd thus recommend moving this code to a static method that would only create
allocate old_handlers_ lazily.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:230: // If the signal was successfully
handled, restore the default signal handler.
I'd prefer if this comment would keep the subtle details explained on line 258+
before your patch, i.e. the fact that the signal is unmasked / re-triggered as
function exit.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:239: if
(sigaction(kExceptionSignals[i], old, NULL) == -1) {
I'm uncomfortable with the fact that you're indexing kExceptionSignals[], a
global constant array, with an index which depends on old_handlers_->size(),
which could be anything.

Granted, the implementation currently ensures that this is correct, but I think
this is yet another indication that a simpler implementation that uses a pointer
to fixed array of struct sigaction items would be preferable.

If not, an assert should be used, at a minimum.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:242: // delete old;
I assumed you commented this because one cannot use 'delete' in this function
(signal handler context + potentially corrupted environment). It's probably a
good idea to add a comment to explain that.

Technically, it's a memory leak, but the process is going to crash anyway so I
wouldn't worry too much about it.

This can also be avoided with a struct sigaction array :)

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.h:227: static std::vector<struct
sigaction*>* old_handlers_;
Since you're simplifying this code (which is a good thing), can I ask you to
avoid the needless use of a std::vector of pointers to structs here. A simple
pointer to a fixed-size array of 'struct sigaction' should be largely enough to
implement the exact same feature.

Or even an global array of 'struct sigaction', since I don't think it would
create a static C++ initializer (it's a POD-struct type). However, this requires
bringing the size of the array into the class declaration, which may or may not
be desirable.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:225: // SIGKILL cannot be
blocked and we do not install a handler for it.
I believe Mark now prefers avoiding the use of "we" in comments. Can you adjust
this (and all the others you touch)?

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:240: static void
CrashWithCallbacks(ExceptionHandler::FilterCallback filter,
ExceptionHandler::MinidumpCallback done, string path) {
can you fix this to not use more than 80 columns?

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:306:
ASSERT_NO_FATAL_FAILURE(WaitForProcessToTerminate(child, SIGSEGV));
It'd be nice if you could add a unit test that stacks two ExceptionHandlers at
once, to verify that the feature works correctly after your patch (I understand
it may not work correctly right now though).

[cjhopman]

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:158: if (handler_stack_->empty()) {
On 2012/08/24 08:34:56, digit wrote:
> Since the code below is modifying global variables, I think it'd be better if
> this was implemented in a static method instead.
> 
> I.e. can you keep UninstallHandlers()? or UninstallHandlersLocked()?

Done.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:192: old_handlers_ = new
std::vector<struct sigaction*>;
On 2012/08/24 08:34:56, digit wrote:
> I think there are several issues with this code:
> 
> 1/ You changed old_handlers_ from a per-object field to a global variable.
That
> looks fine to me, but this means you'll have to add locking any time you want
to
> act on it to avoid race conditions.
> 
> 2/ The line above actually overwrites a global variable each time a new
> ExceptionHandler is created with install_handler=true. It's a memory leak if
> more than one EH object is created.
> 
> I'd thus recommend moving this code to a static method that would only create
> allocate old_handlers_ lazily.

1/ Note that currently interaction with old_handlers is guarded by the same
mutex as used for handler_stack_ (i.e. handler_stack_mutex_)... this could be
changed to use its own (or the mutex could be renamed to clarify its use).

2/ The header states that a process isn't allowed to create two handlers with
install_handler=true, but even the tests violate that requirement. The way that
install_handler works (both with and without this CL) seems kind of broken
also-- that is, it applies to all ExceptionHandlers not just the one created
with install_handler=true (and some other small things).

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:230: // If the signal was successfully
handled, restore the default signal handler.
On 2012/08/24 08:34:56, digit wrote:
> I'd prefer if this comment would keep the subtle details explained on line
258+
> before your patch, i.e. the fact that the signal is unmasked / re-triggered as
> function exit.

How's this?

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:239: if
(sigaction(kExceptionSignals[i], old, NULL) == -1) {
On 2012/08/24 08:34:56, digit wrote:
> I'm uncomfortable with the fact that you're indexing kExceptionSignals[], a
> global constant array, with an index which depends on old_handlers_->size(),
> which could be anything.
> 
> Granted, the implementation currently ensures that this is correct, but I
think
> this is yet another indication that a simpler implementation that uses a
pointer
> to fixed array of struct sigaction items would be preferable.
> 
> If not, an assert should be used, at a minimum.

This is moved to line 227.
Now, kExceptionSignals and old_handlers are both global arrays of size
kNumSignalsHandled, and each of the loops uses that for its bound.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.cc:242: // delete old;
On 2012/08/24 08:34:56, digit wrote:
> I assumed you commented this because one cannot use 'delete' in this function
> (signal handler context + potentially corrupted environment). It's probably a
> good idea to add a comment to explain that.
> 
> Technically, it's a memory leak, but the process is going to crash anyway so I
> wouldn't worry too much about it.
> 
> This can also be avoided with a struct sigaction array :)

Done.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler.h:227: static std::vector<struct
sigaction*>* old_handlers_;
On 2012/08/24 08:34:56, digit wrote:
> Since you're simplifying this code (which is a good thing), can I ask you to
> avoid the needless use of a std::vector of pointers to structs here. A simple
> pointer to a fixed-size array of 'struct sigaction' should be largely enough
to
> implement the exact same feature.
> 
> Or even an global array of 'struct sigaction', since I don't think it would
> create a static C++ initializer (it's a POD-struct type). However, this
requires
> bringing the size of the array into the class declaration, which may or may
not
> be desirable.

This is now a global array of 'struct sigaction' in the source file. This seems
like the best way to handle it and keep it in sync with the kExceptionSignals
array.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:225: // SIGKILL cannot be
blocked and we do not install a handler for it.
On 2012/08/24 08:34:56, digit wrote:
> I believe Mark now prefers avoiding the use of "we" in comments. Can you
adjust
> this (and all the others you touch)?

I think I got all such uses.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:240: static void
CrashWithCallbacks(ExceptionHandler::FilterCallback filter,
ExceptionHandler::MinidumpCallback done, string path) {
On 2012/08/24 08:34:56, digit wrote:
> can you fix this to not use more than 80 columns?

Done.

http://breakpad.appspot.com/440002/diff/1/client/linux/handler/exception_hand...
client/linux/handler/exception_handler_unittest.cc:306:
ASSERT_NO_FATAL_FAILURE(WaitForProcessToTerminate(child, SIGSEGV));
On 2012/08/24 08:34:56, digit wrote:
> It'd be nice if you could add a unit test that stacks two ExceptionHandlers at
> once, to verify that the feature works correctly after your patch (I
understand
> it may not work correctly right now though).

Done. I've added several to test that behavior.

[digit]

Patch set 2 seems very nice. Thank you. lgtm, but not an owner so it's up to
Mark now.

[Mark Mentovai]

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:115: const int kNumHandledSignals = 5;
Don’t do this, it’s a maintenance problem. Instead, make the compiler calculate
things for you:

const int kExceptionSignals[kNumHandledSignals] = {
  SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS
};

and then use arraysize(kExceptionSignals). If arraysize is unavailable, use
sizeof(kExceptionSignals) / sizeof(kExceptionSignals[0]).

If you want a named constant, you can write

const int kNumHandledSignals = arraysize(kExceptionSignals);

Yes, this is a compile-time constant. It’s OK to do this. It doesn’t result in a
variable-length array or any static initialization code even when you later
write

struct sigaction old_handlers[kNumHandledSignals] = {0};

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:125: // We use this value rather than
SIGSTKSZ because we would end up overrunning
I saw David ask you not to use “we” in comments, so…don’t.

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:231: // SIGKILL cannot be
blocked and a handler cannot be installed for it..
Extra dot.

[cjhopman]

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:115: const int kNumHandledSignals = 5;
On 2012/08/27 21:29:11, Mark Mentovai wrote:
> Don’t do this, it’s a maintenance problem. Instead, make the compiler
calculate
> things for you:
> 
> const int kExceptionSignals[kNumHandledSignals] = {
>   SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS
> };
> 
> and then use arraysize(kExceptionSignals). If arraysize is unavailable, use
> sizeof(kExceptionSignals) / sizeof(kExceptionSignals[0]).
> 
> If you want a named constant, you can write
> 
> const int kNumHandledSignals = arraysize(kExceptionSignals);
> 
> Yes, this is a compile-time constant. It’s OK to do this. It doesn’t result in
a
> variable-length array or any static initialization code even when you later
> write
> 
> struct sigaction old_handlers[kNumHandledSignals] = {0};

Done.

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:125: // We use this value rather than
SIGSTKSZ because we would end up overrunning
On 2012/08/27 21:29:11, Mark Mentovai wrote:
> I saw David ask you not to use “we” in comments, so…don’t.

Ah, sorry about that, this code was just moved from elsewhere and so I missed
this comment.

Done.

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/440002/diff/7002/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:231: // SIGKILL cannot be
blocked and a handler cannot be installed for it..
On 2012/08/27 21:29:11, Mark Mentovai wrote:
> Extra dot.

Done.

[Mark Mentovai]

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:128: static const unsigned
kSigStackSize = 8192;
I recommend std::max(8192, SIGSTKSZ) here.

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:188: RestoreHandlersLocked();
Do you want to free the alternate stack memory here?

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:228: for (int i = 0; i <
kNumHandledSignals; ++i) {
Shouldn’t you avoid doing anything at all if !handlers_installed?

[cjhopman]

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
File client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:128: static const unsigned
kSigStackSize = 8192;
On 2012/08/28 13:06:53, Mark Mentovai wrote:
> I recommend std::max(8192, SIGSTKSZ) here.

Done.

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:188: RestoreHandlersLocked();
On 2012/08/28 13:06:53, Mark Mentovai wrote:
> Do you want to free the alternate stack memory here?

I'm not sure what the best approach for handling the stack memory is. There are
a couple details that I'm not sure how to address.

1. The current altstack at this point could be different than the one that we
set. In this case, what is the proper course? Should we still free the memory
that we had set (even though we know somebody else was mucking around with the
altstack)?

2. Not restoring the altstack to its default state is safer than not restoring
the signal handlers. Specifically, a signal handler could be installed between
destruction of the last ExceptionHandler and construction of a new one (as
happens in the tests) or a signal could be sent in that same timeframe. Without
restoring signal handlers we actually handle that second case fine, but not the
first (Note: I wouldn't say that we really support any such uses, just that they
work). The altstack doesn't really have the same issue.

3. Should we then be saving/restoring the previous state of the altstack?

Given those thoughts, and the fact that the handling of the altstack probably
needs to be reworked due to
http://code.google.com/p/google-breakpad/issues/detail?id=374&q=sigaltstack&s...,
I thought it best to not change the behavior. If you'd prefer me to change it
(probably to save/restore) just let me know.

http://breakpad.appspot.com/440002/diff/7006/client/linux/handler/exception_h...
client/linux/handler/exception_handler.cc:228: for (int i = 0; i <
kNumHandledSignals; ++i) {
On 2012/08/28 13:06:53, Mark Mentovai wrote:
> Shouldn’t you avoid doing anything at all if !handlers_installed?

Done.

[Mark Mentovai]

cjhopman wrote:
> 1. The current altstack at this point could be different than the one that we
> set. In this case, what is the proper course? Should we still free the memory
> that we had set (even though we know somebody else was mucking around with the
> altstack)?

Yes, I think you should always free your altstack, but only restore the old
altstack if your altstack is still set as the current altstack.

> 2. Not restoring the altstack to its default state is safer than not restoring
> the signal handlers. Specifically, a signal handler could be installed between
> destruction of the last ExceptionHandler and construction of a new one (as
> happens in the tests) or a signal could be sent in that same timeframe.
Without
> restoring signal handlers we actually handle that second case fine, but not
the
> first (Note: I wouldn't say that we really support any such uses, just that
they
> work). The altstack doesn't really have the same issue.

Right, it has a different issue: it leaks altstack memory.

> 3. Should we then be saving/restoring the previous state of the altstack?

Given (1), usually yes, but you can check to make sure nobody else has messed
with it.

[cjhopman]

On 2012/08/28 18:11:09, Mark Mentovai wrote:
> cjhopman wrote:
> > 1. The current altstack at this point could be different than the one that
we
> > set. In this case, what is the proper course? Should we still free the
memory
> > that we had set (even though we know somebody else was mucking around with
the
> > altstack)?
> 
> Yes, I think you should always free your altstack, but only restore the old
> altstack if your altstack is still set as the current altstack.
> 
> > 2. Not restoring the altstack to its default state is safer than not
restoring
> > the signal handlers. Specifically, a signal handler could be installed
between
> > destruction of the last ExceptionHandler and construction of a new one (as
> > happens in the tests) or a signal could be sent in that same timeframe.
> Without
> > restoring signal handlers we actually handle that second case fine, but not
> the
> > first (Note: I wouldn't say that we really support any such uses, just that
> they
> > work). The altstack doesn't really have the same issue.
> 
> Right, it has a different issue: it leaks altstack memory.
> 
> > 3. Should we then be saving/restoring the previous state of the altstack?
> 
> Given (1), usually yes, but you can check to make sure nobody else has messed
> with it.

Ok, I've added saving/restoring of the alternate stack. I'm not convinced this
is the best way to handle the alternate stack. A problem is that if you have two
libraries that save/restore the alternate stack in this way and they don't do
their restores in the reverse order of their saves, the second restore will
restore an altstack that has already been freed (and in fact there is no way for
the second one to know this).

[Mark Mentovai]

LGTM