Simplified unit testing of sandboxing code.

sandbox/linux/tests/unit_tests.cc

Index: sandbox/linux/tests/unit_tests.cc
diff --git a/sandbox/linux/tests/unit_tests.cc b/sandbox/linux/tests/unit_tests.cc
index 2e24d8c07c8ffa27386099f439bbda83b1894e2c..8fab6c6c385fd923eb64af8f32b0f9a2bd2c15ca 100644
--- a/sandbox/linux/tests/unit_tests.cc
+++ b/sandbox/linux/tests/unit_tests.cc
@@ -2,13 +2,77 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "testing/gtest/include/gtest/gtest.h"
-int main(int argc, char *argv[]) {
-  testing::InitGoogleTest(&argc, argv);
-  // Always go through re-execution for death tests.
-  // This makes gtest only marginally slower for us and has the
-  // additional side effect of getting rid of gtest warnings about fork()
-  // safety.
-  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-  return RUN_ALL_TESTS();
+#include <stdio.h>
+#include <sys/resource.h>
+#include <sys/time.h>
+
+#include "base/file_util.h"
+#include "sandbox/linux/tests/unit_tests.h"
+
+namespace sandbox {
+
+static const int kExpectedValue = 42;
+
+void UnitTests::RunTestInProcess(UnitTests::Test test) {
+  // Runs a test in a sub-process. This is necessary for most of the code
+  // in the BPF sandbox, as it potentially makes global state changes and as
+  // it also tends to raise fatal errors, if the code has been used in an
+  // insecure manner.
+  int fds[2];
+  ASSERT_EQ(0, pipe(fds));
+
+  pid_t pid;
+  ASSERT_LT(0, (pid = fork()));

[jln (very slow on Chromium), 2012/08/24 18:30:07]

For GE, there is no "expected" value, so the previous version was correct.

I personally find the previous version easier to read, but it's up to you!

+  if (!pid) {
+    // In child process
+    // Redirect stderr to our pipe. This way, we can capture all error
+    // messages, if we decide we want to do so in our tests.
+    SANDBOX_ASSERT(dup2(fds[1], 2) == 2);
+    SANDBOX_ASSERT(!close(fds[0]));
+    SANDBOX_ASSERT(!close(fds[1]));
+
+    // Disable core files. They are not very useful for our individual test
+    // cases.
+    struct rlimit no_core = { 0 };
+    setrlimit(RLIMIT_CORE, &no_core);
+
+    test();
+    _exit(kExpectedValue);
+  }
+
+  (void)HANDLE_EINTR(close(fds[1]));
+  std::vector<char> msg;
+  ssize_t rc;
+  do {
+    const unsigned int kCapacity = 256;
+    size_t len = msg.size();
+    msg.resize(len + kCapacity);
+    rc = HANDLE_EINTR(read(fds[0], &msg[len], kCapacity));
+    msg.resize(len + std::max(rc, static_cast<ssize_t>(0)));
+  } while (rc > 0);
+  (void)HANDLE_EINTR(close(fds[0]));
+
+  // If we have read something, an error occurred and we want to print a
+  // message. As far as I can tell, gtest doesn't give us a good way to
+  // control what we are writing, so just make a best effort to print
+  // something intelligible.
+  bool completed_successfully = (msg.size() == 0u);
+  EXPECT_TRUE(completed_successfully)

[jln (very slow on Chromium), 2012/08/24 18:30:07]

This variable name will be in the GTest output. It's nice if we can give a
casual reader (such as a Sheriff watching the build bots) a good hint about
what's going on. So something mentioning "subprocess_test" or similar might me
nice. Up to you.

More important: it should still be clear what this does:
child_has_stderr_messages or something like that maybe ?

More important: "EXPECT_TRUE" will not terminate. This may be what you want, but
since this error message is the only one likely to be intelligible, it may be
better to ASSERT here so that as casual reader is spared the strange messages
about error codes below.

But see the discussion on top of this email!

+    << "Actual test failure: " << std::string(msg.begin(), msg.end());

[jln (very slow on Chromium), 2012/08/24 18:30:07]

Nit: this should be indented 4 spaces. I personally prefer having the first <<
on the previous line and align the second << with the previous one, but it's of
course up to you.

+
+  int status = 0;
+  EXPECT_EQ(pid, HANDLE_EINTR(waitpid(pid, &status, 0)));

[jln (very slow on Chromium), 2012/08/24 18:30:07]

Given how macros work, this gives an absolutely impossible message to read
because of the HANDLE_EINTR expansion.

It's unlikely to happen, so I don't mind.

+  bool terminated_normally = WIFEXITED(status);
+  EXPECT_TRUE(terminated_normally);

[jln (very slow on Chromium), 2012/08/24 18:30:07]

This one should really be an ASSERT ?

+  int exit_status = WEXITSTATUS(status);
+  EXPECT_EQ(kExpectedValue, exit_status);

[jln (very slow on Chromium), 2012/08/24 18:30:07]

Same here, ASSERT?

 }
+
+void UnitTests::AssertionFailure(const char *expr, const char *file,
+                                 int line) {
+  fprintf(stderr, "%s:%d:%s\n", file, line, expr);
+  fflush(stderr);
+  _exit(1);
+}
+
+}  // namespace