Linux ExceptionHandler: don't allocate the CrashContext on the stack

Linux ExceptionHandler: don't allocate the CrashContext on the stack

On Android the size of the alternate stack can be very small (8k).
Even if breakpad uses sigaltstack to increase the size of the alternate
stack during initialization, that call affects only the main thread.
On Android, the libc's pthread initializer reset the sigaltstack to 8k.
When entering a signal handler, the kernel typically pushes the context
on the alternate stack. On arm64, sizeof(CrashContext) is ~5k, which
leaves 3k of usable stack for breakpad.
On top of that, breakpad allocates another struct CrashContext on the
stack. In the case of Android arm64, then, breakpad ends up using
5k + 5k > 8k of stack, which causes a stack overflow.
This got unnoticed in Android L, as the alternate stack didn't have
red-zones between them, so breakpad was often happily overflowing onto
the next thread's stack. This is not the case anymore [1].
This CL moves the CrashContext into a global variable. It should be
safe as the ExceptionHandlers are serialized on a mutex.

[1] https://android.googlesource.com/platform/bionic/+/595752f623ae88f7e4193a6e531a0805f1c6c4dc

BUG=374
R=mark@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/55ba26e52036f745bdaa51fcf4b83a7ef0935f59

[Primiano Tucci (use gerrit), 2015-09-18 14:25:16]

Patchset #1 (id:1) has been deleted

[Primiano Tucci (use gerrit), 2015-09-18 14:29:52]

primiano@chromium.org changed reviewers:
+ mark@chromium.org, thestig@chromium.org

[Primiano Tucci (use gerrit), 2015-09-18 14:29:52]

Mark/Lei, can I ask you take a look to this CL.
It's very small but I'd like a couple of extra eyes on this.

My favorite sport here, working around android "features".

[Tobias Sargeant, 2015-09-18 15:10:41]

I can confirm that this CL results in producing microdumps correctly when
WebView-containing processes crash on Nexus 9.

[Mark Mentovai, 2015-09-18 22:13:20]

What if two threads take signals at the same time?

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
File src/client/linux/handler/exception_handler.cc (right):

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
src/client/linux/handler/exception_handler.cc:249: // Pre-fault the crash
context struct. This is to avoid failing due to OOM
Smart.

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
src/client/linux/handler/exception_handler.cc:440: memset(&g_crash_context_, 0,
sizeof(g_crash_context_));
Necessary? You already wrote zeroes into it when you created the
ExceptionHandler now.

[Primiano Tucci (use gerrit), 2015-09-18 22:22:41]

> What if two threads take signals at the same time?

Given the mutex |g_handler_stack_mutex| and the comment ("All the exception
signals are blocked at this point")
ExceptionHandler::SignalHandler should never be reentrant, right? If I am
reading the code correctly there should be at most one HandleSignal() call at
any given time, even if more thread crash concurrently.

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
File src/client/linux/handler/exception_handler.cc (right):

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
src/client/linux/handler/exception_handler.cc:440: memset(&g_crash_context_, 0,
sizeof(g_crash_context_));
On 2015/09/18 22:13:20, Mark Mentovai - August is over wrote:
> Necessary? You already wrote zeroes into it when you created the
> ExceptionHandler now.

Right, should be safe from the production code anyways, just not sure about the
Asan/valgrind behavior.
Should we remove this?
To be honest, the siginfo and .context below seem absolutely redundant to me, as
they are part of the context struct.
I'd just remove all three at this point. 

The only question might be: is it ever the case that breakpad can handle more
than one signal?
IIRC it should self-unregister after handling the firrst signal.
If there is a case where we want to handle more than one signal per process, the
memset above should stay.

[Mark Mentovai, 2015-09-18 22:34:56]

I don’t know how I missed the mutex literally one line above the change. I only
looked at the beginning of the function. Oops.

LGTM

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
File src/client/linux/handler/exception_handler.cc (right):

https://codereview.chromium.org/1354923002/diff/20001/src/client/linux/handle...
src/client/linux/handler/exception_handler.cc:440: memset(&g_crash_context_, 0,
sizeof(g_crash_context_));
Primiano Tucci wrote:
> On 2015/09/18 22:13:20, Mark Mentovai - August is over wrote:
> > Necessary? You already wrote zeroes into it when you created the
> > ExceptionHandler now.
> 
> Right, should be safe from the production code anyways, just not sure about
the
> Asan/valgrind behavior.

I think that they shouldn’t care as long as it was touched once.

> Should we remove this?
> To be honest, the siginfo and .context below seem absolutely redundant to me,
as
> they are part of the context struct.
> I'd just remove all three at this point. 
> 
> The only question might be: is it ever the case that breakpad can handle more
> than one signal?
> IIRC it should self-unregister after handling the firrst signal.
> If there is a case where we want to handle more than one signal per process,
the
> memset above should stay.

That’s fair, let’s leave it for safety. I think we normally will handle only
one, but I don’t have the inclination to check whether simulated crashes (that
shouldn’t terminate the process) are also routed through HandleSignal.

[Primiano Tucci (use gerrit), 2015-09-22 08:11:30]

Committed patchset #1 (id:20001) manually as
55ba26e52036f745bdaa51fcf4b83a7ef0935f59 (presubmit successful).