Make file_io reads more rational and predictable

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 <rsesek@chromium.org>

client/crash_report_database_win.cc

@@ -77,7 +77,7 @@ std::string ReadRestOfFileAsString(FileHandle file) {
   DCHECK_GT(end, read_from);
   size_t data_length = static_cast<size_t>(end - read_from);
   std::string buffer(data_length, '\0');
-  return LoggingReadFile(file, &buffer[0], data_length) ? buffer
+  return LoggingReadFileExactly(file, &buffer[0], data_length) ? buffer
                                                                : std::string();
 }
 
@@ -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<ReportDisk> reports;
   if (header.num_records > 0) {
     std::vector<MetadataFileReportRecord> records(header.num_records);
-    if (!LoggingReadFile(
+    if (!LoggingReadFileExactly(
             handle_.get(), &records[0], records_size.ValueOrDie())) {
       LOG(ERROR) << "failed to read records";
       return;

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;

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

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

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<ProcessReader::Module>& 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.

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,13 +448,13 @@ 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,
+      CheckedReadFileExactly(read_handle,
                              &expectation.stack_address,
                              sizeof(expectation.stack_address));
-      CheckedReadFile(read_handle,
+      CheckedReadFileExactly(read_handle,
                              &expectation.suspend_count,
                              sizeof(expectation.suspend_count));
 
@@ -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());
 

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.

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.

snapshot/win/exception_snapshot_win_test.cc

@@ -148,7 +148,7 @@ 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(),
+  LoggingReadFileExactly(child.stdout_read_handle(),
                          &break_near_address,
                          sizeof(break_near_address));
   delegate.set_break_near(break_near_address);
@@ -250,7 +250,7 @@ void TestDumpWithoutCrashingChild(
 
   // The child tells us (approximately) where it will capture a dump.
   WinVMAddress dump_near_address;
-  LoggingReadFile(child.stdout_read_handle(),
+  LoggingReadFileExactly(child.stdout_read_handle(),
                          &dump_near_address,
                          sizeof(dump_near_address));
   delegate.set_dump_near(dump_near_address);

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(

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(),

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);
 
     {

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());

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

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

test/win/win_child_process.cc

@@ -203,7 +203,7 @@ std::unique_ptr<WinChildProcess::Handles> 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<Handles>();
   }

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();
 

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<FileOperationResult>(size);
-  FileOperationResult rv = ReadFile(file, buffer, size);
-  if (rv < 0) {
-    PLOG(ERROR) << "read";
+  char* buffer_c = static_cast<char*>(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;
     }
-  if (rv != expect) {
-    LOG(ERROR) << "read: expected " << expect << ", observed " << rv;
+
+    DCHECK_LE(static_cast<size_t>(bytes), size);
+
+    if (bytes == 0) {
+      break;
+    }
+
+    buffer_c += bytes;
+    size -= bytes;
+    total_bytes += bytes;
+  }
+
+  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<FileOperationResult>(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;
   }
 }
 

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

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 <typename Traits>
-crashpad::FileOperationResult
-ReadOrWrite(int fd, typename Traits::VoidBufferType buffer, size_t size) {
-  typename Traits::CharBufferType buffer_c =
-      reinterpret_cast<typename Traits::CharBufferType>(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<ReadTraits>(file, buffer, size);
+  FileOperationResult bytes = HANDLE_EINTR(read(file, buffer, size));
+  if (bytes < 0) {
+    return -1;
+  }
+
+  DCHECK_LE(static_cast<size_t>(bytes), size);
+  return bytes;
 }
 
 FileOperationResult WriteFile(FileHandle file,
                               const void* buffer,
                               size_t size) {
-  return ReadOrWrite<WriteTraits>(file, buffer, size);
+  const char* buffer_c = static_cast<const char*>(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<size_t>(bytes), size);
+
+    buffer_c += bytes;
+    size -= bytes;
+    total_bytes += bytes;
+  }
+
+  return total_bytes;
 }
 
 FileHandle OpenFileForRead(const base::FilePath& path) {

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));
+
+  while (true) {
     DWORD size_dword = base::checked_cast<DWORD>(size);
-  DWORD total_read = 0;
-  char* buffer_c = reinterpret_cast<char*>(buffer);
-  while (size_dword > 0) {
     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) {
+        // read. Treat this as EOF.
+        return 0;
+      }
       return -1;
     }
-    } else if (bytes_read == 0 && GetFileType(file) != FILE_TYPE_PIPE) {
+
+    CHECK_NE(bytes_read, static_cast<DWORD>(-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<DWORD>(-1));
   CHECK_EQ(bytes_written, size_dword);
   return bytes_written;
 }

util/file/file_reader.cc

@@ -22,12 +22,30 @@ namespace crashpad {
 
 bool FileReaderInterface::ReadExactly(void* data, size_t size) {
   FileOperationResult expect = base::checked_cast<FileOperationResult>(size);
-  FileOperationResult rv = Read(data, size);
-  if (rv < 0) {
+  char* data_c = static_cast<char*>(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;
-  } else if (rv != expect) {
-    LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " << rv;
+    }
+
+    DCHECK_LE(static_cast<size_t>(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<FileOperationResult>(size);
   FileOperationResult rv = ReadFile(file_handle_, data, size);
   if (rv < 0) {
-    PLOG(ERROR) << "read";
+    PLOG(ERROR) << kNativeReadFunctionName;
     return -1;
   }
 

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.

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

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();

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

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.

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) {

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

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()));

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