From eeaf460f820752a95ec91cbd29d76c9f7018de85 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 18 Sep 2014 15:03:49 -0400 Subject: [PATCH] Add and use CheckedReadFD(), CheckedWriteFD(), and CheckedReadFDAtEOF(). TEST=util_test R=rsesek@chromium.org Review URL: https://codereview.chromium.org/577333002 --- util/file/fd_io.cc | 32 ++++++ util/file/fd_io.h | 34 ++++++ util/mac/process_reader_test.cc | 152 +++++++------------------- util/mach/exception_ports_test.cc | 16 +-- util/mach/mach_message_server_test.cc | 13 +-- util/test/multiprocess_exec_test.cc | 9 +- util/test/multiprocess_test.cc | 24 ++-- 7 files changed, 124 insertions(+), 156 deletions(-) diff --git a/util/file/fd_io.cc b/util/file/fd_io.cc index 94258800..46a3634f 100644 --- a/util/file/fd_io.cc +++ b/util/file/fd_io.cc @@ -16,6 +16,8 @@ #include +#include "base/logging.h" +#include "base/numerics/safe_conversions.h" #include "base/posix/eintr_wrapper.h" namespace { @@ -72,4 +74,34 @@ ssize_t WriteFD(int fd, const void* buffer, size_t size) { return ReadOrWrite(fd, buffer, size); } +void CheckedReadFD(int fd, void* buffer, size_t size) { + ssize_t expect = base::checked_cast(size); + ssize_t rv = ReadFD(fd, buffer, size); + if (rv < 0) { + PCHECK(rv == expect) << "read"; + } else { + CHECK_EQ(rv, expect) << "read"; + } +} + +void CheckedWriteFD(int fd, const void* buffer, size_t size) { + ssize_t expect = base::checked_cast(size); + ssize_t rv = WriteFD(fd, buffer, size); + if (rv < 0) { + PCHECK(rv == expect) << "write"; + } else { + CHECK_EQ(rv, expect) << "write"; + } +} + +void CheckedReadFDAtEOF(int fd) { + char c; + ssize_t rv = ReadFD(fd, &c, 1); + if (rv < 0) { + PCHECK(rv == 0) << "read"; + } else { + CHECK_EQ(rv, 0) << "read"; + } +} + } // namespace crashpad diff --git a/util/file/fd_io.h b/util/file/fd_io.h index 07f39b6d..14481c93 100644 --- a/util/file/fd_io.h +++ b/util/file/fd_io.h @@ -30,6 +30,8 @@ namespace crashpad { //! have been read into \a buffer. //! //! \sa WriteFD +//! \sa CheckedReadFD +//! \sa CheckedReadFDAtEOF ssize_t ReadFD(int fd, void* buffer, size_t size); //! \brief Wraps `write()`, retrying when interrupted or following a short @@ -43,8 +45,40 @@ ssize_t ReadFD(int fd, void* buffer, size_t size); //! been written to \a fd. //! //! \sa ReadFD +//! \sa CheckedWriteFD ssize_t WriteFD(int fd, const void* buffer, size_t size); +//! \brief Wraps ReadFD(), ensuring that exactly \a size bytes are read. +//! +//! If \a size is out of the range of possible `read()` return values, if the +//! underlying ReadFD() fails, or if other than \a size bytes were read, this +//! function causes execution to terminate without returning. +//! +//! \sa CheckedWriteFD +//! \sa ReadFD +//! \sa CheckedReadFDAtEOF +void CheckedReadFD(int fd, void* buffer, size_t size); + +//! \brief Wraps WriteFD(), ensuring that exactly \a size bytes are written. +//! +//! If \a size is out of the range of possible `write()` return values, if the +//! underlying WriteFD() fails, or if other than \a size bytes were written, +//! this function causes execution to terminate without returning. +//! +//! \sa CheckedReadFD +//! \sa WriteFD +void CheckedWriteFD(int fd, const void* buffer, size_t size); + +//! \brief Wraps ReadFD(), ensuring that it indicates end-of-file. +//! +//! Attempts to read a single byte from \a fd, expecting no data to be read. If +//! the underlying ReadFD() fails, or if a byte actually is read, this function +//! causes execution to terminate without returning. +//! +//! \sa CheckedReadFD +//! \sa ReadFD +void CheckedReadFDAtEOF(int fd); + } // namespace crashpad #endif // CRASHPAD_UTIL_FILE_FD_IO_H_ diff --git a/util/mac/process_reader_test.cc b/util/mac/process_reader_test.cc index d6726738..9ed6c441 100644 --- a/util/mac/process_reader_test.cc +++ b/util/mac/process_reader_test.cc @@ -91,20 +91,11 @@ class ProcessReaderChild final : public MachMultiprocess { int read_fd = ReadPipeFD(); mach_vm_address_t address; - ssize_t rv = ReadFD(read_fd, &address, sizeof(address)); - ASSERT_EQ(static_cast(sizeof(address)), rv) - << ErrnoMessage("read"); + CheckedReadFD(read_fd, &address, sizeof(address)); std::string read_string; ASSERT_TRUE(process_reader.Memory()->ReadCString(address, &read_string)); EXPECT_EQ(kTestMemory, read_string); - - // Tell the child that it’s OK to exit. The child needed to be kept alive - // until the parent finished working with it. - int write_fd = WritePipeFD(); - char c = '\0'; - rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); } void MachMultiprocessChild() override { @@ -112,15 +103,11 @@ class ProcessReaderChild final : public MachMultiprocess { mach_vm_address_t address = reinterpret_cast(kTestMemory); - ssize_t rv = WriteFD(write_fd, &address, sizeof(address)); - ASSERT_EQ(static_cast(sizeof(address)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &address, sizeof(address)); - // Wait for the parent to say that it’s OK to exit. - int read_fd = ReadPipeFD(); - char c; - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + // Wait for the parent to signal that it’s OK to exit by closing its end of + // the pipe. + CheckedReadFDAtEOF(ReadPipeFD()); } DISALLOW_COPY_AND_ASSIGN(ProcessReaderChild); @@ -455,22 +442,15 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { thread_index < thread_count_ + 1; ++thread_index) { uint64_t thread_id; - ssize_t rv = ReadFD(read_fd, &thread_id, sizeof(thread_id)); - ASSERT_EQ(static_cast(sizeof(thread_id)), rv) - << ErrnoMessage("read"); + CheckedReadFD(read_fd, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; - rv = ReadFD(read_fd, - &expectation.stack_address, - sizeof(expectation.stack_address)); - ASSERT_EQ(static_cast(sizeof(expectation.stack_address)), rv) - << ErrnoMessage("read"); - - rv = ReadFD(read_fd, - &expectation.suspend_count, - sizeof(expectation.suspend_count)); - ASSERT_EQ(static_cast(sizeof(expectation.suspend_count)), rv) - << ErrnoMessage("read"); + CheckedReadFD(read_fd, + &expectation.stack_address, + sizeof(expectation.stack_address)); + CheckedReadFD(read_fd, + &expectation.suspend_count, + sizeof(expectation.suspend_count)); // There can’t be any duplicate thread IDs. EXPECT_EQ(0u, thread_map.count(thread_id)); @@ -483,13 +463,6 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { // The child shouldn’t have any threads other than its main thread and the // ones it created in its pool, so pass false for |tolerate_extra_threads|. ExpectSeveralThreads(&thread_map, threads, false); - - // Tell the child that it’s OK to exit. The child needed to be kept alive - // until the parent finished working with it. - int write_fd = WritePipeFD(); - char c = '\0'; - ssize_t rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); } void MachMultiprocessChild() override { @@ -505,25 +478,18 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { // to inspect it. Write an entry for it. uint64_t thread_id = PthreadToThreadID(pthread_self()); - ssize_t rv = WriteFD(write_fd, &thread_id, sizeof(thread_id)); - ASSERT_EQ(static_cast(sizeof(thread_id)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; expectation.stack_address = reinterpret_cast(&thread_id); expectation.suspend_count = 0; - rv = WriteFD(write_fd, - &expectation.stack_address, - sizeof(expectation.stack_address)); - ASSERT_EQ(static_cast(sizeof(expectation.stack_address)), rv) - << ErrnoMessage("write"); - - rv = WriteFD(write_fd, - &expectation.suspend_count, - sizeof(expectation.suspend_count)); - ASSERT_EQ(static_cast(sizeof(expectation.suspend_count)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, + &expectation.stack_address, + sizeof(expectation.stack_address)); + CheckedWriteFD(write_fd, + &expectation.suspend_count, + sizeof(expectation.suspend_count)); // Write an entry for everything in the thread pool. for (size_t thread_index = 0; @@ -532,28 +498,18 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { uint64_t thread_id = thread_pool.GetThreadInfo(thread_index, &expectation); - rv = WriteFD(write_fd, &thread_id, sizeof(thread_id)); - ASSERT_EQ(static_cast(sizeof(thread_id)), rv) - << ErrnoMessage("write"); - - rv = WriteFD(write_fd, - &expectation.stack_address, - sizeof(expectation.stack_address)); - ASSERT_EQ(static_cast(sizeof(expectation.stack_address)), rv) - << ErrnoMessage("write"); - - rv = WriteFD(write_fd, - &expectation.suspend_count, - sizeof(expectation.suspend_count)); - ASSERT_EQ(static_cast(sizeof(expectation.suspend_count)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &thread_id, sizeof(thread_id)); + CheckedWriteFD(write_fd, + &expectation.stack_address, + sizeof(expectation.stack_address)); + CheckedWriteFD(write_fd, + &expectation.suspend_count, + sizeof(expectation.suspend_count)); } - // Wait for the parent to say that it’s OK to exit. - int read_fd = ReadPipeFD(); - char c; - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + // Wait for the parent to signal that it’s OK to exit by closing its end of + // the pipe. + CheckedReadFDAtEOF(ReadPipeFD()); } size_t thread_count_; @@ -650,9 +606,7 @@ class ProcessReaderModulesChild final : public MachMultiprocess { int read_fd = ReadPipeFD(); uint32_t expect_modules; - ssize_t rv = ReadFD(read_fd, &expect_modules, sizeof(expect_modules)); - ASSERT_EQ(static_cast(sizeof(expect_modules)), rv) - << ErrnoMessage("read"); + CheckedReadFD(read_fd, &expect_modules, sizeof(expect_modules)); ASSERT_EQ(expect_modules, modules.size()); @@ -661,24 +615,16 @@ class ProcessReaderModulesChild final : public MachMultiprocess { "index %zu, name %s", index, modules[index].name.c_str())); uint32_t expect_name_length; - rv = ReadFD( + CheckedReadFD( read_fd, &expect_name_length, sizeof(expect_name_length)); - ASSERT_EQ(static_cast(sizeof(expect_name_length)), rv) - << ErrnoMessage("read"); // The NUL terminator is not read. std::string expect_name(expect_name_length, '\0'); - rv = ReadFD(read_fd, &expect_name[0], expect_name_length); - ASSERT_EQ(static_cast(expect_name_length), rv) - << ErrnoMessage("read"); - + CheckedReadFD(read_fd, &expect_name[0], expect_name_length); EXPECT_EQ(expect_name, modules[index].name); mach_vm_address_t expect_address; - rv = ReadFD(read_fd, &expect_address, sizeof(expect_address)); - ASSERT_EQ(static_cast(sizeof(expect_address)), rv) - << ErrnoMessage("read"); - + CheckedReadFD(read_fd, &expect_address, sizeof(expect_address)); EXPECT_EQ(expect_address, modules[index].address); if (index == 0 || index == modules.size() - 1) { @@ -695,13 +641,6 @@ class ProcessReaderModulesChild final : public MachMultiprocess { } } } - - // Tell the child that it’s OK to exit. The child needed to be kept alive - // until the parent finished working with it. - int write_fd = WritePipeFD(); - char c = '\0'; - rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); } void MachMultiprocessChild() override { @@ -718,10 +657,7 @@ class ProcessReaderModulesChild final : public MachMultiprocess { ++write_image_count; } - ssize_t rv = WriteFD( - write_fd, &write_image_count, sizeof(write_image_count)); - ASSERT_EQ(static_cast(sizeof(write_image_count)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &write_image_count, sizeof(write_image_count)); for (size_t index = 0; index < write_image_count; ++index) { const char* dyld_image_name; @@ -738,26 +674,18 @@ class ProcessReaderModulesChild final : public MachMultiprocess { } uint32_t dyld_image_name_length = strlen(dyld_image_name); - rv = WriteFD( + CheckedWriteFD( write_fd, &dyld_image_name_length, sizeof(dyld_image_name_length)); - ASSERT_EQ(static_cast(sizeof(dyld_image_name_length)), rv) - << ErrnoMessage("write"); // The NUL terminator is not written. - rv = WriteFD(write_fd, dyld_image_name, dyld_image_name_length); - ASSERT_EQ(static_cast(dyld_image_name_length), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, dyld_image_name, dyld_image_name_length); - rv = WriteFD(write_fd, &dyld_image_address, sizeof(dyld_image_address)); - ASSERT_EQ(static_cast(sizeof(dyld_image_address)), rv) - << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &dyld_image_address, sizeof(dyld_image_address)); } - // Wait for the parent to say that it’s OK to exit. - int read_fd = ReadPipeFD(); - char c; - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + // Wait for the parent to signal that it’s OK to exit by closing its end of + // the pipe. + CheckedReadFDAtEOF(ReadPipeFD()); } DISALLOW_COPY_AND_ASSIGN(ProcessReaderModulesChild); diff --git a/util/mach/exception_ports_test.cc b/util/mach/exception_ports_test.cc index 4164ca58..8a459f40 100644 --- a/util/mach/exception_ports_test.cc +++ b/util/mach/exception_ports_test.cc @@ -30,7 +30,6 @@ #include "util/mach/exc_server_variants.h" #include "util/mach/mach_extensions.h" #include "util/misc/scoped_forbid_return.h" -#include "util/test/errors.h" #include "util/test/mac/mach_errors.h" #include "util/test/mac/mach_multiprocess.h" @@ -246,12 +245,10 @@ class TestExceptionPorts : public UniversalMachExcServer, // Tell the parent process that everything is set up. char c = '\0'; - ssize_t rv_ssize = WriteFD(test_exception_ports_->WritePipeFD(), &c, 1); - ASSERT_EQ(1, rv_ssize) << ErrnoMessage("write"); + CheckedWriteFD(test_exception_ports_->WritePipeFD(), &c, 1); // Wait for the parent process to say that its end is set up. - rv_ssize = ReadFD(test_exception_ports_->ReadPipeFD(), &c, 1); - ASSERT_EQ(1, rv_ssize) << ErrnoMessage("read"); + CheckedReadFD(test_exception_ports_->ReadPipeFD(), &c, 1); EXPECT_EQ('\0', c); // Regardless of where ExceptionPorts::SetExceptionPort() ran, @@ -367,8 +364,7 @@ class TestExceptionPorts : public UniversalMachExcServer, // Wait for the child process to be ready. It needs to have all of its // threads set up before proceeding if in kSetOutOfProcess mode. char c; - ssize_t rv = ReadFD(ReadPipeFD(), &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(ReadPipeFD(), &c, 1); EXPECT_EQ('\0', c); mach_port_t local_port = LocalPort(); @@ -451,8 +447,7 @@ class TestExceptionPorts : public UniversalMachExcServer, // Let the child process know that everything in the parent process is set // up. c = '\0'; - rv = WriteFD(WritePipeFD(), &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(WritePipeFD(), &c, 1); if (who_crashes_ != kNobodyCrashes) { kern_return_t kr = MachMessageServer::Run(this, @@ -470,8 +465,7 @@ class TestExceptionPorts : public UniversalMachExcServer, // Wait for the child process to exit or terminate, as indicated by it // closing its pipe. This keeps LocalPort() alive in the child as // RemotePort(), for the child’s use in its TestGetExceptionPorts(). - rv = ReadFD(ReadPipeFD(), &c, 1); - ASSERT_EQ(0, rv); + CheckedReadFDAtEOF(ReadPipeFD()); } virtual void MachMultiprocessChild() override { diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index cf2b22de..71eaf20c 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -22,7 +22,6 @@ #include "gtest/gtest.h" #include "util/file/fd_io.h" #include "util/mach/mach_extensions.h" -#include "util/test/errors.h" #include "util/test/mac/mach_errors.h" #include "util/test/mac/mach_multiprocess.h" @@ -301,8 +300,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.parent_wait_for_child_pipe) { // Wait until the child is done sending what it’s going to send. char c; - ssize_t rv = ReadFD(ReadPipeFD(), &c, 1); - EXPECT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(ReadPipeFD(), &c, 1); EXPECT_EQ('\0', c); } @@ -348,8 +346,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe) { // Let the child know it’s safe to exit. char c = '\0'; - ssize_t rv = WriteFD(WritePipeFD(), &c, 1); - EXPECT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(WritePipeFD(), &c, 1); } } @@ -393,8 +390,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe) { char c; - ssize_t rv = ReadFD(ReadPipeFD(), &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(ReadPipeFD(), &c, 1); ASSERT_EQ('\0', c); } } @@ -522,8 +518,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, // running MachMessageServer() once it’s received. void ChildNotifyParentViaPipe() { char c = '\0'; - ssize_t rv = WriteFD(WritePipeFD(), &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(WritePipeFD(), &c, 1); } // In the child process, sends a request message to the server and then diff --git a/util/test/multiprocess_exec_test.cc b/util/test/multiprocess_exec_test.cc index 0edbbcf0..2f30b9b0 100644 --- a/util/test/multiprocess_exec_test.cc +++ b/util/test/multiprocess_exec_test.cc @@ -19,7 +19,6 @@ #include "base/basictypes.h" #include "gtest/gtest.h" #include "util/file/fd_io.h" -#include "util/test/errors.h" #include "util/test/executable_path.h" namespace { @@ -35,14 +34,10 @@ class TestMultiprocessExec final : public MultiprocessExec { private: virtual void MultiprocessParent() override { - int write_fd = WritePipeFD(); char c = 'z'; - ssize_t rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(WritePipeFD(), &c, 1); - int read_fd = ReadPipeFD(); - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(ReadPipeFD(), &c, 1); EXPECT_EQ('Z', c); } diff --git a/util/test/multiprocess_test.cc b/util/test/multiprocess_test.cc index 167f6475..aab0181b 100644 --- a/util/test/multiprocess_test.cc +++ b/util/test/multiprocess_test.cc @@ -21,7 +21,6 @@ #include "base/basictypes.h" #include "gtest/gtest.h" #include "util/file/fd_io.h" -#include "util/test/errors.h" namespace { @@ -38,40 +37,31 @@ class TestMultiprocess final : public Multiprocess { virtual void MultiprocessParent() override { int read_fd = ReadPipeFD(); char c; - ssize_t rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(read_fd, &c, 1); EXPECT_EQ('M', c); pid_t pid; - rv = ReadFD(read_fd, &pid, sizeof(pid)); - ASSERT_EQ(static_cast(sizeof(pid)), rv) << ErrnoMessage("read"); + CheckedReadFD(read_fd, &pid, sizeof(pid)); EXPECT_EQ(pid, ChildPID()); - int write_fd = WritePipeFD(); c = 'm'; - rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(WritePipeFD(), &c, 1); // The child will close its end of the pipe and exit. Make sure that the // parent sees EOF. - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(0, rv) << ErrnoMessage("read"); + CheckedReadFDAtEOF(read_fd); } virtual void MultiprocessChild() override { int write_fd = WritePipeFD(); char c = 'M'; - ssize_t rv = WriteFD(write_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &c, 1); pid_t pid = getpid(); - rv = WriteFD(write_fd, &pid, sizeof(pid)); - ASSERT_EQ(static_cast(sizeof(pid)), rv) << ErrnoMessage("write"); + CheckedWriteFD(write_fd, &pid, sizeof(pid)); - int read_fd = ReadPipeFD(); - rv = ReadFD(read_fd, &c, 1); - ASSERT_EQ(1, rv) << ErrnoMessage("read"); + CheckedReadFD(ReadPipeFD(), &c, 1); EXPECT_EQ('m', c); }