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..e2c71eeb61be6315364e5070302193a6b8bafaa7 100644
--- a/sandbox/linux/tests/unit_tests.cc
+++ b/sandbox/linux/tests/unit_tests.cc
@@ -2,13 +2,72 @@
 // 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(pipe(fds), 0);

[jln (very slow on Chromium), 2012/08/24 04:00:32]

The true syntax is strange, it's ASSERT_EQ(expected, actual). So you should
reverse the arguments.

+
+  pid_t pid;
+  ASSERT_GE((pid = fork()), 0);
+  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);
+  }
+
+  HANDLE_EINTR(close(fds[1]));

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Add (void) to silence warnings.

+  std::vector<char> msg;
+  ssize_t rc;
+  do {
+    const unsigned int kCapacity = 256;
+    size_t len = msg.size();
+    msg.resize(len + kCapacity);

[jln (very slow on Chromium), 2012/08/24 04:00:32]

I'll address my int overflow OCD in a later CL.

+    rc = HANDLE_EINTR(read(fds[0], &msg[len], kCapacity));
+    msg.resize(len + std::max(rc, static_cast<ssize_t>(0)));
+  } while (rc > 0);
+  HANDLE_EINTR(close(fds[0]));

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Add (void) to silence warnings.

+
+  // 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

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Did you forget a word here ? Also add a final dot.

+  EXPECT_EQ(msg.size(), 0u) << std::string(msg.begin(), msg.end());

[jln (very slow on Chromium), 2012/08/24 04:06:42]

Currently, this will result in something such as:

../../sandbox/linux/tests/unit_tests.cc:62: Failure
Value of: msg.size()
  Actual: 88
Expected: 0u
Which is: 0

This is really confusing. I would suggest to do:

bool sub_process_has_error_message = msg.size() == 0u;
EXPECT_FALSE(sub_process_has_error_message) << XXXXXXXXXXXXX

So that GTest's error message is more clear.

[jln (very slow on Chromium), 2012/08/24 04:26:21]

Something such as:

  bool subprocess_has_error_message = msg.size() > 0;
  ASSERT_FALSE(subprocess_has_error_message) << "Actual test failure: " 
                                             << std::string(msg.begin(),
                                                            msg.end());

Gives a reasonable result.

+
+  int status;

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Please, initialize it.

+  HANDLE_EINTR(waitpid(pid, &status, 0));

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Add a SANDBOX_ASSERT(waitpid() >= 0 here ?

Right now it's especially bad because status is not initialized.

[jln (very slow on Chromium), 2012/08/24 04:02:43]

Err, I mean ASSERT_GE of course, don't use SANDBOX_ASSERT here!

+  SANDBOX_ASSERT(WIFEXITED(status));

[jln (very slow on Chromium), 2012/08/24 04:00:32]

We still have the same major bug as before: we're not testing anything at the
moment!
If we exit (which SANDBOX_ASSERT does), we'll fail silently.

This needs to be a GTEST macro: ASSERT_TRUE.

+  SANDBOX_ASSERT(WEXITSTATUS(status) == kExpectedValue);

[jln (very slow on Chromium), 2012/08/24 04:00:32]

Same here, this needs to be ASSERT_EQ().
Be careful with the order: ASSERT_EQ(expected, actual).

[jln (very slow on Chromium), 2012/08/24 04:26:21]

Because of how macro expansion works, this gives a really unreadable error
message.
Use:

int test_process_exit_status = WEXITSTATUS(status);
ASSERT_EQ(kExpectedValue, test_process_exit_status);

instead, so that gtest outputs:

Value of: test_process_exit_status
  Actual: 1
Expected: kExpectedValue
Which is: 42

 }
+
+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