Decouple Trap from ErrorCode

This CL makes a few changes in the direction of removing dependencies
on ErrorCode by pushing them up a layer:

- Trap now simply uses TrapKeys directly to store trap function and
metadata instead of needlessly encoding them as ErrorCode.

- MakeTrap returns a bare trap ID and it's instead the caller's
responsibility to encode in an ErrorCode if needed, which is now
handled by ErrorCode's trap constructor.

- Also, ErrorCodeFromTrapId() is replaced by IsSafeTrapId() to answer
the one question that SandboxBPF was interested in knowing about
existing traps.

- Change a few SandboxBPF trap-constructing functions into static
functions since they don't depend on any SandboxBPF instance member
variables.

BUG=414363

Review URL: https://codereview.chromium.org/572753002

Cr-Commit-Position: refs/heads/master@{#295121}

sandbox/linux/seccomp-bpf/errorcode.cc

@@ -27,12 +27,13 @@ ErrorCode::ErrorCode(int err) {
   }
 }
 
-ErrorCode::ErrorCode(Trap::TrapFnc fnc, const void* aux, bool safe, uint16_t id)
+ErrorCode::ErrorCode(Trap::TrapFnc fnc, const void* aux, bool safe)
     : error_type_(ET_TRAP),
       fnc_(fnc),
       aux_(const_cast<void*>(aux)),
       safe_(safe),
