NonSFI NaCl: Plumb Exception IRT enough for breakpad.

components/nacl/loader/nonsfi/irt_exception_handling.cc

Index: components/nacl/loader/nonsfi/irt_exception_handling.cc
diff --git a/components/nacl/loader/nonsfi/irt_exception_handling.cc b/components/nacl/loader/nonsfi/irt_exception_handling.cc
new file mode 100644
index 0000000000000000000000000000000000000000..707cd9180eaa4364861fdc6ba6548b77f352599e
--- /dev/null
+++ b/components/nacl/loader/nonsfi/irt_exception_handling.cc
@@ -0,0 +1,108 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#include <errno.h>

[Mark Seaborn, 2014/04/23 03:49:50]

Nit: add empty line between boilerplate and first #include

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+#include <signal.h>
+
+#include <map>

[Mark Seaborn, 2014/04/23 03:49:50]

Not used?

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+
+#include "components/nacl/loader/nonsfi/irt_interfaces.h"
+#include "native_client/src/include/nacl_macros.h"
+#include "native_client/src/trusted/service_runtime/nacl_exception.h"
+#include "native_client/src/trusted/service_runtime/nacl_signal.h"
+
+namespace nacl {
+namespace nonsfi {
+namespace {
+
+// This is NonSFI version of exception handling codebase, NaCl side of
+// things resides in:
+// native_client/src/trusted/service_runtime/linux/nacl_signal.c
+// native_client/src/trusted/service_runtime/sys_exception.c
+
+// TODO(uekawa): The list of signals to be handled might need updating.

[Mark Seaborn, 2014/04/23 03:49:50]

It should be OK (so you can remove this TODO).  Any hypothetical omissions we
can find by improving the test coverage later.

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+// NonSFI NaCl does not use NACL_THREAD_SUSPEND_SIGNAL (==SIGUSR1) (??check??)

[Mark Seaborn, 2014/04/23 03:49:50]

Correct.  That's used for thread suspension, currently used for NaCl's GDB debug
stub, which we don't use in Non-SFI NaCl.

[Junichi Uekawa, 2014/04/23 07:56:15]

I've re-wrote the comment with the context. 

+// and SIGSYS is reserved for seccomp-bpf.
+static const int kSignals[] = {
+  SIGSTKFLT,
+  SIGINT, SIGQUIT, SIGILL, SIGTRAP, SIGBUS, SIGFPE, SIGSEGV,
+  // Handle SIGABRT in case someone sends it asynchronously using kill().
+  SIGABRT
+};
+
+NaClExceptionHandler signal_handler_function_pointer = NULL;
+
+// Signal handler, responsible for calling the registered handlers.
+static void SignalCatch(int sig, siginfo_t *info, void *uc) {

[Mark Seaborn, 2014/04/23 03:49:50]

Nit: use Chromium-style "* " spacing

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+  if (signal_handler_function_pointer) {
+    // TODO(uekawa): Whether to add dependency or copy the implementation?

[Mark Seaborn, 2014/04/23 03:49:50]

Adding the dependency isn't ideal but it's OK for now.  Eventually we'll want to
move the IRT interface implementations to the NaCl repo (where they will be more
easily testable).  Then maybe we can refactor this so that it's not depending on
part of service_runtime.

[Junichi Uekawa, 2014/04/23 07:56:15]

I've pondered of not adding a dependency; it introduces yet another list of
registers for all architectures, so I would prefer doing a refactor after we are
inside native_client repo.

https://codereview.chromium.org/248463005/

+    NaClSignalContext signal_context;
+    NaClSignalContextFromHandler(&signal_context, uc);
+    // Is this safe to allocate this on stack ?

[Mark Seaborn, 2014/04/23 03:49:50]

Yes

[Junichi Uekawa, 2014/04/23 07:56:15]

thanks

+    NaClExceptionFrame exception_frame;
+    NaClSignalSetUpExceptionFrame(&exception_frame,
+                                  &signal_context,
+                                  0 /* context_user_addr, what is this? */);

[Mark Seaborn, 2014/04/23 03:49:50]

This parameter doesn't apply if we're calling user code's handler function from
C.

[Junichi Uekawa, 2014/04/23 07:56:15]

I think we don't need this for nonsfi NaCl, but probably won't hurt to leave it
as it is either.

+
+    signal_handler_function_pointer(&exception_frame.context);
+  }
+  // TODO(uekawa): Only exit on crash signals?

[Mark Seaborn, 2014/04/23 03:49:50]

All the signals you're handling are crash signals

[Junichi Uekawa, 2014/04/23 07:56:15]

Removed the comment.

+  _exit(-1);
+}
+
+static int IrtExceptionHandler(NaClExceptionHandler handler,
+                               NaClExceptionHandler *old_handler) {
+  // TODO(uekawa): Do I need to have a mutex lock?

[Mark Seaborn, 2014/04/23 03:49:50]

You could use AtomicExchange from base/atomicops.h

[Junichi Uekawa, 2014/04/23 07:56:15]

after staring at AtomicExchange, I don't think this is worth it (unless we are
running on 386 which doesn't have atomic word writes).

[Mark Seaborn, 2014/04/24 23:39:27]

But setting the new handler and getting the old one might be expected to be
atomic overall, so you should either use a mutex (as your earlier comment
suggested) or use AtomicExchange.

[Junichi Uekawa, 2014/04/25 01:06:09]

Done.

+  if (old_handler)
+    *old_handler = signal_handler_function_pointer;
+  signal_handler_function_pointer = handler;
+  return 0;
+}
+
+static int IrtExceptionStack(void *stack, size_t size) {
+  // TODO(uekawa): I think we shouldn't implement this until we really
+  // need it.  IrtThreadCreate allocates sigaltstack already, and
+  // there is no legitimate reason I can think of that you would want
+  // to reallocate an altstack. Note that
+  // native_client/src/tests/exception_test/exception_crash_test.c
+  // wants to use this but we are not running that test yet.
+  return -EINVAL;

[Mark Seaborn, 2014/04/23 03:49:50]

This could return 0, since it's mostly-OK as a no-op.  That would prevent crash
reporter libraries from failing on startup if this returns an error.

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+}
+
+static int IrtExceptionClearFlag(void) {
+  // TODO(uekawa): I think we shouldn't implement this until we really
+  // need it.
+  return -EINVAL;

[Mark Seaborn, 2014/04/23 03:49:50]

ENOSYS would be more appropriate

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+}
+
+}  // namespace
+
+const struct nacl_irt_exception_handling kIrtExceptionHandling = {
+  IrtExceptionHandler,
+  IrtExceptionStack,
+  IrtExceptionClearFlag,
+};
+
+void InitializeSignalHandler(void) {

[Mark Seaborn, 2014/04/23 03:49:50]

Nit: Should be "()" rather than "(void)" in C++.  (There's only a difference in
C.)

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+  struct sigaction sa;
+  unsigned int a;
+
+  memset(&sa, 0, sizeof(sa));
+  sigemptyset(&sa.sa_mask);
+  sa.sa_sigaction = SignalCatch;
+  sa.sa_flags = SA_ONSTACK | SA_SIGINFO;
+
+  // Mask all signals we catch to prevent re-entry.
+  for (a = 0; a < NACL_ARRAY_SIZE(kSignals); a++) {
+    sigaddset(&sa.sa_mask, kSignals[a]);
+  }
+
+  // Install all handlers.
+  for (a = 0; a < NACL_ARRAY_SIZE(kSignals); a++) {
+    if (sigaction(kSignals[a], &sa, NULL) != 0)
+      perror("sigaction");

[Mark Seaborn, 2014/04/23 03:49:50]

This should be a fatal error, otherwise this would be ignored too easily

[Junichi Uekawa, 2014/04/23 07:56:15]

Done.

+  }
+}
+
+}  // namespace nonsfi
+}  // namespace nacl