[Host] handle short reads and writes

[zturner]

Summary

The original motivation for this came from D25712, in which Eugene pointed out that File::Read() does not correctly handle short reads. However, I felt the fix was incomplete because it left the bug in other functions, and the code in general could have used some cleanup since there was a ton of duplication, which may be what led to this bug showing up in the first place.

Changes in this patch are:

1. Have the normal Read() and Write() functions delegate to the versions that read and write with offset.
2. Supply thread-safe versions of the Windows codepaths, which were previously incorrect in a multi-threaded environment.
3. Delete a bunch of dead functions that are not used anywhere in LLDB.
4. Remove the apple specific path due to MAX_READ_SIZE and MAX_WRITE_SIZE and merge with the standard non-Windows path.

The only real tricky part about this patch was that when you open a file in append mode, the old version of Write() with no offset would write at the end, whereas pwrite() always writes at the offset you specify. So to fix this I made the version of Write() with no offset explicitly compute the offset of the end of the file and pass that to the offset-version of Write().

[labath]

I am not sure the "append" thing is actually a "fix". I consider it more like a feature of the append mode. It's also nice that it guarantees atomicity of writes even if two processes are writing to the same file (very useful for logging, although I'm not sure if that goes through these functions). I think we should keep the original behavior of append mode, as that is the behavior anyone familiar with standard file APIs will expect.

[zturner]

In D25783#574679, @labath wrote:

I am not sure the "append" thing is actually a "fix". I consider it more like a feature of the append mode. It's also nice that it guarantees atomicity of writes even if two processes are writing to the same file (very useful for logging, although I'm not sure if that goes through these functions). I think we should keep the original behavior of append mode, as that is the behavior anyone familiar with standard file APIs will expect.

There are many other problems with this code if we want to deal with atomicity. For example, the whole point of this patch was to handle short reads and writes. Well, if you have a short read or a write, then reading and writing a subsequent chunk is not atomic.

That being said, I don't think it's hugely important here. The worst that will happen is two log messages will be printed out of order. I don't think anything will be corrupt or interwoven. For example, if two threads come in at the same time and both compute the same file size, they will both attempt to write at the same offset. One will win, the other will insert right before the message. So it's possible the two could wind up reversed, but that's about it.

Only way to deal with multi-threading correctly is to put all this in a mutex.

[zturner]

Also, for the record, if you specify the threadsafe logging option, it already does put this in a mutex, so there should be no issue.

[labath]

In D25783#574684, @zturner wrote:

There are many other problems with this code if we want to deal with atomicity. For example, the whole point of this patch was to handle short reads and writes. Well, if you have a short read or a write, then reading and writing a subsequent chunk is not atomic.

I am willing abandon the atomicity if someone tries to write more than 2GB of data - I think he has bigger problems than that.

That being said, I don't think it's hugely important here. The worst that will happen is two log messages will be printed out of order. I don't think anything will be corrupt or interwoven. For example, if two threads come in at the same time and both compute the same file size, they will both attempt to write at the same offset. One will win, the other will insert right before the message. So it's possible the two could wind up reversed, but that's about it.

