[wasm] Initial signal handler

src/trap-handler/signal-handler.cc

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// PLEASE READ BEFORE CHANGING THIS FILE!
+//
+// This file implements the out of bounds signal handler for
+// WebAssembly. Signal handlers are notoriously difficult to get

[Mark Seaborn, 2017/02/09 16:44:36]

Linking to the design doc (the Google Doc) could be useful.  Does V8 have a
place for collecting design docs?

[Eric Holk, 2017/02/15 02:02:45]

I don't know if we have an official place, but I made the doc public from my
Chromium account and added a link.

Done.

+// right, and getting it wrong can lead to security
+// vulnerabilities. In order to minimize this risk, here are some
+// rules to follow.
+//
+// 1. Do not introduce any new external dependencies. This file needs
+//    to be self contained so it is easy to audit everything that a
+//    signal handler might do.
+//
+// 2. Any changes must be reviewed by someone from the crash reporting
+//    or security team.

[Mark Mentovai, 2017/02/09 17:39:41]

Point to the this directory’s OWNERS file as a reminder, especially if you have
to keep certain things commented out in there. Same in the other .cc.

[Eric Holk, 2017/02/15 02:02:45]

Done.

+
+// This file contains all of the code that actually runs in a signal handler
+// context.
+
+#include <signal.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <atomic>
+
+#include "src/flags.h"
+#include "src/trap-handler/trap-handler-internal.h"
+#include "src/trap-handler/trap-handler.h"
+
+#define TRACE(...)
+
+namespace v8 {
+namespace internal {
+namespace trap_handler {
+
+MetadataLock::MetadataLock() {

[Mark Seaborn, 2017/02/09 16:44:36]

MetadataLock is the only code that runs both inside and outside the trap
handler.  If we use the naming "{inside,outside}-handler.cc", maybe this ought
to go in a separate file -- "shared.cc" perhaps?

[Eric Holk, 2017/02/15 02:02:45]

How does handler-{inside,outside,shared}.cc sound?

I moved the shared variable definitions to handler-shared.cc as well.

[Mark Seaborn, 2017/02/17 21:41:12]

Sounds good.

+  if (g_thread_in_wasm_code) {
+    abort();
+  }
+
+  while (spinlock_.test_and_set(std::memory_order::memory_order_acquire)) {
+  }
+}
+
+MetadataLock::~MetadataLock() {
+  if (g_thread_in_wasm_code) {
+    abort();
+  }
+
+  spinlock_.clear(std::memory_order::memory_order_release);
+}
+
+namespace {
+
+#if V8_TRAP_HANDLER_SUPPORTED
+class SigUnmaskStack {
+ public:
+  explicit SigUnmaskStack(sigset_t sigs) {
+    // TODO(eholk): consider using linux-syscall-support instead of
+    // pthread_sigmask.
+    pthread_sigmask(SIG_UNBLOCK, &sigs, &old_mask_);
+  }
+
+  ~SigUnmaskStack() { pthread_sigmask(SIG_SETMASK, &old_mask_, nullptr); }
+
+ private:
+  sigset_t old_mask_;
+
+  SigUnmaskStack(const SigUnmaskStack&) = delete;
+  void operator=(const SigUnmaskStack&) = delete;
+};
+#endif
+}  // namespace
+
+#if V8_OS_LINUX
+bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context) {
+#if V8_TRAP_HANDLER_SUPPORTED
+  // Bail out early in case we got called for the wrong kind of signal.
+  if (signum != SIGSEGV && signum != SIGBUS) {

[Mark Mentovai, 2017/02/09 17:39:41]

We decided on just SIGSEGV here and on line 112.

[Eric Holk, 2017/02/15 02:02:45]

Done.

+    return false;
+  }
+
+  // Make sure the signal was generated by the kernel and not some other source.
+  if (info->si_code <= 0 || info->si_code == SI_USER ||
+      info->si_code == SI_QUEUE || info->si_code == SI_TIMER ||
+      info->si_code == SI_ASYNCIO || info->si_code == SI_MESGQ) {
+    return false;
+  }
+
+  // Ensure the faulting thread was actually running Wasm code.
+  if (!g_thread_in_wasm_code) {
+    TRACE("signal handler: Thread not running WASM code; crashing\n");
+    return false;
+  }
+
+  // Clear g_thread_in_wasm_code, primarily to protect against nested faults.
+  g_thread_in_wasm_code = false;
+
+  // Unmask the signal so that if this signal handler crashes, the crash will be
+  // handled by the crash reporter.  Otherwise, the process might be killed with
+  // the crash going unreported.
+  sigset_t sigs;
+  // Fortunately, sigemptyset and sigaddset are async-signal-safe according to
+  // the POSIX standard.
+  sigemptyset(&sigs);
+  sigaddset(&sigs, SIGSEGV);
+  sigaddset(&sigs, SIGBUS);
+  SigUnmaskStack unmask(sigs);
+
+  uintptr_t fault_addr = context->uc_mcontext.gregs[REG_RIP];
+
+  TRACE("Handling fault at %p\n", reinterpret_cast<void*>(fault_addr));
+
+  // TODO(eholk): broad code range check
+
+  MetadataLock lock_holder;

[Mark Mentovai, 2017/02/09 17:39:41]

Add a comment here explaining how you know that this lock won’t already be held
by the same thread (g_thread_in_wasm_code). The assertion in MetadataLock’s
constructor is good, but make it more obvious by putting a comment right here,
front and center.

[Eric Holk, 2017/02/15 02:02:45]

Done.

+
+  for (size_t i = 0; i < gNumCodeObjects; ++i) {
+    const CodeProtectionInfo* data = gCodeObjects[i];
+    if (data == nullptr) {
+      continue;
+    }
+    const uintptr_t base = reinterpret_cast<uintptr_t>(data->base);
+
+    TRACE("  Checking range base=%p, length = %zu\n",
+          reinterpret_cast<void*>(base), data->size);
+
+    if (fault_addr >= base && fault_addr < base + data->size) {
+      // Hurray, we found the code object. Check for protected addresses.
+      const ptrdiff_t offset = fault_addr - base;
+
+      TRACE("Range found, offset is %td\n", offset);
+
+      for (unsigned i = 0; i < data->num_protected_instructions; ++i) {
+        TRACE("  Checking instr offset %d, landing offset %d\n",
+              data->instructions[i].instr_offset,
+              data->instructions[i].landing_offset);
+        if (data->instructions[i].instr_offset == offset) {
+          // Hurray again, we found the actual instruction. Tell the caller to
+          // return to the landing pad.
+          context->uc_mcontext.gregs[REG_RIP] =
+              data->instructions[i].landing_offset + base;
+          return true;
+        }
+      }
+    }
+  }
+
+  // If we get here, it's not a recoverable wasm fault, so we go to the next
+  // handler.
+  g_thread_in_wasm_code = true;
+  return false;
+#else
+  abort();

[Mark Mentovai, 2017/02/09 17:39:41]

I don’t see how this #else abort() is ever reachable, but if it is, the next
comment applies here too.

[Eric Holk, 2017/02/15 02:02:45]

It shouldn't ever be reachable. I'll see if I can tighten up the #ifdefs to
eliminate it and if not I will add a comment and change it to NOTREACHED().

+#endif  // V8_TRAP_HANDLER_SUPPORTED
+}
+#endif  // V8_OS_LINUX
+
+namespace {
+
+#if V8_TRAP_HANDLER_SUPPORTED
+void HandleSignal(int signum, siginfo_t* info, void* context) {
+  ucontext_t* uc = reinterpret_cast<ucontext_t*>(context);
+
+  if (!TryHandleSignal(signum, info, uc)) {
+    abort();

[Mark Mentovai, 2017/02/09 17:39:41]

Is this really how you want things to go down, as opposed to preserving the
actual signal and context?

[Eric Holk, 2017/02/15 02:02:45]

We want to preserve the signal and context.

Done.

+  }
+  // TryHandleSignal modifies context to change where we return to.
+}
+#endif
+}  // namespace
+
+bool RegisterDefaultSignalHandler() {
+#if V8_TRAP_HANDLER_SUPPORTED
+  struct sigaction action;
+  struct sigaction old_action;
+  action.sa_sigaction = HandleSignal;
+  action.sa_flags = SA_SIGINFO;
+  sigemptyset(&action.sa_mask);
+  if (sigaction(SIGSEGV, &action, &old_action) != 0) {

[Mark Mentovai, 2017/02/09 17:39:41]

You can give nullptr instead of old_action, since you don’t care.

[Eric Holk, 2017/02/15 02:02:45]

Done.

+    return false;
+  }
+
+  return true;
+#else
+  return false;
+#endif
+}
+
+}  // namespace trap_handler
+}  // namespace internal
+}  // namespace v8