[wasm] Initial signal handler

src/trap-handler/signal-handler.cc

Index: src/trap-handler/signal-handler.cc
diff --git a/src/trap-handler/signal-handler.cc b/src/trap-handler/signal-handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..01f3ab1790e86954b273c658f7d721d66c4464f3
--- /dev/null
+++ b/src/trap-handler/signal-handler.cc
@@ -0,0 +1,197 @@
+// 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
+// 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.
+
+// 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 {
+
+extern THREAD_LOCAL bool g_thread_in_wasm_code;

[Mark Seaborn, 2017/01/24 07:14:39]

Can you put these extern declarations in a header, please?  Presumably
trap-handler-internal.h.  Then the compiler will check they match the
definitions.

[Eric Holk, 2017/01/26 01:33:36]

Done.

+
+extern size_t gNumCodeObjects;
+extern CodeProtectionInfo **gCodeObjects;

[Mark Seaborn, 2017/01/24 07:14:39]

Nit: Presumably this should follow the Chromium style and use "* " spacing?

[Eric Holk, 2017/01/26 01:33:36]

Done.

I was fighting with clangfmt because it was trying to infer what I should use
from the rest of the file. V8 clangfmt is set to always use the Chromium style
spacing now.

+
+MetadataLock::MetadataLock() {
+  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) {
+    pthread_sigmask(SIG_UNBLOCK, &sigs, &old_mask_);

[Mark Seaborn, 2017/01/24 07:14:39]

Can you add a TODO to consider using linux-syscall-support for calling this
syscall, please?

[Eric Holk, 2017/01/26 01:33:36]

Done.

+  }
+
+  ~SigUnmaskStack() { pthread_sigmask(SIG_SETMASK, &old_mask_, nullptr); }
+
+ private:
+  sigset_t old_mask_;
+
+  SigUnmaskStack(const SigUnmaskStack &) = delete;
+  void operator=(const SigUnmaskStack &) = delete;
+};
+#endif
+}  // namespace
+
+bool TryHandleFault(void *source_instruction, void *target_data,
+                    void **return_address) {
+#if V8_TRAP_HANDLER_SUPPORTED
+  // 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 in case we fault in the signal hander.

[Mark Seaborn, 2017/01/24 07:14:39]

This could be more explicit.  How about: "This means 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."

[Eric Holk, 2017/01/26 01:33:36]

Done.

+  sigset_t sigs;
+  sigemptyset(&sigs);

[Mark Seaborn, 2017/01/24 07:14:39]

Happily, sigemptyset() and sigaddset() are declared as async-signal-safe by
POSIX.  That might be worth noting in a comment.

[Eric Holk, 2017/01/26 01:33:37]

Done.

+  sigaddset(&sigs, SIGSEGV);
+  sigaddset(&sigs, SIGBUS);
+  SigUnmaskStack unmask(sigs);
+
+  intptr_t fault_addr = reinterpret_cast<intptr_t>(source_instruction);
+
+  TRACE("Handling fault at %p\n", reinterpret_cast<void *>(fault_addr));
+
+  // TODO(eholk): broad code range check
+
+  MetadataLock lock_holder;
+
+  for (size_t i = 0; i < gNumCodeObjects; ++i) {
+    const CodeProtectionInfo *data = gCodeObjects[i];
+    if (data == nullptr) {
+      continue;
+    }
+    const intptr_t base = reinterpret_cast<intptr_t>(data->base);

[Mark Seaborn, 2017/01/24 07:14:39]

Can you use uintptr_t for this, rather than intptr_t?

Firstly, that avoids having to worry about any undefined behaviour from C signed
overflow.  (I generally prefer unsigned types over signed types for that
reason.)

Secondly, "fault_addr >= base" on signed addresses could do the wrong thing if
used on 32-bit systems, in rare cases. :-)

[Eric Holk, 2017/01/26 01:33:36]

You're right, there's no reason for these to be signed.

Done.

+
+    TRACE("  Checking range base=%p, length = %zu\n",
+          reinterpret_cast<void *>(base), data->size);
+
+    if (fault_addr >= base &&
+        fault_addr < base + static_cast<intptr_t>(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. Jump to
+          // the landing pad.
+
+          *return_address = reinterpret_cast<void *>(
+              data->instructions[i].landing_offset + base);
+          return true;
+        }
+      }
+    }
+  }
+
+  // If we get here, it's not a wasm fault, so we go to the next handler.

[Mark Seaborn, 2017/01/24 07:14:39]

To be more precise, "If we get here, it's not a recoverable wasm fault".

If we get here, it *should* be a fault in wasm-generated code, unless
g_thread_in_wasm_code was set wrongly.

Shouldn't we restore g_thread_in_wasm_code to true here for completeness?

[Eric Holk, 2017/01/26 01:33:36]

Done.