-      err_(SECCOMP_RET_TRAP + id) {}
+      err_(SECCOMP_RET_TRAP + Trap::MakeTrap(fnc, aux, safe)) {
+}
 
 ErrorCode::ErrorCode(int argno,
                      ArgType width,

sandbox/linux/seccomp-bpf/errorcode.h

@@ -155,7 +155,7 @@ class SANDBOX_EXPORT ErrorCode {
   // If we are wrapping a callback, we must assign a unique id. This id is
   // how the kernel tells us which one of our different SECCOMP_RET_TRAP
   // cases has been triggered.
-  ErrorCode(Trap::TrapFnc fnc, const void* aux, bool safe, uint16_t id);
+  ErrorCode(Trap::TrapFnc fnc, const void* aux, bool safe);
 
   // Some system calls require inspection of arguments. This constructor
   // allows us to specify additional constraints.

sandbox/linux/seccomp-bpf/sandbox_bpf.cc

@@ -170,9 +170,7 @@ void CheckForUnsafeErrorCodes(Instruction* insn, void* aux) {
   if (!*is_unsafe) {
     if (BPF_CLASS(insn->code) == BPF_RET && insn->k > SECCOMP_RET_TRAP &&
         insn->k - SECCOMP_RET_TRAP <= SECCOMP_RET_DATA) {
-      const ErrorCode& err =
-          Trap::ErrorCodeFromTrapId(insn->k & SECCOMP_RET_DATA);
-      if (err.error_type() != ErrorCode::ET_INVALID && !err.safe()) {
+      if (!Trap::IsSafeTrapId(insn->k & SECCOMP_RET_DATA)) {
         *is_unsafe = true;
       }
     }
@@ -1049,11 +1047,11 @@ ErrorCode SandboxBPF::Unexpected64bitArgument() {
 }
 
 ErrorCode SandboxBPF::Trap(Trap::TrapFnc fnc, const void* aux) {
-  return Trap::MakeTrap(fnc, aux, true /* Safe Trap */);
+  return ErrorCode(fnc, aux, true /* Safe Trap */);
 }
 
 ErrorCode SandboxBPF::UnsafeTrap(Trap::TrapFnc fnc, const void* aux) {
-  return Trap::MakeTrap(fnc, aux, false /* Unsafe Trap */);
+  return ErrorCode(fnc, aux, false /* Unsafe Trap */);
 }
 
 bool SandboxBPF::IsRequiredForUnsafeTrap(int sysno) {

sandbox/linux/seccomp-bpf/sandbox_bpf.h

@@ -21,24 +21,11 @@
 #include "sandbox/linux/seccomp-bpf/die.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
 #include "sandbox/linux/seccomp-bpf/linux_seccomp.h"
+#include "sandbox/linux/seccomp-bpf/trap.h"
 #include "sandbox/sandbox_export.h"
 
 namespace sandbox {
 
-// 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];
-};
-
-struct arch_sigsys {
-  void* ip;
-  int nr;
-  unsigned int arch;
-};
-
 class CodeGen;
 class SandboxBPFPolicy;
 class SandboxUnittestHelper;
@@ -116,7 +103,7 @@ class SANDBOX_EXPORT SandboxBPF {
   // The "aux" field can carry a pointer to arbitrary data. See EvaluateSyscall
   // for a description of how to pass data from SetSandboxPolicy() to a Trap()
   // handler.
-  ErrorCode Trap(Trap::TrapFnc fnc, const void* aux);
+  static ErrorCode Trap(Trap::TrapFnc fnc, const void* aux);
 
   // Calls a user-space trap handler and disables all sandboxing for system
   // calls made from this trap handler.
@@ -128,7 +115,7 @@ class SANDBOX_EXPORT SandboxBPF {
   //   very useful to diagnose code that is incompatible with the sandbox.
   //   If even a single system call returns "UnsafeTrap", the security of
   //   entire sandbox should be considered compromised.
-  ErrorCode UnsafeTrap(Trap::TrapFnc fnc, const void* aux);
+  static ErrorCode UnsafeTrap(Trap::TrapFnc fnc, const void* aux);
 
   // UnsafeTraps require some syscalls to always be allowed.
   // This helper function returns true for these calls.
@@ -170,7 +157,7 @@ class SANDBOX_EXPORT SandboxBPF {
                  const ErrorCode& failed);
 
   // Kill the program and print an error message.
-  ErrorCode Kill(const char* msg);
+  static ErrorCode Kill(const char* msg);
 
   // This is the main public entry point. It finds all system calls that
   // need rewriting, sets up the resources needed by the sandbox, and
@@ -200,7 +187,7 @@ class SANDBOX_EXPORT SandboxBPF {
   // Returns the fatal ErrorCode that is used to indicate that somebody
   // attempted to pass a 64bit value in a 32bit system call argument.
   // This method is primarily needed for testing purposes.
-  ErrorCode Unexpected64bitArgument();
+  static ErrorCode Unexpected64bitArgument();
 
  private:
   friend class CodeGen;

sandbox/linux/seccomp-bpf/trap.cc

@@ -7,14 +7,15 @@
 #include <errno.h>
 #include <signal.h>
 #include <string.h>
-#include <sys/prctl.h>
 #include <sys/syscall.h>
 
+#include <algorithm>
 #include <limits>
 
 #include "base/logging.h"
-#include "sandbox/linux/seccomp-bpf/codegen.h"
+#include "build/build_config.h"
 #include "sandbox/linux/seccomp-bpf/die.h"
+#include "sandbox/linux/seccomp-bpf/linux_seccomp.h"
 #include "sandbox/linux/seccomp-bpf/syscall.h"
 
 // Android's signal.h doesn't define ucontext etc.
@@ -24,6 +25,12 @@
 
 namespace {
 
+struct arch_sigsys {
+  void* ip;
+  int nr;
+  unsigned int arch;
+};
+
 const int kCapacityIncrement = 20;
 
 // Unsafe traps can only be turned on, if the user explicitly allowed them
@@ -200,8 +207,8 @@ void Trap::SigSys(int nr, siginfo_t* info, void* void_context) {
                        SECCOMP_PARM6(ctx));
 #endif  // defined(__mips__)
   } else {
-    const ErrorCode& err = trap_array_[info->si_errno - 1];
-    if (!err.safe_) {
+    const TrapKey& trap = trap_array_[info->si_errno - 1];
+    if (!trap.safe) {
       SetIsInSigHandler();
     }
 
@@ -221,7 +228,7 @@ void Trap::SigSys(int nr, siginfo_t* info, void* void_context) {
 
     // Now call the TrapFnc callback associated with this particular instance
     // of SECCOMP_RET_TRAP.
-    rc = err.fnc_(data, err.aux_);
+    rc = trap.fnc(data, const_cast<void*>(trap.aux));
   }
 
   // Update the CPU register that stores the return code of the system call
@@ -243,11 +250,11 @@ bool Trap::TrapKey::operator<(const TrapKey& o) const {
   }
 }
 
-ErrorCode Trap::MakeTrap(TrapFnc fnc, const void* aux, bool safe) {
+uint16_t Trap::MakeTrap(TrapFnc fnc, const void* aux, bool safe) {
   return GetInstance()->MakeTrapImpl(fnc, aux, safe);
 }
 
-ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
+uint16_t Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
   if (!safe && !SandboxDebuggingAllowedByUser()) {
     // Unless the user set the CHROME_SANDBOX_DEBUGGING environment variable,
     // we never return an ErrorCode that is marked as "unsafe". This also
@@ -257,20 +264,19 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
 
     // This SANDBOX_DIE() can optionally be removed. It won't break security,
     // but it might make error messages from the BPF compiler a little harder
-    // to understand. Removing the SANDBOX_DIE() allows callers to easyly check
+    // to understand. Removing the SANDBOX_DIE() allows callers to easily check
     // whether unsafe traps are supported (by checking whether the returned
     // ErrorCode is ET_INVALID).
     SANDBOX_DIE(
         "Cannot use unsafe traps unless CHROME_SANDBOX_DEBUGGING "
         "is enabled");
 
-    return ErrorCode();
+    return 0;
   }
 
   // Each unique pair of TrapFnc and auxiliary data make up a distinct instance
   // of a SECCOMP_RET_TRAP.
   TrapKey key(fnc, aux, safe);
-  TrapIds::const_iterator iter = trap_ids_.find(key);
 
   // We return unique identifiers together with SECCOMP_RET_TRAP. This allows
   // us to associate trap with the appropriate handler. The kernel allows us
@@ -280,66 +286,70 @@ ErrorCode Trap::MakeTrapImpl(TrapFnc fnc, const void* aux, bool safe) {
   // trivially look them up from our signal handler without making any system
   // calls that might be async-signal-unsafe.
   // In order to do so, we store all of our traps in a C-style trap_array_.
-  uint16_t id;
+
+  TrapIds::const_iterator iter = trap_ids_.find(key);
   if (iter != trap_ids_.end()) {
     // We have seen this pair before. Return the same id that we assigned
     // earlier.
-    id = iter->second;
-  } else {
-    // This is a new pair. Remember it and assign a new id.
-    if (trap_array_size_ >= SECCOMP_RET_DATA /* 0xFFFF */ ||
-        trap_array_size_ >= std::numeric_limits<typeof(id)>::max()) {
-      // In practice, this is pretty much impossible to trigger, as there
-      // are other kernel limitations that restrict overall BPF program sizes.
-      SANDBOX_DIE("Too many SECCOMP_RET_TRAP callback instances");
-    }
-    id = trap_array_size_ + 1;
-
-    // Our callers ensure that there are no other threads accessing trap_array_
-    // concurrently (typically this is done by ensuring that we are single-
-    // threaded while the sandbox is being set up). But we nonetheless are
-    // modifying a life data structure that could be accessed any time a
-    // system call is made; as system calls could be triggering SIGSYS.
-    // So, we have to be extra careful that we update trap_array_ atomically.
-    // In particular, this means we shouldn't be using realloc() to resize it.
-    // Instead, we allocate a new array, copy the values, and then switch the
-    // pointer. We only really care about the pointer being updated atomically
-    // and the data that is pointed to being valid, as these are the only
-    // values accessed from the signal handler. It is OK if trap_array_size_
-    // is inconsistent with the pointer, as it is monotonously increasing.
-    // Also, we only care about compiler barriers, as the signal handler is
-    // triggered synchronously from a system call. We don't have to protect
-    // against issues with the memory model or with completely asynchronous
-    // events.
-    if (trap_array_size_ >= trap_array_capacity_) {
-      trap_array_capacity_ += kCapacityIncrement;
-      ErrorCode* old_trap_array = trap_array_;
-      ErrorCode* new_trap_array = new ErrorCode[trap_array_capacity_];
-
-      // Language specs are unclear on whether the compiler is allowed to move
-      // the "delete[]" above our preceding assignments and/or memory moves,
-      // iff the compiler believes that "delete[]" doesn't have any other
-      // global side-effects.
-      // We insert optimization barriers to prevent this from happening.
-      // The first barrier is probably not needed, but better be explicit in
-      // what we want to tell the compiler.
-      // The clang developer mailing list couldn't answer whether this is a
-      // legitimate worry; but they at least thought that the barrier is
-      // sufficient to prevent the (so far hypothetical) problem of re-ordering
-      // of instructions by the compiler.
-      memcpy(new_trap_array, trap_array_, trap_array_size_ * sizeof(ErrorCode));
-      asm volatile("" : "=r"(new_trap_array) : "0"(new_trap_array) : "memory");
-      trap_array_ = new_trap_array;
-      asm volatile("" : "=r"(trap_array_) : "0"(trap_array_) : "memory");
-
-      delete[] old_trap_array;
-    }
-    trap_ids_[key] = id;
-    trap_array_[trap_array_size_] = ErrorCode(fnc, aux, safe, id);
-    return trap_array_[trap_array_size_++];
+    return iter->second;
+  }
+
+  // This is a new pair. Remember it and assign a new id.
+  if (trap_array_size_ >= SECCOMP_RET_DATA /* 0xFFFF */ ||
+      trap_array_size_ >= std::numeric_limits<uint16_t>::max()) {
+    // In practice, this is pretty much impossible to trigger, as there
+    // are other kernel limitations that restrict overall BPF program sizes.
+    SANDBOX_DIE("Too many SECCOMP_RET_TRAP callback instances");
   }
 
-  return ErrorCode(fnc, aux, safe, id);
+  // Our callers ensure that there are no other threads accessing trap_array_
+  // concurrently (typically this is done by ensuring that we are single-
+  // threaded while the sandbox is being set up). But we nonetheless are
+  // modifying a live data structure that could be accessed any time a
+  // system call is made; as system calls could be triggering SIGSYS.
+  // So, we have to be extra careful that we update trap_array_ atomically.
+  // In particular, this means we shouldn't be using realloc() to resize it.
+  // Instead, we allocate a new array, copy the values, and then switch the
+  // pointer. We only really care about the pointer being updated atomically
+  // and the data that is pointed to being valid, as these are the only
+  // values accessed from the signal handler. It is OK if trap_array_size_
+  // is inconsistent with the pointer, as it is monotonously increasing.
+  // Also, we only care about compiler barriers, as the signal handler is
+  // triggered synchronously from a system call. We don't have to protect
+  // against issues with the memory model or with completely asynchronous
+  // events.
+  if (trap_array_size_ >= trap_array_capacity_) {
+    trap_array_capacity_ += kCapacityIncrement;
+    TrapKey* old_trap_array = trap_array_;
+    TrapKey* new_trap_array = new TrapKey[trap_array_capacity_];
+    std::copy_n(old_trap_array, trap_array_size_, new_trap_array);
+
+    // Language specs are unclear on whether the compiler is allowed to move
+    // the "delete[]" above our preceding assignments and/or memory moves,
+    // iff the compiler believes that "delete[]" doesn't have any other
+    // global side-effects.
+    // We insert optimization barriers to prevent this from happening.
+    // The first barrier is probably not needed, but better be explicit in
+    // what we want to tell the compiler.
+    // The clang developer mailing list couldn't answer whether this is a
+    // legitimate worry; but they at least thought that the barrier is
+    // sufficient to prevent the (so far hypothetical) problem of re-ordering
+    // of instructions by the compiler.
+    //
+    // TODO(mdempsky): Try to clean this up using base/atomicops or C++11
+    // atomics; see crbug.com/414363.
+    asm volatile("" : "=r"(new_trap_array) : "0"(new_trap_array) : "memory");
+    trap_array_ = new_trap_array;
+    asm volatile("" : "=r"(trap_array_) : "0"(trap_array_) : "memory");
+
+    delete[] old_trap_array;
+  }
+
+  uint16_t id = trap_array_size_ + 1;
+  trap_ids_[key] = id;
+  trap_array_[trap_array_size_] = key;
+  trap_array_size_++;
+  return id;
 }
 
 bool Trap::SandboxDebuggingAllowedByUser() const {
@@ -370,12 +380,11 @@ bool Trap::EnableUnsafeTrapsInSigSysHandler() {
   return trap->has_unsafe_traps_;
 }
 
-ErrorCode Trap::ErrorCodeFromTrapId(uint16_t id) {
+bool Trap::IsSafeTrapId(uint16_t id) {
   if (global_trap_ && id > 0 && id <= global_trap_->trap_array_size_) {
-    return global_trap_->trap_array_[id - 1];
-  } else {
-    return ErrorCode();
+    return global_trap_->trap_array_[id - 1].safe;
   }
+  return false;
 }
 
 Trap* Trap::global_trap_;

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
@@ -104,7 +111,7 @@ class SANDBOX_EXPORT Trap {
   static Trap* global_trap_;
 
   TrapIds trap_ids_;            // Maps from TrapKeys to numeric ids
-  ErrorCode* trap_array_;       // Array of ErrorCodes indexed by ids
+  TrapKey* trap_array_;         // Array of TrapKeys 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