From fa08872963455d9142dff82ab6cc83f0ddf5e9bc Mon Sep 17 00:00:00 2001 From: "jln@chromium.org" Date: Sat, 23 Nov 2013 00:44:47 +0000 Subject: Linux sandbox: cleanup BrokerProcess class. A few style guide cleanups and more usage of scopers. BUG=316486 R=rsesek@chromium.org Review URL: https://codereview.chromium.org/84183002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236883 0039d316-1c4b-4281-b951-d872f2087c98 --- sandbox/linux/services/broker_process.cc | 9 +++--- sandbox/linux/services/broker_process.h | 25 +++++++++------- sandbox/linux/services/broker_process_unittest.cc | 35 +++++++++++------------ 3 files changed, 36 insertions(+), 33 deletions(-) (limited to 'sandbox') diff --git a/sandbox/linux/services/broker_process.cc b/sandbox/linux/services/broker_process.cc index 81d78b0..0e91c20 100644 --- a/sandbox/linux/services/broker_process.cc +++ b/sandbox/linux/services/broker_process.cc @@ -16,6 +16,7 @@ #include #include "base/basictypes.h" +#include "base/compiler_specific.h" #include "base/logging.h" #include "base/pickle.h" #include "base/posix/eintr_wrapper.h" @@ -147,13 +148,13 @@ bool BrokerProcess::Init(bool (*sandbox_callback)(void)) { int child_pid = fork(); if (child_pid == -1) { - (void) HANDLE_EINTR(close(socket_pair[0])); - (void) HANDLE_EINTR(close(socket_pair[1])); + ignore_result(HANDLE_EINTR(close(socket_pair[0]))); + ignore_result(HANDLE_EINTR(close(socket_pair[1]))); return false; } if (child_pid) { // We are the parent and we have just forked our broker process. - (void) HANDLE_EINTR(close(socket_pair[0])); + ignore_result(HANDLE_EINTR(close(socket_pair[0]))); // We should only be able to write to the IPC channel. We'll always send // a new file descriptor to receive the reply on. shutdown(socket_pair[1], SHUT_RD); @@ -164,7 +165,7 @@ bool BrokerProcess::Init(bool (*sandbox_callback)(void)) { return true; } else { // We are the broker. - (void) HANDLE_EINTR(close(socket_pair[1])); + ignore_result(HANDLE_EINTR(close(socket_pair[1]))); // We should only be able to read from this IPC channel. We will send our // replies on a new file descriptor attached to the requests. shutdown(socket_pair[0], SHUT_WR); diff --git a/sandbox/linux/services/broker_process.h b/sandbox/linux/services/broker_process.h index 00218a0..6b13b33 100644 --- a/sandbox/linux/services/broker_process.h +++ b/sandbox/linux/services/broker_process.h @@ -66,26 +66,31 @@ class BrokerProcess { kCommandAccess, }; int PathAndFlagsSyscall(enum IPCCommands command_type, - const char* pathname, int flags) const; + const char* pathname, + int flags) const; bool HandleRequest() const; - bool HandleRemoteCommand(IPCCommands command_type, int reply_ipc, - const Pickle& read_pickle, PickleIterator iter) const; + bool HandleRemoteCommand(IPCCommands command_type, + int reply_ipc, + const Pickle& read_pickle, + PickleIterator iter) const; void AccessFileForIPC(const std::string& requested_filename, - int mode, Pickle* write_pickle) const; + int mode, + Pickle* write_pickle) const; void OpenFileForIPC(const std::string& requested_filename, - int flags, Pickle* write_pickle, + int flags, + Pickle* write_pickle, std::vector* opened_files) const; bool GetFileNameIfAllowedToAccess(const char*, int, const char**) const; bool GetFileNameIfAllowedToOpen(const char*, int, const char**) const; const int denied_errno_; - bool initialized_; // Whether we've been through Init() yet. - bool is_child_; // Whether we're the child (broker process). - bool fast_check_in_client_; // Whether to forward a request that we know - // will be denied to the broker. + bool initialized_; // Whether we've been through Init() yet. + bool is_child_; // Whether we're the child (broker process). + bool fast_check_in_client_; // Whether to forward a request that we know + // will be denied to the broker. bool quiet_failures_for_tests_; // Disable certain error message when // testing for failures. - pid_t broker_pid_; // The PID of the broker (child). + pid_t broker_pid_; // The PID of the broker (child). const std::vector allowed_r_files_; // Files allowed for read. const std::vector allowed_w_files_; // Files allowed for write. int ipc_socketpair_; // Our communication channel to parent or child. diff --git a/sandbox/linux/services/broker_process_unittest.cc b/sandbox/linux/services/broker_process_unittest.cc index 6ce294e..4cb9c6f 100644 --- a/sandbox/linux/services/broker_process_unittest.cc +++ b/sandbox/linux/services/broker_process_unittest.cc @@ -15,11 +15,15 @@ #include #include "base/basictypes.h" +#include "base/file_util.h" #include "base/logging.h" +#include "base/memory/scoped_ptr.h" #include "base/posix/eintr_wrapper.h" #include "sandbox/linux/tests/unit_tests.h" #include "testing/gtest/include/gtest/gtest.h" +using file_util::ScopedFD; + namespace sandbox { namespace { @@ -68,14 +72,13 @@ TEST(BrokerProcess, CreateAndDestroy) { std::vector read_whitelist; read_whitelist.push_back("/proc/cpuinfo"); - BrokerProcess* open_broker = new BrokerProcess(EPERM, - read_whitelist, - std::vector()); + scoped_ptr open_broker( + new BrokerProcess(EPERM, read_whitelist, std::vector())); ASSERT_TRUE(open_broker->Init(NULL)); pid_t broker_pid = open_broker->broker_pid(); - delete(open_broker); - // Now we check that the broker has exited properly. + // 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)); @@ -224,12 +227,12 @@ void TestOpenFilePerms(bool fast_check_in_client, int denied_errno) { ASSERT_EQ(ret, -denied_errno); // We have some extra sanity check for clearly wrong values. - fd = open_broker.Open(kRW_WhiteListed, O_RDONLY|O_WRONLY|O_RDWR); + fd = open_broker.Open(kRW_WhiteListed, O_RDONLY | O_WRONLY | O_RDWR); ASSERT_EQ(fd, -denied_errno); // It makes no sense to allow O_CREAT in a 2-parameters open. Ensure this // is denied. - fd = open_broker.Open(kRW_WhiteListed, O_RDWR|O_CREAT); + fd = open_broker.Open(kRW_WhiteListed, O_RDWR | O_CREAT); ASSERT_EQ(fd, -denied_errno); } @@ -265,15 +268,14 @@ void TestOpenCpuinfo(bool fast_check_in_client) { std::vector read_whitelist; read_whitelist.push_back(kFileCpuInfo); - BrokerProcess* open_broker = new BrokerProcess(EPERM, - read_whitelist, - std::vector(), - fast_check_in_client); + scoped_ptr open_broker(new BrokerProcess( + EPERM, read_whitelist, std::vector(), fast_check_in_client)); ASSERT_TRUE(open_broker->Init(NULL)); pid_t broker_pid = open_broker->broker_pid(); int fd = -1; fd = open_broker->Open(kFileCpuInfo, O_RDWR); + ScopedFD fd_closer(&fd); ASSERT_EQ(fd, -EPERM); // Check we can read /proc/cpuinfo. @@ -285,6 +287,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) { // Open cpuinfo via the broker. int cpuinfo_fd = open_broker->Open(kFileCpuInfo, O_RDONLY); + ScopedFD cpuinfo_fd_closer(&cpuinfo_fd); ASSERT_GE(cpuinfo_fd, 0); char buf[3]; memset(buf, 0, sizeof(buf)); @@ -293,6 +296,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) { // Open cpuinfo directly. int cpuinfo_fd2 = open(kFileCpuInfo, O_RDONLY); + ScopedFD cpuinfo_fd2_closer(&cpuinfo_fd2); ASSERT_GE(cpuinfo_fd2, 0); char buf2[3]; memset(buf2, 1, sizeof(buf2)); @@ -305,14 +309,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) { // ourselves. ASSERT_EQ(memcmp(buf, buf2, read_len1), 0); - if (fd >= 0) - close(fd); - if (cpuinfo_fd >= 0) - close(cpuinfo_fd); - if (cpuinfo_fd2 >= 0) - close(cpuinfo_fd); - - delete(open_broker); + open_broker.reset(); // Now we check that the broker has exited properly. int status = 0; -- cgit v1.1