Seccomp-BPF: relax failure in probe process setup

sandbox/linux/seccomp-bpf/sandbox_bpf.cc

Index: sandbox/linux/seccomp-bpf/sandbox_bpf.cc
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index 6926b5dfdd16c4b8df2dcc0f8a63d072cf0abcc4..e0c8937d39d536360cf0c4ae5102ecc4d337ac70 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -7,11 +7,26 @@
 #include "sandbox/linux/seccomp-bpf/syscall_iterator.h"
 #include "sandbox/linux/seccomp-bpf/verifier.h"
 
+namespace {
+
+void WriteFailedStderrSetupMessage(int out_fd) {
+  const char* error_string = strerror(errno);
+  static const char msg[] = "Failed to set up stderr: ";
+  if (HANDLE_EINTR(write(out_fd, msg, sizeof(msg)-1)) > 0 && error_string &&
+      HANDLE_EINTR(write(out_fd, error_string, strlen(error_string))) > 0 &&
+      HANDLE_EINTR(write(out_fd, "\n", 1))) {

[Markus (顧孟勤), 2012/10/26 23:14:11]

After this change, do we still expect to see these messages in some place? If
so, should we consider logging a lot more data?

We could log the output from fstat(2) (we should probably collect this data
before trying to close stderr).

We could log the file descriptors returned by pipe() and we could log whether
they seem to be functional (e.g. do another fstat())

Instead of doing just a dup2(2), we could first attempt a close(2) and log it's
result.

There are probably a few other things that might be interesting but that I can't
think of right now.

In either case, since you can now call WriteFailedStderrSetupMessage() from
multiple locations, I would strongly suggest you add some data to disambiguate
why we returning this message. That could make debugging much easier.

[jln (very slow on Chromium), 2012/10/29 21:15:37]

That makes sense. But let's land a simple, easy to merge patch first, and we can
have something more audacious and add some logging information on the dev
channel afterwards. What do you think ?

+  }
+}
+
+}  // namespace
+
 // The kernel gives us a sandbox, we turn it into a playground :-)
 // This is version 2 of the playground; version 1 was built on top of
 // pre-BPF seccomp mode.
 namespace playground2 {
 
+const int kExpectedExitCode = 100;
+
 // We define a really simple sandbox policy. It is just good enough for us
 // to tell that the sandbox has actually been activated.
 ErrorCode Sandbox::probeEvaluator(int signo) {
@@ -30,7 +45,7 @@ ErrorCode Sandbox::probeEvaluator(int signo) {
 
 void Sandbox::probeProcess(void) {
   if (syscall(__NR_getpid) < 0 && errno == EPERM) {
-    syscall(__NR_exit_group, (intptr_t)100);
+    syscall(__NR_exit_group, static_cast<intptr_t>(kExpectedExitCode));
   }
 }
 
@@ -51,7 +66,7 @@ void Sandbox::tryVsyscallProcess(void) {
   // vsyscall=emulate and some versions of the seccomp BPF patch
   // we may get SIGKILL-ed. Detect this!
   if (time(&current_time) != static_cast<time_t>(-1)) {
-    syscall(__NR_exit_group, (intptr_t)100);
+    syscall(__NR_exit_group, static_cast<intptr_t>(kExpectedExitCode));
   }
 }
 
@@ -70,6 +85,10 @@ bool Sandbox::RunFunctionInPolicy(void (*CodeInSandbox)(),
     SANDBOX_DIE("pipe() failed");
   }
 
+  if (fds[0] <= 2 || fds[1] <= 2) {
+    SANDBOX_DIE("Process started without standard file descriptors");
+  }
+
   pid_t pid = fork();
   if (pid < 0) {
     // Die if we cannot fork(). We would probably fail a little later
@@ -87,29 +106,37 @@ bool Sandbox::RunFunctionInPolicy(void (*CodeInSandbox)(),
     // Test a very simple sandbox policy to verify that we can
     // successfully turn on sandboxing.
     Die::EnableSimpleExit();
-    errno = 0;
-    if (HANDLE_EINTR(close(fds[0])) ||
-        HANDLE_EINTR(dup2(fds[1], 2)) != 2 ||
-        HANDLE_EINTR(close(fds[1]))) {
-      const char* error_string = strerror(errno);
-      static const char msg[] = "Failed to set up stderr: ";
-      if (HANDLE_EINTR(write(fds[1], msg, sizeof(msg)-1)) > 0 && error_string &&
-          HANDLE_EINTR(write(fds[1], error_string, strlen(error_string))) > 0 &&
-          HANDLE_EINTR(write(fds[1], "\n", 1))) {
-      }
-    } else {
-      evaluators_.clear();
-      setSandboxPolicy(syscallEvaluator, NULL);
-      setProcFd(proc_fd);
 
-      // By passing "quiet=true" to "startSandboxInternal()" we suppress
-      // messages for expected and benign failures (e.g. if the current
-      // kernel lacks support for BPF filters).
-      startSandboxInternal(true);
-
-      // Run our code in the sandbox
-      CodeInSandbox();
+    if (HANDLE_EINTR(close(fds[0]))) {
+      WriteFailedStderrSetupMessage(fds[1]);
+      SANDBOX_DIE(NULL);
+    }
+    if (HANDLE_EINTR(dup2(fds[1], 2)) != 2) {

[Markus (顧孟勤), 2012/10/26 23:14:11]

Try calling HANDLE_EINTR(close(2)) first and ignore the exit code. That seems
harmless and might just be good enough to make dup2() happy. Who knows.

+      // Stderr could very well be a file descriptor to .xsession-errors, or
+      // another file, which could be backed by a file system that could cause
+      // dup2 to fail while trying to close stderr. It's important that we do
+      // not fail on trying to close stderr.
+      // If dup2 fails here, we will continue normally, this means that our
+      // parent won't cause a fatal failure if something writes to stderr in
+      // this child.

[Markus (顧孟勤), 2012/10/26 23:14:11]

Do you still want this to be fatal in debug builds?

[jln (very slow on Chromium), 2012/10/29 21:15:37]

Yeah, the first iteration of this patch, as you can see on the review tool),
made it so, but this should wait for a refactor of sandbox_bpf.cc into a clean
interface, because we're not even including proper header files here (and are
relying on the .h file to do it instead).

Since the way it works is to check for NDEBUG (for non debug), I was scared that
we may have a bug of NDEBUG not being defined not because we are in a debug
build, but just because something strange happened with our headers.

     }
+    if (HANDLE_EINTR(close(fds[1]))) {
+      WriteFailedStderrSetupMessage(fds[1]);
+      SANDBOX_DIE(NULL);
+    }
+
+    evaluators_.clear();
+    setSandboxPolicy(syscallEvaluator, NULL);
+    setProcFd(proc_fd);
+
+    // By passing "quiet=true" to "startSandboxInternal()" we suppress
+    // messages for expected and benign failures (e.g. if the current
+    // kernel lacks support for BPF filters).
+    startSandboxInternal(true);
+
+    // Run our code in the sandbox.
+    CodeInSandbox();

[Markus (顧孟勤), 2012/10/26 23:14:11]

I'd prefer a blank line before the comment, as I feel it is easier to read
quickly. But it's your call, if you prefer the denser code without extra blank
lines.

[jln (very slow on Chromium), 2012/10/29 21:15:37]

Done.

+    // CodeInSandbox() is not supposed to return here.
     SANDBOX_DIE(NULL);
   }
 
@@ -124,7 +151,7 @@ bool Sandbox::RunFunctionInPolicy(void (*CodeInSandbox)(),
   if (HANDLE_EINTR(waitpid(pid, &status, 0)) != pid) {
     SANDBOX_DIE("waitpid() failed unexpectedly");
   }
-  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == 100;
+  bool rc = WIFEXITED(status) && WEXITSTATUS(status) == kExpectedExitCode;
 
   // If we fail to support sandboxing, there might be an additional
   // error message. If so, this was an entirely unexpected and fatal