Decouple Trap from ErrorCode

sandbox/linux/seccomp-bpf/trap.h

Index: sandbox/linux/seccomp-bpf/trap.h
diff --git a/sandbox/linux/seccomp-bpf/trap.h b/sandbox/linux/seccomp-bpf/trap.h
index adc6d7f5e22f4e1cad6b0aecee6aa11e80012fa3..b4e7c31e3f66db4271e2ac852cbdc65dd9fa17c5 100644
--- a/sandbox/linux/seccomp-bpf/trap.h
+++ b/sandbox/linux/seccomp-bpf/trap.h
@@ -9,14 +9,19 @@
 #include <stdint.h>
 
 #include <map>
-#include <vector>
 
-#include "base/basictypes.h"
+#include "base/macros.h"
 #include "sandbox/sandbox_export.h"
 
 namespace sandbox {
 
-class ErrorCode;
+// This must match the kernel's seccomp_data structure.
+struct arch_seccomp_data {
+  int nr;
+  uint32_t arch;
+  uint64_t instruction_pointer;
+  uint64_t args[6];
+};
 
 // The Trap class allows a BPF filter program to branch out to user space by
 // raising a SIGSYS signal.
@@ -47,7 +52,7 @@ class SANDBOX_EXPORT Trap {
   // as needed.
   // N.B.: This makes a permanent state change. Traps cannot be unregistered,
   //   as that would break existing BPF filters that are still active.
-  static ErrorCode MakeTrap(TrapFnc fnc, const void* aux, bool safe);
+  static uint16_t MakeTrap(TrapFnc fnc, const void* aux, bool safe);
 
   // Enables support for unsafe traps in the SIGSYS signal handler. This is a
   // one-way fuse. It works in conjunction with the BPF compiler emitting code
@@ -59,11 +64,13 @@ class SANDBOX_EXPORT Trap {
   // Returns "true", if unsafe traps were turned on.
   static bool EnableUnsafeTrapsInSigSysHandler();
 
-  // Returns the ErrorCode associate with a particular trap id.
-  static ErrorCode ErrorCodeFromTrapId(uint16_t id);
+  // Returns true if a safe trap handler is associated with a
+  // particular trap ID.
+  static bool IsSafeTrapId(uint16_t id);
 
  private:
   struct TrapKey {
+    TrapKey() : fnc(NULL), aux(NULL), safe(false) {}
     TrapKey(TrapFnc f, const void* a, bool s) : fnc(f), aux(a), safe(s) {}
     TrapFnc fnc;
     const void* aux;
@@ -94,7 +101,7 @@ class SANDBOX_EXPORT Trap {
   // dumps.
   void SigSys(int nr, siginfo_t* info, void* void_context)
       __attribute__((noinline));
-  ErrorCode MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe);
+  uint16_t MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe);
   bool SandboxDebuggingAllowedByUser() const;
 
   // We have a global singleton that handles all of our SIGSYS traps. This
@@ -103,11 +110,11 @@ class SANDBOX_EXPORT Trap {
   // events.
   static Trap* global_trap_;
 
-  TrapIds trap_ids_;            // Maps from TrapKeys to numeric ids
-  ErrorCode* trap_array_;       // Array of ErrorCodes indexed by ids
-  size_t trap_array_size_;      // Currently used size of array
-  size_t trap_array_capacity_;  // Currently allocated capacity of array
-  bool has_unsafe_traps_;       // Whether unsafe traps have been enabled
+  TrapIds trap_ids_;                 // Maps from TrapKeys to numeric ids
+  TrapKey* volatile trap_array_;     // Array of TrapKeys indexed by ids
+  size_t volatile trap_array_size_;  // Currently used size of array

[leecam, 2014/09/16 12:36:44]

why are these volatile?

[mdempsky, 2014/09/16 18:48:19]

Reverted.

My rationale was roughly that async signal handlers are only allowed to access
global variables by storing to volatile sig_atomic_t values. These variables are
modified by normal code while the SIGSYS handler is unblocked, so theoretically
they need some stricter access patterns than normal variables.

But since we currently require(?) single-threaded execution and SIGSYS is
actually only triggered synchronously (by calling a signal handler), the async
concerns don't quite apply.

I've reverted these changes for now in favor of a simple "TODO" in trap.cc, and
I'll update the issue with some longer term thoughts on how to fix it.

+  size_t trap_array_capacity_;       // Currently allocated capacity of array
+  bool has_unsafe_traps_;            // Whether unsafe traps have been enabled
 
   // Copying and assigning is unimplemented. It doesn't make sense for a
   // singleton.