Add an assertion to check that Chrome/Breakpad does not install a signal handler

Add an assertion to check that Chrome/Breakpad does not install a signal handler

Breakpad's signal handler is typically not designed to handle faults
that occur in NaCl sandboxed code, especially on x86-32 Linux where
the handler must restore %gs in order to be safe.

Breakpad's signal handler is being turned off for NaCl in a
Chrome-side change, http://codereview.chromium.org/6677168/.

This change is intended to prevent Chrome from accidentally
regressing.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=1607
TEST=build Chrome with this using "-D target_arch=ia32 -D linux_breakpad=1";
  run with CHROME_HEADLESS=1 --enable-nacl --enable-crash-reporter

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=5004

[Cliff L. Biffle, 2011-04-06 20:50:10]

LGTM modulo the comment in the Windows file, below.

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
File src/native_client/src/trusted/service_runtime/win/nacl_signal.c (right):

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
src/native_client/src/trusted/service_runtime/win/nacl_signal.c:137: * Ideally
we would assert that no exception handlers have been
Is this correct?  Does VEH have this problem and require this assertion?

[bsy, 2011-04-06 20:59:44]

a nit.  o/w lgtm modulo cbiffle's windows comment.

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
File src/native_client/src/trusted/service_runtime/posix/nacl_signal.c (right):

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
src/native_client/src/trusted/service_runtime/posix/nacl_signal.c:312: "A signal
handler is registered for signal %i.  "
%d is much more common.  please use that equivalent format specifier.

[noelallen_use_chromium, 2011-04-06 23:34:58]

Please make sure we have a bug to track both the interaction of VEH and
UnhandledExceptionFilters as well as Chrome breakpad for Windows.   Otherwise
LGTM.

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
File src/native_client/src/trusted/service_runtime/win/nacl_signal.c (right):

http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
src/native_client/src/trusted/service_runtime/win/nacl_signal.c:137: * Ideally
we would assert that no exception handlers have been
You probably need too look at both SetVectoredExceptionHandler and
SetUnhandledExceptionFilter.

I'm not sure that you can determine if something has been installed in either
case.

[noelallen_use_chromium, 2011-04-08 19:56:05]

Actually, since you haven't checked this in yet...

It's true that Windows does not have a direct mechanism for determining if there
is a handler installed, but you can determine if there is a handler consuming
the signal.  Instead of installing the handler at the head, you can install it
at the tail.  If your handler gets called, you still don't know if another one
got called before it, but you can at least determine if a previously installed
handler is consuming the signal.

[Mark Seaborn, 2011-04-21 15:36:15]

On 6 April 2011 13:59, <bsy@google.com> wrote:

>
>
http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
> src/native_client/src/trusted/service_runtime/posix/nacl_signal.c:312:
>
> "A signal handler is registered for signal %i.  "
> %d is much more common.  please use that equivalent format specifier.
>

OK, changed to use %d.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2011-04-21 15:38:27]

On 6 April 2011 13:50, <cbiffle@google.com> wrote:

> LGTM modulo the comment in the Windows file, below.
>
>
>
>
http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
> File src/native_client/src/trusted/service_runtime/win/nacl_signal.c
> (right):
>
>
>
http://codereview.chromium.org/6798008/diff/1/src/native_client/src/trusted/s...
> src/native_client/src/trusted/service_runtime/win/nacl_signal.c:137: *
>
> Ideally we would assert that no exception handlers have been
> Is this correct?  Does VEH have this problem and require this assertion?


I think we have since established that Windows is a lost cause in this
regard. :-)  See http://code.google.com/p/nativeclient/issues/detail?id=1633.
I have amended the comment.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2011-04-21 15:41:11]

On 6 April 2011 16:34, <noelallen@google.com> wrote:

> Please make sure we have a bug to track both the interaction of VEH and
> UnhandledExceptionFilters as well as Chrome breakpad for Windows.
> Otherwise
> LGTM.
>

Yes, we have this bug to track NaCl's interaction with VEH and all that
stuff:
http://code.google.com/p/nativeclient/issues/detail?id=1633
I am confident that we will not forget about this issue. :-)

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[bsy, 2011-04-22 17:15:44]

lgtm