From 00b64427523b2413a0f2711dcf2f9caf0c6d6c94 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 16 Mar 2017 13:36:38 -0400 Subject: [PATCH] Make file_io reads more rational and predictable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReadFile() attempted to continue reading after a short read. In most cases, this is fine. However, ReadFile() would keep trying to fill a partially-filled buffer until experiencing a 0-length read(), signaling end-of-file. For certain weird file descriptors like terminal input, EOF is an ephemeral condition, and attempting to read beyond EOF doesn’t actually return 0 (EOF) provided that they remain open, it will block waiting for more input. Consequently, ReadFile() and anything based on ReadFile() had an undocumented and quirky interface, which was that any short read that it returned (not an underlying short read) actually indicated EOF. This facet of ReadFile() was unexpected, so it’s being removed. The new behavior is that ReadFile() will return an underlying short read. The behavior of FileReaderInterface::Read() is updated in accordance with this change. Upon experiencing a short read, the caller can determine the best action. Most callers were already prepared for this behavior. Outside of util/file, only crashpad_database_util properly implemented EOF detection according to previous semantics, and adapting it to new semantics is trivial. Callers who require an exact-length read can use the new ReadFileExactly(), or the newly renamed LoggingReadFileExactly() or CheckedReadFileExactly(). These functions will retry following a short read. The renamed functions were previously called LoggingReadFile() and CheckedReadFile(), but those names implied that they were simply wrapping ReadFile(), which is not the case. They wrapped ReadFile() and further, insisted on a full read. Since ReadFile()’s semantics are now changing but these functions’ are not, they’re now even more distinct from ReadFile(), and must be renamed to avoid confusion. Test: * Change-Id: I06b77e0d6ad8719bd2eb67dab93a8740542dd908 Reviewed-on: https://chromium-review.googlesource.com/456676 Reviewed-by: Robert Sesek --- client/crash_report_database_win.cc | 8 +- client/settings.cc | 11 +-- handler/win/crash_other_program.cc | 3 +- snapshot/api/module_annotations_win_test.cc | 4 +- .../mach_o_image_annotations_reader_test.cc | 4 +- snapshot/mac/process_reader_test.cc | 26 +++--- ...shpad_snapshot_test_extra_memory_ranges.cc | 2 +- ...ashpad_snapshot_test_simple_annotations.cc | 2 +- snapshot/win/exception_snapshot_win_test.cc | 12 +-- snapshot/win/extra_memory_ranges_test.cc | 2 +- .../win/pe_image_annotations_reader_test.cc | 2 +- snapshot/win/process_reader_win_test.cc | 4 +- snapshot/win/process_snapshot_win_test.cc | 3 +- test/multiprocess_exec_test.cc | 2 +- test/multiprocess_posix_test.cc | 6 +- test/win/win_child_process.cc | 2 +- tools/crashpad_database_util.cc | 2 +- util/file/file_io.cc | 59 ++++++++++--- util/file/file_io.h | 82 +++++++++++++------ util/file/file_io_posix.cc | 82 ++++++++----------- util/file/file_io_win.cc | 35 ++++---- util/file/file_reader.cc | 35 +++++--- util/file/file_reader.h | 2 +- util/mach/child_port_handshake.cc | 8 +- util/mach/exception_ports_test.cc | 4 +- util/mach/mach_message_server_test.cc | 6 +- util/net/http_transport_test.cc | 2 +- util/win/exception_handler_server.cc | 3 +- util/win/exception_handler_server_test.cc | 5 +- util/win/process_info_test.cc | 2 +- util/win/scoped_process_suspend_test.cc | 2 +- 31 files changed, 246 insertions(+), 176 deletions(-) diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 4fccd3fd..6629d25d 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -77,8 +77,8 @@ std::string ReadRestOfFileAsString(FileHandle file) { DCHECK_GT(end, read_from); size_t data_length = static_cast(end - read_from); std::string buffer(data_length, '\0'); - return LoggingReadFile(file, &buffer[0], data_length) ? buffer - : std::string(); + return LoggingReadFileExactly(file, &buffer[0], data_length) ? buffer + : std::string(); } // Helper structures, and conversions ------------------------------------------ @@ -417,7 +417,7 @@ void Metadata::Read() { } MetadataFileHeader header; - if (!LoggingReadFile(handle_.get(), &header, sizeof(header))) { + if (!LoggingReadFileExactly(handle_.get(), &header, sizeof(header))) { LOG(ERROR) << "failed to read header"; return; } @@ -438,7 +438,7 @@ void Metadata::Read() { std::vector reports; if (header.num_records > 0) { std::vector records(header.num_records); - if (!LoggingReadFile( + if (!LoggingReadFileExactly( handle_.get(), &records[0], records_size.ValueOrDie())) { LOG(ERROR) << "failed to read records"; return; diff --git a/client/settings.cc b/client/settings.cc index 7757ecb0..15d16f2e 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -246,13 +246,10 @@ bool Settings::ReadSettings(FileHandle handle, if (LoggingSeekFile(handle, 0, SEEK_SET) != 0) return false; - bool read_result; - if (log_read_error) { - read_result = LoggingReadFile(handle, out_data, sizeof(*out_data)); - } else { - read_result = - ReadFile(handle, out_data, sizeof(*out_data)) == sizeof(*out_data); - } + bool read_result = + log_read_error + ? LoggingReadFileExactly(handle, out_data, sizeof(*out_data)) + : ReadFileExactly(handle, out_data, sizeof(*out_data)); if (!read_result) return false; diff --git a/handler/win/crash_other_program.cc b/handler/win/crash_other_program.cc index d191aac0..012b4ba3 100644 --- a/handler/win/crash_other_program.cc +++ b/handler/win/crash_other_program.cc @@ -90,7 +90,8 @@ int CrashOtherProgram(int argc, wchar_t* argv[]) { // Wait until it's ready. char c; - if (!LoggingReadFile(child.stdout_read_handle(), &c, sizeof(c)) || c != ' ') { + if (!LoggingReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)) || + c != ' ') { LOG(ERROR) << "failed child communication"; return EXIT_FAILURE; } diff --git a/snapshot/api/module_annotations_win_test.cc b/snapshot/api/module_annotations_win_test.cc index 30e880f3..ee028683 100644 --- a/snapshot/api/module_annotations_win_test.cc +++ b/snapshot/api/module_annotations_win_test.cc @@ -28,7 +28,7 @@ class ModuleAnnotationsMultiprocessTest final : public WinMultiprocess { void WinMultiprocessParent() override { // Read the child executable module. HMODULE module = nullptr; - CheckedReadFile(ReadPipeHandle(), &module, sizeof(module)); + CheckedReadFileExactly(ReadPipeHandle(), &module, sizeof(module)); // Reopen the child process with necessary access. HANDLE process_handle = @@ -70,7 +70,7 @@ class ModuleAnnotationsMultiprocessTest final : public WinMultiprocess { // Wait until a signal from the parent process to terminate. char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); } }; diff --git a/snapshot/mac/mach_o_image_annotations_reader_test.cc b/snapshot/mac/mach_o_image_annotations_reader_test.cc index 7a94eb2c..50e646ae 100644 --- a/snapshot/mac/mach_o_image_annotations_reader_test.cc +++ b/snapshot/mac/mach_o_image_annotations_reader_test.cc @@ -245,7 +245,7 @@ class TestMachOImageAnnotationsReader final // Wait for the child process to indicate that it’s done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); // Verify the “simple map” annotations set via the CrashpadInfo interface. const std::vector& modules = @@ -333,7 +333,7 @@ class TestMachOImageAnnotationsReader final CheckedWriteFile(WritePipeHandle(), &c, sizeof(c)); // Wait for the parent to indicate that it’s safe to crash. - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); // Direct an exception message to the exception server running in the // parent. diff --git a/snapshot/mac/process_reader_test.cc b/snapshot/mac/process_reader_test.cc index e679d2ef..87fb6394 100644 --- a/snapshot/mac/process_reader_test.cc +++ b/snapshot/mac/process_reader_test.cc @@ -105,7 +105,7 @@ class ProcessReaderChild final : public MachMultiprocess { FileHandle read_handle = ReadPipeHandle(); mach_vm_address_t address; - CheckedReadFile(read_handle, &address, sizeof(address)); + CheckedReadFileExactly(read_handle, &address, sizeof(address)); std::string read_string; ASSERT_TRUE(process_reader.Memory()->ReadCString(address, &read_string)); @@ -448,15 +448,15 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { thread_index < thread_count_ + 1; ++thread_index) { uint64_t thread_id; - CheckedReadFile(read_handle, &thread_id, sizeof(thread_id)); + CheckedReadFileExactly(read_handle, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; - CheckedReadFile(read_handle, - &expectation.stack_address, - sizeof(expectation.stack_address)); - CheckedReadFile(read_handle, - &expectation.suspend_count, - sizeof(expectation.suspend_count)); + CheckedReadFileExactly(read_handle, + &expectation.stack_address, + sizeof(expectation.stack_address)); + CheckedReadFileExactly(read_handle, + &expectation.suspend_count, + sizeof(expectation.suspend_count)); // There can’t be any duplicate thread IDs. EXPECT_EQ(0u, thread_map.count(thread_id)); @@ -730,7 +730,8 @@ class ProcessReaderModulesChild final : public MachMultiprocess { FileHandle read_handle = ReadPipeHandle(); uint32_t expect_modules; - CheckedReadFile(read_handle, &expect_modules, sizeof(expect_modules)); + CheckedReadFileExactly( + read_handle, &expect_modules, sizeof(expect_modules)); ASSERT_EQ(expect_modules, modules.size()); @@ -740,16 +741,17 @@ class ProcessReaderModulesChild final : public MachMultiprocess { "index %zu, name %s", index, modules[index].name.c_str())); uint32_t expect_name_length; - CheckedReadFile( + CheckedReadFileExactly( read_handle, &expect_name_length, sizeof(expect_name_length)); // The NUL terminator is not read. std::string expect_name(expect_name_length, '\0'); - CheckedReadFile(read_handle, &expect_name[0], expect_name_length); + CheckedReadFileExactly(read_handle, &expect_name[0], expect_name_length); EXPECT_EQ(expect_name, modules[index].name); mach_vm_address_t expect_address; - CheckedReadFile(read_handle, &expect_address, sizeof(expect_address)); + CheckedReadFileExactly( + read_handle, &expect_address, sizeof(expect_address)); ASSERT_TRUE(modules[index].reader); EXPECT_EQ(expect_address, modules[index].reader->Address()); diff --git a/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc b/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc index 58b145e5..e6875fdd 100644 --- a/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc +++ b/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc @@ -43,7 +43,7 @@ int wmain(int argc, wchar_t* argv[]) { HANDLE in = GetStdHandle(STD_INPUT_HANDLE); PCHECK(in != INVALID_HANDLE_VALUE) << "GetStdHandle"; - CheckedReadFile(in, &c, sizeof(c)); + CheckedReadFileExactly(in, &c, sizeof(c)); CHECK(c == 'd' || c == ' '); // If 'd' we crash with a debug break, otherwise exit normally. diff --git a/snapshot/win/crashpad_snapshot_test_simple_annotations.cc b/snapshot/win/crashpad_snapshot_test_simple_annotations.cc index b66a19e1..11e7b4e7 100644 --- a/snapshot/win/crashpad_snapshot_test_simple_annotations.cc +++ b/snapshot/win/crashpad_snapshot_test_simple_annotations.cc @@ -42,7 +42,7 @@ int wmain(int argc, wchar_t* argv[]) { HANDLE in = GetStdHandle(STD_INPUT_HANDLE); PCHECK(in != INVALID_HANDLE_VALUE) << "GetStdHandle"; - crashpad::CheckedReadFile(in, &c, sizeof(c)); + crashpad::CheckedReadFileExactly(in, &c, sizeof(c)); CHECK(c == 'd' || c == ' '); // If 'd' we crash with a debug break, otherwise exit normally. diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index af0cc3a6..03aa0499 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -148,9 +148,9 @@ void TestCrashingChild(const base::string16& directory_modification) { // The child tells us (approximately) where it will crash. WinVMAddress break_near_address; - LoggingReadFile(child.stdout_read_handle(), - &break_near_address, - sizeof(break_near_address)); + LoggingReadFileExactly(child.stdout_read_handle(), + &break_near_address, + sizeof(break_near_address)); delegate.set_break_near(break_near_address); // Wait for the child to crash and the exception information to be validated. @@ -250,9 +250,9 @@ void TestDumpWithoutCrashingChild( // The child tells us (approximately) where it will capture a dump. WinVMAddress dump_near_address; - LoggingReadFile(child.stdout_read_handle(), - &dump_near_address, - sizeof(dump_near_address)); + LoggingReadFileExactly(child.stdout_read_handle(), + &dump_near_address, + sizeof(dump_near_address)); delegate.set_dump_near(dump_near_address); // Wait for the child to crash and the exception information to be validated. diff --git a/snapshot/win/extra_memory_ranges_test.cc b/snapshot/win/extra_memory_ranges_test.cc index d6d56596..57d41bb5 100644 --- a/snapshot/win/extra_memory_ranges_test.cc +++ b/snapshot/win/extra_memory_ranges_test.cc @@ -58,7 +58,7 @@ void TestExtraMemoryRanges(TestType type, // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(child.stdout_read_handle(), &c, sizeof(c)); + CheckedReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)); ProcessSnapshotWin snapshot; ASSERT_TRUE(snapshot.Initialize( diff --git a/snapshot/win/pe_image_annotations_reader_test.cc b/snapshot/win/pe_image_annotations_reader_test.cc index 523a4494..f791f80e 100644 --- a/snapshot/win/pe_image_annotations_reader_test.cc +++ b/snapshot/win/pe_image_annotations_reader_test.cc @@ -62,7 +62,7 @@ void TestAnnotationsOnCrash(TestType type, // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(child.stdout_read_handle(), &c, sizeof(c)); + CheckedReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)); ProcessReaderWin process_reader; ASSERT_TRUE(process_reader.Initialize(child.process_handle(), diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index b9befc9a..af57c622 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -69,7 +69,7 @@ class ProcessReaderChild final : public WinMultiprocess { #endif WinVMAddress address; - CheckedReadFile(ReadPipeHandle(), &address, sizeof(address)); + CheckedReadFileExactly(ReadPipeHandle(), &address, sizeof(address)); char buffer[sizeof(kTestMemory)]; ASSERT_TRUE( @@ -142,7 +142,7 @@ class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { void WinMultiprocessParent() override { char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); ASSERT_EQ(' ', c); { diff --git a/snapshot/win/process_snapshot_win_test.cc b/snapshot/win/process_snapshot_win_test.cc index a3e31a4e..82b29035 100644 --- a/snapshot/win/process_snapshot_win_test.cc +++ b/snapshot/win/process_snapshot_win_test.cc @@ -48,7 +48,8 @@ void TestImageReaderChild(const base::string16& directory_modification) { child.Start(); char c; - ASSERT_TRUE(LoggingReadFile(child.stdout_read_handle(), &c, sizeof(c))); + ASSERT_TRUE( + LoggingReadFileExactly(child.stdout_read_handle(), &c, sizeof(c))); ASSERT_EQ(' ', c); ScopedProcessSuspend suspend(child.process_handle()); diff --git a/test/multiprocess_exec_test.cc b/test/multiprocess_exec_test.cc index f6231e9c..3e958aa8 100644 --- a/test/multiprocess_exec_test.cc +++ b/test/multiprocess_exec_test.cc @@ -39,7 +39,7 @@ class TestMultiprocessExec final : public MultiprocessExec { char c = 'z'; ASSERT_TRUE(LoggingWriteFile(WritePipeHandle(), &c, 1)); - ASSERT_TRUE(LoggingReadFile(ReadPipeHandle(), &c, 1)); + ASSERT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, 1)); EXPECT_EQ('Z', c); } diff --git a/test/multiprocess_posix_test.cc b/test/multiprocess_posix_test.cc index be927111..9f557fab 100644 --- a/test/multiprocess_posix_test.cc +++ b/test/multiprocess_posix_test.cc @@ -39,11 +39,11 @@ class TestMultiprocess final : public Multiprocess { void MultiprocessParent() override { FileHandle read_handle = ReadPipeHandle(); char c; - CheckedReadFile(read_handle, &c, 1); + CheckedReadFileExactly(read_handle, &c, 1); EXPECT_EQ('M', c); pid_t pid; - CheckedReadFile(read_handle, &pid, sizeof(pid)); + CheckedReadFileExactly(read_handle, &pid, sizeof(pid)); EXPECT_EQ(pid, ChildPID()); c = 'm'; @@ -63,7 +63,7 @@ class TestMultiprocess final : public Multiprocess { pid_t pid = getpid(); CheckedWriteFile(write_handle, &pid, sizeof(pid)); - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('m', c); } diff --git a/test/win/win_child_process.cc b/test/win/win_child_process.cc index 156aa6dc..06ab21b2 100644 --- a/test/win/win_child_process.cc +++ b/test/win/win_child_process.cc @@ -203,7 +203,7 @@ std::unique_ptr WinChildProcess::Launch() { // immediately, and test code expects process initialization to have // completed so it can, for example, read the process memory. char c; - if (!LoggingReadFile(handles_for_parent->read.get(), &c, sizeof(c))) { + if (!LoggingReadFileExactly(handles_for_parent->read.get(), &c, sizeof(c))) { ADD_FAILURE() << "LoggedReadFile"; return std::unique_ptr(); } diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index e7d89bc4..4a2dcc65 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -597,7 +597,7 @@ int DatabaseUtilMain(int argc, char* argv[]) { !LoggingWriteFile(new_report->handle, buf, read_result)) { return EXIT_FAILURE; } - } while (read_result == sizeof(buf)); + } while (read_result > 0); call_error_writing_crash_report.Disarm(); diff --git a/util/file/file_io.cc b/util/file/file_io.cc index 98eb3404..8d3e466c 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -19,38 +19,71 @@ namespace crashpad { -bool LoggingReadFile(FileHandle file, void* buffer, size_t size) { +namespace { + +bool ReadFileExactlyInternal(FileHandle file, + void* buffer, + size_t size, + bool can_log) { FileOperationResult expect = base::checked_cast(size); - FileOperationResult rv = ReadFile(file, buffer, size); - if (rv < 0) { - PLOG(ERROR) << "read"; - return false; + char* buffer_c = static_cast(buffer); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = ReadFile(file, buffer, size); + if (bytes < 0) { + PLOG_IF(ERROR, can_log) << kNativeReadFunctionName; + return false; + } + + DCHECK_LE(static_cast(bytes), size); + + if (bytes == 0) { + break; + } + + buffer_c += bytes; + size -= bytes; + total_bytes += bytes; } - if (rv != expect) { - LOG(ERROR) << "read: expected " << expect << ", observed " << rv; + + if (total_bytes != expect) { + LOG_IF(ERROR, can_log) << kNativeReadFunctionName << ": expected " << expect + << ", observed " << total_bytes; return false; } return true; } +} // namespace + +bool ReadFileExactly(FileHandle file, void* buffer, size_t size) { + return ReadFileExactlyInternal(file, buffer, size, false); +} + +bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size) { + return ReadFileExactlyInternal(file, buffer, size, true); +} + bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size) { FileOperationResult expect = base::checked_cast(size); FileOperationResult rv = WriteFile(file, buffer, size); if (rv < 0) { - PLOG(ERROR) << "write"; + PLOG(ERROR) << kNativeWriteFunctionName; return false; } if (rv != expect) { - LOG(ERROR) << "write: expected " << expect << ", observed " << rv; + LOG(ERROR) << kNativeWriteFunctionName << ": expected " << expect + << ", observed " << rv; return false; } return true; } -void CheckedReadFile(FileHandle file, void* buffer, size_t size) { - CHECK(LoggingReadFile(file, buffer, size)); +void CheckedReadFileExactly(FileHandle file, void* buffer, size_t size) { + CHECK(LoggingReadFileExactly(file, buffer, size)); } void CheckedWriteFile(FileHandle file, const void* buffer, size_t size) { @@ -61,9 +94,9 @@ void CheckedReadFileAtEOF(FileHandle file) { char c; FileOperationResult rv = ReadFile(file, &c, 1); if (rv < 0) { - PCHECK(rv == 0) << "read"; + PCHECK(rv == 0) << kNativeReadFunctionName; } else { - CHECK_EQ(rv, 0) << "read"; + CHECK_EQ(rv, 0) << kNativeReadFunctionName; } } diff --git a/util/file/file_io.h b/util/file/file_io.h index ff20488d..a7415b8c 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -114,20 +114,33 @@ enum class FileLocking : bool { kExclusive, }; -//! \brief Reads from a file, retrying when interrupted on POSIX or following a -//! short read. +//! \brief The name of the native read function used by ReadFile(). //! -//! This function reads into \a buffer, stopping only when \a size bytes have -//! been read or when end-of-file has been reached. On Windows, reading from -//! sockets is not currently supported. +//! This value may be useful for logging. +//! +//! \sa kNativeWriteFunctionName +extern const char kNativeReadFunctionName[]; + +//! \brief The name of the native write function used by WriteFile(). +//! +//! This value may be useful for logging. +//! +//! \sa kNativeReadFunctionName +extern const char kNativeWriteFunctionName[]; + +//! \brief Reads from a file, retrying when interrupted on POSIX. +//! +//! This function reads into \a buffer. Fewer than \a size bytes may be read. +//! On Windows, reading from sockets is not currently supported. //! //! \return The number of bytes read and placed into \a buffer, or `-1` on //! error, with `errno` or `GetLastError()` set appropriately. On error, a //! portion of \a file may have been read into \a buffer. //! //! \sa WriteFile -//! \sa LoggingReadFile -//! \sa CheckedReadFile +//! \sa ReadFileExactly +//! \sa LoggingReadFileExactly +//! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); @@ -146,27 +159,50 @@ FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); //! \sa CheckedWriteFile FileOperationResult WriteFile(FileHandle file, const void* buffer, size_t size); -//! \brief Wraps ReadFile(), ensuring that exactly \a size bytes are read. +//! \brief Wraps ReadFile(), retrying following a short read, ensuring that +//! exactly \a size bytes are read. //! -//! \return `true` on success. If \a size is out of the range of possible -//! ReadFile() return values, if the underlying ReadFile() fails, or if -//! other than \a size bytes were read, this function logs a message and +//! If \a size is out of the range of possible ReadFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying ReadFile() fails, or if fewer +//! than \a size bytes were read, this function logs a message and //! returns `false`. //! //! \sa LoggingWriteFile //! \sa ReadFile -//! \sa CheckedReadFile +//! \sa LoggingReadFileExactly +//! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF -bool LoggingReadFile(FileHandle file, void* buffer, size_t size); +bool ReadFileExactly(FileHandle file, void* buffer, size_t size); + +//! \brief Wraps ReadFile(), retrying following a short read, ensuring that +//! exactly \a size bytes are read. +//! +//! If \a size is out of the range of possible ReadFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying ReadFile() fails, or if fewer +//! than \a size bytes were read, this function logs a message and +//! returns `false`. +//! +//! \sa LoggingWriteFile +//! \sa ReadFile +//! \sa ReadFileExactly +//! \sa CheckedReadFileExactly +//! \sa CheckedReadFileAtEOF +bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! -//! \return `true` on success. If \a size is out of the range of possible -//! WriteFile() return values, if the underlying WriteFile() fails, or if -//! other than \a size bytes were written, this function logs a message and +//! If \a size is out of the range of possible WriteFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying WriteFile() fails, or if fewer +//! than \a size bytes were written, this function logs a message and //! returns `false`. //! -//! \sa LoggingReadFile +//! \sa LoggingReadFileExactly //! \sa WriteFile //! \sa CheckedWriteFile bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size); @@ -174,22 +210,22 @@ bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size); //! \brief Wraps ReadFile(), ensuring that exactly \a size bytes are read. //! //! If \a size is out of the range of possible ReadFile() return values, if the -//! underlying ReadFile() fails, or if other than \a size bytes were read, this +//! underlying ReadFile() fails, or if fewer than \a size bytes were read, this //! function causes execution to terminate without returning. //! //! \sa CheckedWriteFile //! \sa ReadFile -//! \sa LoggingReadFile +//! \sa LoggingReadFileExactly //! \sa CheckedReadFileAtEOF -void CheckedReadFile(FileHandle file, void* buffer, size_t size); +void CheckedReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! //! If \a size is out of the range of possible WriteFile() return values, if the -//! underlying WriteFile() fails, or if other than \a size bytes were written, +//! underlying WriteFile() fails, or if fewer than \a size bytes were written, //! this function causes execution to terminate without returning. //! -//! \sa CheckedReadFile +//! \sa CheckedReadFileExactly //! \sa WriteFile //! \sa LoggingWriteFile void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); @@ -200,7 +236,7 @@ void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); //! If the underlying ReadFile() fails, or if a byte actually is read, this //! function causes execution to terminate without returning. //! -//! \sa CheckedReadFile +//! \sa CheckedReadFileExactly //! \sa ReadFile void CheckedReadFileAtEOF(FileHandle file); diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 1534db15..00d14cd2 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -24,54 +24,6 @@ #include "base/numerics/safe_conversions.h" #include "base/posix/eintr_wrapper.h" -namespace { - -struct ReadTraits { - using VoidBufferType = void*; - using CharBufferType = char*; - static crashpad::FileOperationResult Operate(int fd, - CharBufferType buffer, - size_t size) { - return read(fd, buffer, size); - } -}; - -struct WriteTraits { - using VoidBufferType = const void*; - using CharBufferType = const char*; - static crashpad::FileOperationResult Operate(int fd, - CharBufferType buffer, - size_t size) { - return write(fd, buffer, size); - } -}; - -template -crashpad::FileOperationResult -ReadOrWrite(int fd, typename Traits::VoidBufferType buffer, size_t size) { - typename Traits::CharBufferType buffer_c = - reinterpret_cast(buffer); - - crashpad::FileOperationResult total_bytes = 0; - while (size > 0) { - crashpad::FileOperationResult bytes = - HANDLE_EINTR(Traits::Operate(fd, buffer_c, size)); - if (bytes < 0) { - return bytes; - } else if (bytes == 0) { - break; - } - - buffer_c += bytes; - size -= bytes; - total_bytes += bytes; - } - - return total_bytes; -} - -} // namespace - namespace crashpad { namespace { @@ -108,14 +60,44 @@ FileHandle OpenFileForOutput(int rdwr_or_wronly, } // namespace +const char kNativeReadFunctionName[] = "read"; +const char kNativeWriteFunctionName[] = "write"; + +// TODO(mark): Handle > ssize_t-sized reads and writes if necessary. The +// standard leaves this implementation-defined. Some systems return EINVAL in +// this case. ReadFile() and WriteFile() could enforce this behavior. + FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { - return ReadOrWrite(file, buffer, size); + FileOperationResult bytes = HANDLE_EINTR(read(file, buffer, size)); + if (bytes < 0) { + return -1; + } + + DCHECK_LE(static_cast(bytes), size); + return bytes; } FileOperationResult WriteFile(FileHandle file, const void* buffer, size_t size) { - return ReadOrWrite(file, buffer, size); + const char* buffer_c = static_cast(buffer); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = HANDLE_EINTR(write(file, buffer_c, size)); + if (bytes < 0) { + return -1; + } + + DCHECK_NE(bytes, 0); + DCHECK_LE(static_cast(bytes), size); + + buffer_c += bytes; + size -= bytes; + total_bytes += bytes; + } + + return total_bytes; } FileHandle OpenFileForRead(const base::FilePath& path) { diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 5144869e..110a5c6a 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -70,37 +70,37 @@ FileHandle OpenFileForOutput(DWORD access, } // namespace -// TODO(scottmg): Handle > DWORD sized writes if necessary. +const char kNativeReadFunctionName[] = "ReadFile"; +const char kNativeWriteFunctionName[] = "WriteFile"; + +// TODO(scottmg): Handle > DWORD-sized reads and writes if necessary. FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { DCHECK(!IsSocketHandle(file)); - DWORD size_dword = base::checked_cast(size); - DWORD total_read = 0; - char* buffer_c = reinterpret_cast(buffer); - while (size_dword > 0) { + + while (true) { + DWORD size_dword = base::checked_cast(size); DWORD bytes_read; - BOOL success = ::ReadFile(file, buffer_c, size_dword, &bytes_read, nullptr); + BOOL success = ::ReadFile(file, buffer, size_dword, &bytes_read, nullptr); if (!success) { if (GetLastError() == ERROR_BROKEN_PIPE) { // When reading a pipe and the write handle has been closed, ReadFile // fails with ERROR_BROKEN_PIPE, but only once all pending data has been - // read. - break; - } else if (GetLastError() != ERROR_MORE_DATA) { - return -1; + // read. Treat this as EOF. + return 0; } - } else if (bytes_read == 0 && GetFileType(file) != FILE_TYPE_PIPE) { + return -1; + } + + CHECK_NE(bytes_read, static_cast(-1)); + DCHECK_LE(bytes_read, size_dword); + if (bytes_read != 0 || GetFileType(file) != FILE_TYPE_PIPE) { // Zero bytes read for a file indicates reaching EOF. Zero bytes read from // a pipe indicates only that there was a zero byte WriteFile issued on // the other end, so continue reading. - break; + return bytes_read; } - - buffer_c += bytes_read; - size_dword -= bytes_read; - total_read += bytes_read; } - return total_read; } FileOperationResult WriteFile(FileHandle file, @@ -113,6 +113,7 @@ FileOperationResult WriteFile(FileHandle file, BOOL rv = ::WriteFile(file, buffer, size_dword, &bytes_written, nullptr); if (!rv) return -1; + CHECK_NE(bytes_written, static_cast(-1)); CHECK_EQ(bytes_written, size_dword); return bytes_written; } diff --git a/util/file/file_reader.cc b/util/file/file_reader.cc index 4f5ae773..ac625b87 100644 --- a/util/file/file_reader.cc +++ b/util/file/file_reader.cc @@ -22,12 +22,30 @@ namespace crashpad { bool FileReaderInterface::ReadExactly(void* data, size_t size) { FileOperationResult expect = base::checked_cast(size); - FileOperationResult rv = Read(data, size); - if (rv < 0) { - // Read() will have logged its own error. - return false; - } else if (rv != expect) { - LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " << rv; + char* data_c = static_cast(data); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = Read(data, size); + if (bytes < 0) { + // Read() will have logged its own error. + return false; + } + + DCHECK_LE(static_cast(bytes), size); + + if (bytes == 0) { + break; + } + + data_c += bytes; + size -= bytes; + total_bytes += bytes; + } + + if (total_bytes != expect) { + LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " + << total_bytes; return false; } @@ -44,13 +62,10 @@ WeakFileHandleFileReader::~WeakFileHandleFileReader() { FileOperationResult WeakFileHandleFileReader::Read(void* data, size_t size) { DCHECK_NE(file_handle_, kInvalidFileHandle); - // Don’t use LoggingReadFile(), which insists on a full read and only returns - // a bool. This method permits short reads and returns the number of bytes - // read. base::checked_cast(size); FileOperationResult rv = ReadFile(file_handle_, data, size); if (rv < 0) { - PLOG(ERROR) << "read"; + PLOG(ERROR) << kNativeReadFunctionName; return -1; } diff --git a/util/file/file_reader.h b/util/file/file_reader.h index 39225e54..04724170 100644 --- a/util/file/file_reader.h +++ b/util/file/file_reader.h @@ -42,7 +42,7 @@ class FileReaderInterface : public virtual FileSeekerInterface { //! \brief Wraps Read(), ensuring that the read succeeded and exactly \a size //! bytes were read. //! - //! Semantically, this behaves as LoggingReadFile(). + //! Semantically, this behaves as LoggingReadFileExactly(). //! //! \return `true` if the operation succeeded, `false` if it failed, with an //! error message logged. Short reads are treated as failures. diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index 51aad905..e3624d70 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -403,20 +403,20 @@ bool ChildPortHandshake::RunClientInternal_ReadPipe(int client_read_fd, child_port_token_t* token, std::string* service_name) { // Read the token from the pipe. - if (!LoggingReadFile(client_read_fd, token, sizeof(*token))) { + if (!LoggingReadFileExactly(client_read_fd, token, sizeof(*token))) { return false; } // Read the service name from the pipe. uint32_t service_name_length; - if (!LoggingReadFile( - client_read_fd, &service_name_length, sizeof(service_name_length))) { + if (!LoggingReadFileExactly( + client_read_fd, &service_name_length, sizeof(service_name_length))) { return false; } service_name->resize(service_name_length); if (!service_name->empty() && - !LoggingReadFile( + !LoggingReadFileExactly( client_read_fd, &(*service_name)[0], service_name_length)) { return false; } diff --git a/util/mach/exception_ports_test.cc b/util/mach/exception_ports_test.cc index 37c7d118..f3d40029 100644 --- a/util/mach/exception_ports_test.cc +++ b/util/mach/exception_ports_test.cc @@ -244,7 +244,7 @@ class TestExceptionPorts : public MachMultiprocess, CheckedWriteFile(test_exception_ports_->WritePipeHandle(), &c, 1); // Wait for the parent process to say that its end is set up. - CheckedReadFile(test_exception_ports_->ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(test_exception_ports_->ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); // Regardless of where ExceptionPorts::SetExceptionPort() ran, @@ -352,7 +352,7 @@ class TestExceptionPorts : public MachMultiprocess, // 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; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); mach_port_t local_port = LocalPort(); diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index a2d2eb1f..2c4c8e1d 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -344,7 +344,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; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); } @@ -397,7 +397,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe_early) { // Wait until the parent is done setting things up on its end. char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); } @@ -431,7 +431,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe_late) { char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); ASSERT_EQ('\0', c); } } diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index 8dfa68a4..cb5c4f1f 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -81,7 +81,7 @@ class HTTPTransportTestFixture : public MultiprocessExec { // The child will write the HTTP server port number as a packed unsigned // short to stdout. uint16_t port; - ASSERT_TRUE(LoggingReadFile(ReadPipeHandle(), &port, sizeof(port))); + ASSERT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &port, sizeof(port))); // Then the parent will tell the web server what response code to send // for the HTTP request. diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 70955c82..fdc159c0 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -396,7 +396,8 @@ bool ExceptionHandlerServer::ServiceClientConnection( const internal::PipeServiceContext& service_context) { ClientToServerMessage message; - if (!LoggingReadFile(service_context.pipe(), &message, sizeof(message))) + if (!LoggingReadFileExactly( + service_context.pipe(), &message, sizeof(message))) return false; switch (message.type) { diff --git a/util/win/exception_handler_server_test.cc b/util/win/exception_handler_server_test.cc index e94b91d5..e9f865be 100644 --- a/util/win/exception_handler_server_test.cc +++ b/util/win/exception_handler_server_test.cc @@ -143,10 +143,11 @@ TEST_F(ExceptionHandlerServerTest, StopWhileConnected) { std::wstring ReadWString(FileHandle handle) { size_t length = 0; - EXPECT_TRUE(LoggingReadFile(handle, &length, sizeof(length))); + EXPECT_TRUE(LoggingReadFileExactly(handle, &length, sizeof(length))); std::wstring str(length, L'\0'); if (length > 0) { - EXPECT_TRUE(LoggingReadFile(handle, &str[0], length * sizeof(str[0]))); + EXPECT_TRUE( + LoggingReadFileExactly(handle, &str[0], length * sizeof(str[0]))); } return str; } diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index e377a6ee..dfa6564d 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -156,7 +156,7 @@ void TestOtherProcess(const base::string16& directory_modification) { // The child sends us a code address we can look up in the memory map. WinVMAddress code_address; - CheckedReadFile( + CheckedReadFileExactly( child.stdout_read_handle(), &code_address, sizeof(code_address)); ASSERT_TRUE(process_info.Initialize(child.process_handle())); diff --git a/util/win/scoped_process_suspend_test.cc b/util/win/scoped_process_suspend_test.cc index c968f710..adddfe27 100644 --- a/util/win/scoped_process_suspend_test.cc +++ b/util/win/scoped_process_suspend_test.cc @@ -80,7 +80,7 @@ class ScopedProcessSuspendTest final : public WinChildProcess { int Run() override { char c; // Wait for notification from parent. - EXPECT_TRUE(LoggingReadFile(ReadPipeHandle(), &c, sizeof(c))); + EXPECT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, sizeof(c))); EXPECT_EQ(' ', c); return EXIT_SUCCESS; }