+  return false;
+#else
+  abort();
+#endif
+}
+
+namespace {
+
+#if V8_TRAP_HANDLER_SUPPORTED
+void HandleSignal(int signum, siginfo_t *info, void *context) {
+  // Bail out early in case we got called for the wrong kind of signal.
+  if (signum != SIGSEGV && signum != SIGBUS) {

[Mark Seaborn, 2017/01/24 07:14:39]

RegisterDefaultSignalHandler() only registers for SIGSEGV.

[Eric Holk, 2017/01/26 01:33:36]

I changed it so that RegisterDefaultSignalHandler registers for SIGBUS now too.

Done.

[Mark Seaborn, 2017/01/27 20:17:30]

I'd recommend only doing SIGSEGV, because I don't think SIGBUS is needed for
Wasm on x86-64 Linux, is it?  It's simpler to keep the set smaller until we know
we need to change it.

[Mark Mentovai, 2017/01/27 20:34:09]

Mark Seaborn wrote:
I have seen signal mappings move with kernel versions, and although I can offer
no directly applicable evidence, I feel like the difference between SIGSEGV and
SIGBUS can be tickled by things like which instruction tried to access memory,
whether the access was aligned, whether alignment checks were enabled, etc. It’s
certainly more portable to lump SIGSEGV and SIGBUS into one category, although I
concede that we’re only concerned with Linux x86_64 here at the moment. But
still, I’m of the mind that this should consider both, although I wouldn’t force
the issue if otherMark disagrees.

[Mark Seaborn, 2017/01/27 20:50:51]

Those are the kind of things that it would be good to be strict about.  e.g. If
SIGBUS were generated for an unaligned access, then only handle it once we have
test coverage for that case.

However, I can live with handling both if this has changed between Linux kernel
versions.

I just didn't want the reason for having SIGBUS here be "Mac OS X generates
SIGBUS for the Wasm case", because we're pretty sure we don't want to use POSIX
signal handling for this on Mac.

[Eric Holk, 2017/02/02 18:43:30]

I will go with handling just SIGSEGV until we have a specific and concrete
reason to do otherwise. It's easy to fix if we find a case where different
kernel versions are behaving differently.

+    return;
+  }
+
+  // Make sure the signal was generated by the kernel and not some other source.
+  if (info->si_code <= 0 || info->si_code == SI_USER ||

[Mark Seaborn, 2017/01/24 07:14:39]

I think these checks should be done by TryHandleFault(), since that's meant to
be the API that the embedder (Chromium) uses.

TryHandleFault() should take the 'siginfo_t*' and 'void* context', so that the
embedder doesn't have to unpack things from those data structures to pass to
TryHandleFault().

That will be a POSIX-specific interface, but that's OK.  This is all
POSIX-specific.

[Eric Holk, 2017/01/26 01:33:36]

Doing it this way is the result of some changes Jochen requested, so there's
probably a bigger design question here. Because TryHandleFault() is expected to
be called by the host-supplied signal handler, it is part of the V8 public API.
As such, we need it to be platform independent.

I decided to break out the pieces of information we need now or will need in the
near future (i.e. before we turn this feature on by default), since there should
be some way to get this on any platform we plan to support.

I'll follow up with Jochen to see what precedent there is in V8 for having
platform specificity in the API.

[Mark Seaborn, 2017/01/27 20:17:30]

In the interests of doing things incrementally, you can leave it as-is for now. 
We can revisit this when it comes to hooking this up to crash reporting in
Chrome.

[Eric Holk, 2017/02/02 18:43:30]

Jochen suggested that he was okay with having #ifdef'd APIs to do this on a
per-platform basis, so I renamed the public API function TryHandleSignal and
changed it back so that it takes the Linux signal handler parameters.

+      info->si_code == SI_QUEUE || info->si_code == SI_TIMER ||
+      info->si_code == SI_ASYNCIO || info->si_code == SI_MESGQ) {
+    return;
+  }
+
+  ucontext_t *uc = reinterpret_cast<ucontext_t *>(context);
+
+  void *source_instruction =
+      reinterpret_cast<void *>(uc->uc_mcontext.gregs[REG_RIP]);
+  void *target_data = reinterpret_cast<void *>(info->si_addr);

[Mark Seaborn, 2017/01/24 07:14:39]

I notice this isn't used yet, so you could remove it.

[Eric Holk, 2017/01/26 01:33:36]

Assuming the signature for TryHandleFault stay as is, it seems like we might as
well keep it here. I've included it as a parameter to TryHandleFault because
it's needed for one of the planned checks and I want to avoid making a breaking
change to the API in the future.

[Mark Seaborn, 2017/01/27 20:17:30]

OK, though I'd prefer to remove this variable if it's not used, if you want to
leave it in, can you add a TODO to make use of it?

[Eric Holk, 2017/02/02 18:43:30]

Since this has moved back into TryHandleSignal, I will remove it.

+
+  void *return_address;
+
+  if (!TryHandleFault(source_instruction, target_data, &return_address)) {
+    abort();
+  }
+
+  uc->uc_mcontext.gregs[REG_RIP] = reinterpret_cast<intptr_t>(return_address);
+}
+#endif
+}  // namespace
+
+bool RegisterDefaultSignalHandler() {
+#if V8_TRAP_HANDLER_SUPPORTED
+  struct sigaction action;
+  action.sa_sigaction = HandleSignal;
+  action.sa_flags = SA_SIGINFO;
+  sigemptyset(&action.sa_mask);
+  return sigaction(SIGSEGV, &action, nullptr) == 0;
+#else
+  return false;
+#endif
+}
+
+}  // namespace trap_handler
+}  // namespace internal
+}  // namespace v8