I am not sure what you mean by "insert right before the message". The second thread will just overwrite the thing that the first one wrote (plus you'll end up with a dangling fragment at the end if the second message was shorter.

Only way to deal with multi-threading correctly is to put all this in a mutex.

Actually, a mutex cannot handle this case if you have the multiple file descriptors (possibly in multiple processes) referring to the same file. That is the reason why append mode exists - to let the kernel do the arbitration.

[zturner]

In D25783#574699, @labath wrote:

In D25783#574684, @zturner wrote:

There are many other problems with this code if we want to deal with atomicity. For example, the whole point of this patch was to handle short reads and writes. Well, if you have a short read or a write, then reading and writing a subsequent chunk is not atomic.

I am willing abandon the atomicity if someone tries to write more than 2GB of data - I think he has bigger problems than that.

Writing more than 2GB of data is not the only reason to get a short read or write. You can be interrupted by a signal after some of the data has been written but not all. This can happen with any number of bytes and at any time. write, read, and all other related functions will return a non-negative value indicating the number of bytes successfully read/written, which will be less than the number requested.

[clayborg]

Just a few questions on why we are calling lseek in the read and write functions. See inlined comments.

source/Host/common/File.cpp

405

Why are we calling lseek when we are passing the offset into the read below?

Shouldn't this just be:

off_t offset = 0;

416

Why are we calling lseek here? We specify the offset to the Write below and that function should do the right thing with the offset.

Shouldn't this just be:

off_t offset = 0;

[zturner]

Also you are right that I misspoke about the append case. But still, I just think that if writing to the same file from multiple processes is something we care about, we should support it "for real" instead of just pretending to. That means some kind of cross-process synchronization such as a shared mutex.

[zturner]

source/Host/common/File.cpp

405

This lseek is to get the current file pointer. If someone calls Write() with no offset, they expect this to mean "write at the current file position". In order to do that with pwrite(), you need to know what the current file position actually is. If we just set offset=0, it will write at the beginning of the file, which is probably not the intention.

LMK if I've misunderstood.

[clayborg]

Ok, the lseek calls make sense then.

[labath]

This can happen with any number of bytes and at any time. write, read, and all other related functions will return a non-negative value indicating the number of bytes successfully read/written, which will be less than the number requested.

Except if fd refers to a pipe, in which case writes of up to PIPE_MAX will be atomic.

Also, I just noticed another problem. What if the fd does not refer to an actual file, but a non-seekable file system object (named pipe, domain socket, ...). Will the lseek work on that? I have no idea. But, I think you're implementing a broken API to save a couple of lines of code.

(btw, you may want to know that pwrite() on O_APPEND descriptors basically ignores the offset argument, and always does an append).

[zturner]

In D25783#574860, @labath wrote:

This can happen with any number of bytes and at any time. write, read, and all other related functions will return a non-negative value indicating the number of bytes successfully read/written, which will be less than the number requested.

Except if fd refers to a pipe, in which case writes of up to PIPE_MAX will be atomic.

Also, I just noticed another problem. What if the fd does not refer to an actual file, but a non-seekable file system object (named pipe, domain socket, ...). Will the lseek work on that? I have no idea. But, I think you're implementing a broken API to save a couple of lines of code.

In that case the user should be using Host/Pipe or a more suitable class, this class is already unsuitable just on the grounds that it exposes a method (the offset version) that requires a seekable device. I'd even be fine asserting in the constructor if the device is not seekable.

(btw, you may want to know that pwrite() on O_APPEND descriptors basically ignores the offset argument, and always does an append).

Only on Linux, but this is apparently non-conformant behavior :(

https://linux.die.net/man/2/pwrite

POSIX requires that opening a file with the O_APPEND flag should have no affect on the location at which pwrite() writes data. However, on Linux, if a file is opened with O_APPEND, pwrite() appends data to the end of the file, regardless of the value of offset.

IDK, I'm the Windows guy so this isn't really my call, but I think part of the reason why the test suite is so flaky and bugs are so hard to track down sometimes is because we don't make assumptions. If we find some code that is clearly broken without a given set of assumptions, I think making it break "even more" without those assumptions is not only fine, but even desirable so that problems become easier to track down.

[zturner]

Incidentally, this patch actually makes all platforms behave consistently when the Write overload with offset is used with O_APPEND, so there's probably some value in having that consistency.

[labath]

In D25783#574873, @zturner wrote:

In D25783#574860, @labath wrote:

This can happen with any number of bytes and at any time. write, read, and all other related functions will return a non-negative value indicating the number of bytes successfully read/written, which will be less than the number requested.

Except if fd refers to a pipe, in which case writes of up to PIPE_MAX will be atomic.

Also, I just noticed another problem. What if the fd does not refer to an actual file, but a non-seekable file system object (named pipe, domain socket, ...). Will the lseek work on that? I have no idea. But, I think you're implementing a broken API to save a couple of lines of code.

In that case the user should be using Host/Pipe or a more suitable class, this class is already unsuitable just on the grounds that it exposes a method (the offset version) that requires a seekable device. I'd even be fine asserting in the constructor if the device is not seekable.

If I have a string, the only way to know whether it refers to a seekable file is to open it and do an fstat() on the descriptor. And then I'm in the business of trying to decipher stat.st_mode to figure out whether to instantiate a Pipe, Socket, or a File object (do we want a CharacterDevice class ?). Check the 'a' attribute of https://linux.die.net/man/1/chattr for a fun corner case. Files in the proc filesystem are also very amusing. They are perfectly well readable and writable and appear as regular files in most aspects, but if you do an fstat() on them, you'll see their size as zero. I have no idea what would happen if I did a pwrite on a file in /proc -- it would probably work just fine as linux seems to ignore the offsets when they don't make sense, but that's not something I want to rely on.

IDK, I'm the Windows guy so this isn't really my call, but I think part of the reason why the test suite is so flaky and bugs are so hard to track down sometimes is because we don't make assumptions. If we find some code that is clearly broken without a given set of assumptions, I think making it break "even more" without those assumptions is not only fine, but even desirable so that problems become easier to track down.

I'm all for assertions, but I don't think this is a good example. In UNIX world (and linux is an extreme example of this) a great many things can be represented by a file-like objects (hell, you can even receive signals over a file descriptor, although I don't think you can assign a *name* to that), and here we should make as few assumptions as possible. The safest route for me seems:

• File::Write -> ::write
• File::Write(offset) -> ::pwrite

if pwrite fails on some crazy file, then so be it (you have to expect it to fail for other reasons anyway). It will also be faster, as Write() will do only one syscall instead of three. With a bit of clever programming, I think we can make the code reasonably small as well.

include/lldb/Host/File.h

public:
   ///     filled in if non-nullptr.
   ///
   /// @return
   ///     The resulting seek offset, or -1 on error.
   //------------------------------------------------------------------
   off_t SeekFromStart(off_t offset, Error *error_ptr = nullptr);
 
   //------------------------------------------------------------------
-  /// Seek to an offset relative to the current file position.
-  ///
-  /// NOTE: This function is NOT thread safe, other threads that
-  /// access this object might also change the current file position.
-  /// For thread safe reads and writes see the following functions:
-  /// @see File::Read (void *, size_t, off_t &)
-  /// @see File::Write (const void *, size_t, off_t &)
-  ///
-  /// @param[in] offset
-  ///     The offset to seek to within the file relative to the
-  ///     current file position.
-  ///
-  /// @param[in] error_ptr
-  ///     A pointer to a lldb_private::Error object that will be
-  ///     filled in if non-nullptr.
-  ///
-  /// @return
-  ///     The resulting seek offset, or -1 on error.
-  //------------------------------------------------------------------
-  off_t SeekFromCurrent(off_t offset, Error *error_ptr = nullptr);
-
-  //------------------------------------------------------------------
-  /// Seek to an offset relative to the end of the file.
-  ///
-  /// NOTE: This function is NOT thread safe, other threads that
-  /// access this object might also change the current file position.
-  /// For thread safe reads and writes see the following functions:
-  /// @see File::Read (void *, size_t, off_t &)
-  /// @see File::Write (const void *, size_t, off_t &)
-  ///
-  /// @param[in,out] offset
-  ///     The offset to seek to within the file relative to the
-  ///     end of the file which gets filled in with the resulting
-  ///     absolute file offset.
-  ///
-  /// @param[in] error_ptr
-  ///     A pointer to a lldb_private::Error object that will be
-  ///     filled in if non-nullptr.
-  ///
-  /// @return
-  ///     The resulting seek offset, or -1 on error.
-  //------------------------------------------------------------------
-  off_t SeekFromEnd(off_t offset, Error *error_ptr = nullptr);
-
-  //------------------------------------------------------------------
   /// Read bytes from a file from the specified file offset.
   ///
   /// NOTE: This function is thread safe in that clients manager their
   /// own file position markers and reads on other threads won't mess
   /// up the current read.
   ///
   /// @param[in] dst
   ///     A buffer where to put the bytes that are read.
public:
   ///
   /// @return
   ///     An error object that indicates success or the reason for
   ///     failure.
   //------------------------------------------------------------------
   Error Flush();
 
   //------------------------------------------------------------------
-  /// Sync to disk.
-  ///
-  /// @return
-  ///     An error object that indicates success or the reason for
-  ///     failure.
-  //------------------------------------------------------------------
-  Error Sync();
-
-  //------------------------------------------------------------------
   /// Get the permissions for a this file.
   ///
   /// @return
   ///     Bits logical OR'ed together from the permission bits defined
   ///     in lldb_private::File::Permissions.
   //------------------------------------------------------------------
   uint32_t GetPermissions(Error &error) const;
 

source/Host/common/File.cpp

if (error_ptr) {
         error_ptr->Clear();
     }
   } else if (error_ptr) {
     error_ptr->SetErrorString("invalid file handle");
   }
   return result;
 }
 
-off_t File::SeekFromCurrent(off_t offset, Error *error_ptr) {
-  off_t result = -1;
-  if (DescriptorIsValid()) {
-    result = ::lseek(m_descriptor, offset, SEEK_CUR);
-
-    if (error_ptr) {
-      if (result == -1)
-        error_ptr->SetErrorToErrno();
-      else
-        error_ptr->Clear();
-    }
-  } else if (StreamIsValid()) {
-    result = ::fseek(m_stream, offset, SEEK_CUR);
-
-    if (error_ptr) {
-      if (result == -1)
-        error_ptr->SetErrorToErrno();
-      else
-        error_ptr->Clear();
-    }
-  } else if (error_ptr) {
-    error_ptr->SetErrorString("invalid file handle");
-  }
-  return result;
-}
-
-off_t File::SeekFromEnd(off_t offset, Error *error_ptr) {
-  off_t result = -1;
-  if (DescriptorIsValid()) {
-    result = ::lseek(m_descriptor, offset, SEEK_END);
-
-    if (error_ptr) {
-      if (result == -1)
-        error_ptr->SetErrorToErrno();
-      else
-        error_ptr->Clear();
-    }
-  } else if (StreamIsValid()) {
-    result = ::fseek(m_stream, offset, SEEK_END);
-
-    if (error_ptr) {
-      if (result == -1)
-        error_ptr->SetErrorToErrno();
-      else
-        error_ptr->Clear();
-    }
-  } else if (error_ptr) {
-    error_ptr->SetErrorString("invalid file handle");
-  }
-  return result;
-}
-
 Error File::Flush() {
   Error error;
   if (StreamIsValid()) {
     int err = 0;
     do {
       err = ::fflush(m_stream);
     } while (err == EOF && errno == EINTR);
 
     if (err == EOF)
       error.SetErrorToErrno();
   } else if (!DescriptorIsValid()) {
     error.SetErrorString("invalid file handle");
   }
   return error;
 }
 
-Error File::Sync() {
-  Error error;
-  if (DescriptorIsValid()) {
-#ifdef _WIN32
-    int err = FlushFileBuffers((HANDLE)_get_osfhandle(m_descriptor));
-    if (err == 0)
-      error.SetErrorToGenericError();
-#else
-    int err = 0;
-    do {
-      err = ::fsync(m_descriptor);
-    } while (err == -1 && errno == EINTR);
-
-    if (err == -1)
-      error.SetErrorToErrno();
-#endif
-  } else {
-    error.SetErrorString("invalid file handle");
-  }
-  return error;
-}
-
 #if defined(__APPLE__)
 // Darwin kernels only can read/write <= INT_MAX bytes
-#define MAX_READ_SIZE INT_MAX
-#define MAX_WRITE_SIZE INT_MAX
+constexpr size_t MAX_READ_SIZE = INT_MAX;
+constexpr size_t MAX_WRITE_SIZE = INT_MAX;
+#elif defined(LLVM_ON_WIN32)
+constexpr size_t MAX_READ_SIZE = ULONG_MAX;
+constexpr size_t MAX_WRITE_SIZE = ULONG_MAX;
+#else
+constexpr size_t MAX_READ_SIZE = SIZE_MAX;
+constexpr size_t MAX_WRITE_SIZE = SIZE_MAX;
 #endif
 
 Error File::Read(void *buf, size_t &num_bytes) {
-  Error error;
-
-#if defined(MAX_READ_SIZE)
-  if (num_bytes > MAX_READ_SIZE) {
-    uint8_t *p = (uint8_t *)buf;
-    size_t bytes_left = num_bytes;
-    // Init the num_bytes read to zero
+  int fd = GetDescriptor();
+  if (fd == kInvalidDescriptor) {
     num_bytes = 0;
-
-    while (bytes_left > 0) {
-      size_t curr_num_bytes;
-      if (bytes_left > MAX_READ_SIZE)
-        curr_num_bytes = MAX_READ_SIZE;
-      else
-        curr_num_bytes = bytes_left;
-
-      error = Read(p + num_bytes, curr_num_bytes);
-
-      // Update how many bytes were read
-      num_bytes += curr_num_bytes;
-      if (bytes_left < curr_num_bytes)
-        bytes_left = 0;
-      else
-        bytes_left -= curr_num_bytes;
-
-      if (error.Fail())
-        break;
-    }
-    return error;
-  }
-#endif
-
-  ssize_t bytes_read = -1;
-  if (DescriptorIsValid()) {
-    do {
-      bytes_read = ::read(m_descriptor, buf, num_bytes);
-    } while (bytes_read < 0 && errno == EINTR);
+    return Error("invalid file handle");
 
-    if (bytes_read == -1) {
-      error.SetErrorToErrno();
-      num_bytes = 0;
-    } else
-      num_bytes = bytes_read;
-  } else if (StreamIsValid()) {
-    bytes_read = ::fread(buf, 1, num_bytes, m_stream);
-
-    if (bytes_read == 0) {
-      if (::feof(m_stream))
-        error.SetErrorString("feof");
-      else if (::ferror(m_stream))
-        error.SetErrorString("ferror");
-      num_bytes = 0;
-    } else
-      num_bytes = bytes_read;
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
-  }
-  return error;
+  off_t offset = lseek(fd, 0, SEEK_CUR);
+  return Read(buf, num_bytes, offset);
 }
 
 Error File::Write(const void *buf, size_t &num_bytes) {
-  Error error;
-
-#if defined(MAX_WRITE_SIZE)
-  if (num_bytes > MAX_WRITE_SIZE) {
-    const uint8_t *p = (const uint8_t *)buf;
-    size_t bytes_left = num_bytes;
-    // Init the num_bytes written to zero
+  int fd = GetDescriptor();
+  if (fd == kInvalidDescriptor) {
     num_bytes = 0;
-
-    while (bytes_left > 0) {
-      size_t curr_num_bytes;
-      if (bytes_left > MAX_WRITE_SIZE)
-        curr_num_bytes = MAX_WRITE_SIZE;
-      else
-        curr_num_bytes = bytes_left;
-
-      error = Write(p + num_bytes, curr_num_bytes);
-
-      // Update how many bytes were read
-      num_bytes += curr_num_bytes;
-      if (bytes_left < curr_num_bytes)
-        bytes_left = 0;
-      else
-        bytes_left -= curr_num_bytes;
-
-      if (error.Fail())
-        break;
-    }
-    return error;
+    return Error("invalid file handle");
   }
-#endif
 
-  ssize_t bytes_written = -1;
-  if (DescriptorIsValid()) {
-    do {
-      bytes_written = ::write(m_descriptor, buf, num_bytes);
-    } while (bytes_written < 0 && errno == EINTR);
-
-    if (bytes_written == -1) {
-      error.SetErrorToErrno();
-      num_bytes = 0;
-    } else
-      num_bytes = bytes_written;
-  } else if (StreamIsValid()) {
-    bytes_written = ::fwrite(buf, 1, num_bytes, m_stream);
-
-    if (bytes_written == 0) {
-      if (::feof(m_stream))
-        error.SetErrorString("feof");
-      else if (::ferror(m_stream))
-        error.SetErrorString("ferror");
-      num_bytes = 0;
-    } else
-      num_bytes = bytes_written;
-
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
+  off_t offset = lseek(fd, 0, SEEK_CUR);
+  if (m_options & File::eOpenOptionAppend) {
+    struct stat S;
+    if (::fstat(fd, &S) != 0)
+      return Error("Invalid file handle!");
+    offset = S.st_size;
   }
-
-  return error;
+  return Write(buf, num_bytes, offset);
 }
 
 Error File::Read(void *buf, size_t &num_bytes, off_t &offset) {
   Error error;
-
-#if defined(MAX_READ_SIZE)
-  if (num_bytes > MAX_READ_SIZE) {
-    uint8_t *p = (uint8_t *)buf;
-    size_t bytes_left = num_bytes;
-    // Init the num_bytes read to zero
+  int fd = GetDescriptor();
+  if (fd == kInvalidDescriptor) {
     num_bytes = 0;
-
-    while (bytes_left > 0) {
-      size_t curr_num_bytes;
-      if (bytes_left > MAX_READ_SIZE)
-        curr_num_bytes = MAX_READ_SIZE;
-      else
-        curr_num_bytes = bytes_left;
-
-      error = Read(p + num_bytes, curr_num_bytes, offset);
-
-      // Update how many bytes were read
-      num_bytes += curr_num_bytes;
-      if (bytes_left < curr_num_bytes)
-        bytes_left = 0;
-      else
-        bytes_left -= curr_num_bytes;
-
-      if (error.Fail())
-        break;
-    }
-    return error;
+    return Error("invalid file handle");
   }
-#endif
 
+  uint8_t *dest = static_cast<uint8_t *>(buf);
 #ifndef _WIN32
-  int fd = GetDescriptor();
-  if (fd != kInvalidDescriptor) {
-    ssize_t bytes_read = -1;
+  size_t bytes_to_read = std::min(MAX_READ_SIZE, bytes_left);
+  while (bytes_left != 0 && bytes_read != 0) {
     do {
-      bytes_read = ::pread(fd, buf, num_bytes, offset);
+      bytes_read = ::pread(fd, dest, num_bytes, offset);
     } while (bytes_read < 0 && errno == EINTR);
 
     if (bytes_read < 0) {
       num_bytes = 0;
       error.SetErrorToErrno();
-    } else {
-      offset += bytes_read;
-      num_bytes = bytes_read;
+      return error;
     }
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
+
+    dest += bytes_read;
+    offset += bytes_read;
+    bytes_left -= bytes_read;
   }
+  num_bytes -= bytes_left;
 #else
-  long cur = ::lseek(m_descriptor, 0, SEEK_CUR);
-  SeekFromStart(offset);
-  error = Read(buf, num_bytes);
-  if (!error.Fail())
-    SeekFromStart(cur);
+  HANDLE H = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  if (!H || H == INVALID_HANDLE_VALUE)
+    return Error("invalid file handle");
+
+  LARGE_INTEGER large_off;
+  large_off.QuadPart = offset;
+
+  OVERLAPPED OV = {};
+  OV.Offset = large_off.LowPart;
+  OV.OffsetHigh = large_off.HighPart;
+
+  while (bytes_left != 0) {
+    DWORD req_size = static_cast<DWORD>(std::min(MAX_READ_SIZE, bytes_left));
+    DWORD actual_size = 0;
+    BOOL result = ::ReadFile(H, dest, req_size, &actual_size, &OV);
+    if (!result) {
+      error.SetError(GetLastError(), eErrorTypeWin32);
+      return error;
+    }
+    if (actual_size == 0) {
+      // EOF was reached.
+      break;
+    }
+    dest += actual_size;
+    bytes_left -= actual_size;
+  }
+  num_bytes -= bytes_left;
 #endif
   return error;
 }
 
 Error File::Read(size_t &num_bytes, off_t &offset, bool null_terminate,
                  DataBufferSP &data_buffer_sp) {
   Error error;
 
Error File::Read(size_t &num_bytes, off_t &offset, bool null_terminate,
 
   num_bytes = 0;
   data_buffer_sp.reset();
   return error;
 }
 
 Error File::Write(const void *buf, size_t &num_bytes, off_t &offset) {
   Error error;
-
-#if defined(MAX_WRITE_SIZE)
-  if (num_bytes > MAX_WRITE_SIZE) {
-    const uint8_t *p = (const uint8_t *)buf;
-    size_t bytes_left = num_bytes;
-    // Init the num_bytes written to zero
+  int fd = GetDescriptor();
+  if (fd == kInvalidDescriptor) {
     num_bytes = 0;
-
-    while (bytes_left > 0) {
-      size_t curr_num_bytes;
-      if (bytes_left > MAX_WRITE_SIZE)
-        curr_num_bytes = MAX_WRITE_SIZE;
-      else
-        curr_num_bytes = bytes_left;
-
-      error = Write(p + num_bytes, curr_num_bytes, offset);
-
-      // Update how many bytes were read
-      num_bytes += curr_num_bytes;
-      if (bytes_left < curr_num_bytes)
-        bytes_left = 0;
-      else
-        bytes_left -= curr_num_bytes;
-
-      if (error.Fail())
-        break;
-    }
-    return error;
+    return Error("invalid file handle");
   }
-#endif
 
-  int fd = GetDescriptor();
-  if (fd != kInvalidDescriptor) {
+  const uint8_t *src = static_cast<const uint8_t *>(buf);
 #ifndef _WIN32
-    ssize_t bytes_written = -1;
+  size_t bytes_to_write = std::min(MAX_WRITE_SIZE, bytes_left);
+  while (bytes_left != 0) {
     do {
-      bytes_written = ::pwrite(m_descriptor, buf, num_bytes, offset);
+      bytes_written = ::pwrite(fd, src, num_bytes, offset);
     } while (bytes_written < 0 && errno == EINTR);
 
     if (bytes_written < 0) {
       num_bytes = 0;
       error.SetErrorToErrno();
-    } else {
-      offset += bytes_written;
-      num_bytes = bytes_written;
-    }
+      return error;
+    }
+
+    src += bytes_written;
+    bytes_left -= bytes_written;
 #else
-    long cur = ::lseek(m_descriptor, 0, SEEK_CUR);
-    error = Write(buf, num_bytes);
-    long after = ::lseek(m_descriptor, 0, SEEK_CUR);
-
-    if (!error.Fail())
-      SeekFromStart(cur);
-
-    offset = after;
-#endif
-  } else {
-    num_bytes = 0;
-    error.SetErrorString("invalid file handle");
+  HANDLE H = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  if (!H || H == INVALID_HANDLE_VALUE)
+    return Error("invalid file handle");
+
+  LARGE_INTEGER large_off;
+  large_off.QuadPart = offset;
+
+  OVERLAPPED OV = {};
+  OV.Offset = large_off.LowPart;
+  OV.OffsetHigh = large_off.HighPart;
+
+  while (bytes_left != 0) {
+    DWORD req_size = static_cast<DWORD>(std::min(MAX_WRITE_SIZE, bytes_left));
+    DWORD actual_size = 0;
+    BOOL result = ::WriteFile(H, src, req_size, &actual_size, &OV);
+    if (!result) {
+      error.SetError(GetLastError(), eErrorTypeWin32);
+      return error;
     }
+    src += actual_size;
+    bytes_left -= actual_size;
+  }
+#endif
   return error;
 }
 
 //------------------------------------------------------------------
 // Print some formatted output to the stream.
 //------------------------------------------------------------------
 size_t File::Printf(const char *format, ...) {
   va_list args;