Linux Sandbox: scope child process with BrokerProcess class.

Make sure that the broker process (the child) dies when a BrokerProcess
object is destroyed.

This also adds automatic detection of leaking processes in
sandbox_linux_unittests.

R=jorgelo@chromium.org, mdempsky@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256027 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 102 insertions, 20 deletions

diff --git a/sandbox/linux/sandbox_linux_test_sources.gypi b/sandbox/linux/sandbox_linux_test_sources.gypi
index d6d4f8e..a48017b 100644
--- a/sandbox/linux/sandbox_linux_test_sources.gypi
+++ b/sandbox/linux/sandbox_linux_test_sources.gypi
@@ -15,6 +15,8 @@
   ],
   'sources': [
     'tests/main.cc',
+    'tests/test_utils.cc',
+    'tests/test_utils.h',
     'tests/unit_tests.cc',
     'tests/unit_tests.h',
     'services/broker_process_unittest.cc',
diff --git a/sandbox/linux/services/broker_process.cc b/sandbox/linux/services/broker_process.cc
index ac4b3c4..2956cf9 100644
--- a/sandbox/linux/services/broker_process.cc
+++ b/sandbox/linux/services/broker_process.cc
@@ -5,10 +5,12 @@
 #include "sandbox/linux/services/broker_process.h"
 
 #include <fcntl.h>
+#include <signal.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
 #include <algorithm>
@@ -133,7 +135,14 @@ BrokerProcess::BrokerProcess(int denied_errno,
 
 BrokerProcess::~BrokerProcess() {
   if (initialized_ && ipc_socketpair_ != -1) {
-    close(ipc_socketpair_);
+    // Closing the socket should be enough to notify the child to die,
+    // unless it has been duplicated.
+    PCHECK(0 == IGNORE_EINTR(close(ipc_socketpair_)));
+    PCHECK(0 == kill(broker_pid_, SIGKILL));
+    siginfo_t process_info;
+    // Reap the child.
+    int ret = HANDLE_EINTR(waitid(P_PID, broker_pid_, &process_info, WEXITED));
+    PCHECK(0 == ret);
   }
 }
 
diff --git a/sandbox/linux/services/broker_process_unittest.cc b/sandbox/linux/services/broker_process_unittest.cc
index f3effa10..771790a 100644
--- a/sandbox/linux/services/broker_process_unittest.cc
+++ b/sandbox/linux/services/broker_process_unittest.cc
@@ -20,6 +20,7 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/posix/eintr_wrapper.h"
+#include "sandbox/linux/tests/test_utils.h"
 #include "sandbox/linux/tests/unit_tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -72,14 +73,11 @@ TEST(BrokerProcess, CreateAndDestroy) {
   scoped_ptr<BrokerProcess> open_broker(
       new BrokerProcess(EPERM, read_whitelist, std::vector<std::string>()));
   ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
-  pid_t broker_pid = open_broker->broker_pid();
 
+  ASSERT_TRUE(TestUtils::CurrentProcessHasChildren());
   // Destroy the broker and check it has exited properly.
   open_broker.reset();
-  int status = 0;
-  ASSERT_EQ(waitpid(broker_pid, &status, 0), broker_pid);
-  ASSERT_TRUE(WIFEXITED(status));
-  ASSERT_EQ(WEXITSTATUS(status), 0);
+  ASSERT_FALSE(TestUtils::CurrentProcessHasChildren());
 }
 
 TEST(BrokerProcess, TestOpenAccessNull) {
@@ -268,7 +266,6 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
   scoped_ptr<BrokerProcess> open_broker(new BrokerProcess(
       EPERM, read_whitelist, std::vector<std::string>(), fast_check_in_client));
   ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
-  pid_t broker_pid = open_broker->broker_pid();
 
   int fd = -1;
   fd = open_broker->Open(kFileCpuInfo, O_RDWR);
@@ -306,13 +303,9 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
   // ourselves.
   ASSERT_EQ(memcmp(buf, buf2, read_len1), 0);
 
+  ASSERT_TRUE(TestUtils::CurrentProcessHasChildren());
   open_broker.reset();
-
-  // Now we check that the broker has exited properly.
-  int status = 0;
-  ASSERT_EQ(waitpid(broker_pid, &status, 0), broker_pid);
-  ASSERT_TRUE(WIFEXITED(status));
-  ASSERT_EQ(WEXITSTATUS(status), 0);
+  ASSERT_FALSE(TestUtils::CurrentProcessHasChildren());
 }
 
 // Run the same thing twice. The second time, we make sure that no security
@@ -375,14 +368,18 @@ SANDBOX_TEST(BrokerProcess, BrokerDied) {
                             true /* fast_check_in_client */,
                             true /* quiet_failures_for_tests */);
   SANDBOX_ASSERT(open_broker.Init(base::Bind(&NoOpCallback)));
-  pid_t broker_pid = open_broker.broker_pid();
+  const pid_t broker_pid = open_broker.broker_pid();
   SANDBOX_ASSERT(kill(broker_pid, SIGKILL) == 0);
 
-  // Now we check that the broker has exited properly.
-  int status = 0;
-  SANDBOX_ASSERT(waitpid(broker_pid, &status, 0) == broker_pid);
-  SANDBOX_ASSERT(WIFSIGNALED(status));
-  SANDBOX_ASSERT(WTERMSIG(status) == SIGKILL);
+  // Now we check that the broker has been signaled, but do not reap it.
+  siginfo_t process_info;
+  SANDBOX_ASSERT(HANDLE_EINTR(waitid(
+                     P_PID, broker_pid, &process_info, WEXITED | WNOWAIT)) ==
+                 0);
+  SANDBOX_ASSERT(broker_pid == process_info.si_pid);
+  SANDBOX_ASSERT(CLD_KILLED == process_info.si_code);
+  SANDBOX_ASSERT(SIGKILL == process_info.si_status);
+
   // Check that doing Open with a dead broker won't SIGPIPE us.
   SANDBOX_ASSERT(open_broker.Open("/proc/cpuinfo", O_RDONLY) == -ENOMEM);
   SANDBOX_ASSERT(open_broker.Access("/proc/cpuinfo", O_RDONLY) == -ENOMEM);
diff --git a/sandbox/linux/tests/main.cc b/sandbox/linux/tests/main.cc
index 8fd85d9..b07718e 100644
--- a/sandbox/linux/tests/main.cc
+++ b/sandbox/linux/tests/main.cc
@@ -3,8 +3,25 @@
 // found in the LICENSE file.
 
 #include "base/at_exit.h"
+#include "base/logging.h"
+#include "sandbox/linux/tests/test_utils.h"
+#include "sandbox/linux/tests/unit_tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace sandbox {
+namespace {
+
+// Check for leaks in our tests.
+void RunPostTestsChecks() {
+  if (TestUtils::CurrentProcessHasChildren()) {
+    LOG(ERROR) << "One of the tests created a child that was not waited for. "
+               << "Please, clean-up after your tests!";
+  }
+}
+
+}  // namespace
+}  // namespace sandbox
+
 int main(int argc, char* argv[]) {
   // The use of Callbacks requires an AtExitManager.
   base::AtExitManager exit_manager;
@@ -14,5 +31,8 @@ int main(int argc, char* argv[]) {
   // additional side effect of getting rid of gtest warnings about fork()
   // safety.
   ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-  return RUN_ALL_TESTS();
+  int tests_result = RUN_ALL_TESTS();
+
+  sandbox::RunPostTestsChecks();
+  return tests_result;
 }
diff --git a/sandbox/linux/tests/test_utils.cc b/sandbox/linux/tests/test_utils.cc
new file mode 100644
index 0000000..398654b
--- /dev/null
+++ b/sandbox/linux/tests/test_utils.cc
@@ -0,0 +1,31 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sandbox/linux/tests/test_utils.h"
+
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+
+namespace sandbox {
+
+bool TestUtils::CurrentProcessHasChildren() {
+  siginfo_t process_info;
+  int ret = HANDLE_EINTR(
+      waitid(P_ALL, 0, &process_info, WEXITED | WNOHANG | WNOWAIT));
+  if (-1 == ret) {
+    PCHECK(ECHILD == errno);
+    return false;
+  } else {
+    return true;
+  }
+}
+
+}  // namespace sandbox
diff --git a/sandbox/linux/tests/test_utils.h b/sandbox/linux/tests/test_utils.h
new file mode 100644
index 0000000..3269847
--- /dev/null
+++ b/sandbox/linux/tests/test_utils.h
@@ -0,0 +1,23 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SANDBOX_LINUX_TESTS_TEST_UTILS_H_
+#define SANDBOX_LINUX_TESTS_TEST_UTILS_H_
+
+#include "base/basictypes.h"
+
+namespace sandbox {
+
+// This class provide small helpers to help writing tests.
+class TestUtils {
+ public:
+  static bool CurrentProcessHasChildren();
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(TestUtils);
+};
+
+}  // namespace sandbox
+
+#endif  // SANDBOX_LINUX_TESTS_TEST_UTILS_H_