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