Fix a crash in the FileStream::Context::Read code path where we were invoking a NULL callback.

This could happen if if the ReadFileAsync completion calls OnIOComplete for an orphanaed object.

Fix is to rework the way these callbacks work.

1. Handle the case where in an orphaned context could be
   deleted before the read async completion callback is received.

2. Pass the return of the ReadFile call to the ReadAsyncResult callback. The logic to call the IO completion routine or the user callback has been consolidated in this function.

BUG=458253

Review URL: https://codereview.chromium.org/920123002

Cr-Commit-Position: refs/heads/master@{#316362}

2 files changed, 45 insertions, 21 deletions

diff --git a/net/base/file_stream_context.h b/net/base/file_stream_context.h
index 8d037bc..cb1b7aa 100644
--- a/net/base/file_stream_context.h
+++ b/net/base/file_stream_context.h
@@ -164,6 +164,9 @@ class FileStream::Context {
   // Invokes the user callback.
   void InvokeUserCallback();
 
+  // Deletes an orphaned context.
+  void DeleteOrphanedContext();
+
   // The ReadFile call on Windows can execute synchonously at times.
   // http://support.microsoft.com/kb/156932. This ends up blocking the calling
   // thread which is undesirable. To avoid this we execute the ReadFile call
@@ -189,11 +192,13 @@ class FileStream::Context {
 
   // This callback executes on the main calling thread. It informs the caller
   // about the result of the ReadFile call.
+  // The |read_file_ret| parameter contains the return value of the ReadFile
+  // call.
   // The |bytes_read| contains the number of bytes read from the file, if
   // ReadFile succeeds.
   // The |os_error| parameter contains the value of the last error returned by
   // the ReadFile API.
-  void ReadAsyncResult(DWORD bytes_read, DWORD os_error);
+  void ReadAsyncResult(BOOL read_file_ret, DWORD bytes_read, DWORD os_error);
 
 #elif defined(OS_POSIX)
   // ReadFileImpl() is a simple wrapper around read() that handles EINTR
diff --git a/net/base/file_stream_context_win.cc b/net/base/file_stream_context_win.cc
index 5ffbde7..a41a74c 100644
--- a/net/base/file_stream_context_win.cc
+++ b/net/base/file_stream_context_win.cc
@@ -81,6 +81,7 @@ int FileStream::Context::Read(IOBuffer* buf,
   IOCompletionIsPending(callback, buf);
 
   async_read_initiated_ = true;
+  result_ = 0;
 
   task_runner_->PostTask(
       FROM_HERE,
@@ -96,6 +97,8 @@ int FileStream::Context::Write(IOBuffer* buf,
                                const CompletionCallback& callback) {
   CHECK(!async_in_progress_);
 
+  result_ = 0;
+
   DWORD bytes_written = 0;
   if (!WriteFile(file_.GetPlatformFile(), buf->data(), buf_len,
                  &bytes_written, &io_context_.overlapped)) {
@@ -150,10 +153,12 @@ void FileStream::Context::OnIOCompleted(
     async_in_progress_ = false;
 
   if (orphaned_) {
-    async_in_progress_ = false;
-    callback_.Reset();
-    in_flight_buf_ = NULL;
-    CloseAndDelete();
+    io_complete_for_read_received_ = true;
+    // If we are called due to a pending read and the asynchronous read task
+    // has not completed we have to keep the context around until it completes.
+    if (async_read_initiated_ && !async_read_completed_)
+      return;
+    DeleteOrphanedContext();
     return;
   }
 
@@ -163,6 +168,8 @@ void FileStream::Context::OnIOCompleted(
     IOResult error_result = IOResult::FromOSError(error);
     result_ = static_cast<int>(error_result.result);
   } else {
+    if (result_)
+      DCHECK_EQ(result_, static_cast<int>(bytes_read));
     result_ = bytes_read;
     IncrementOffset(&io_context_.overlapped, bytes_read);
   }
@@ -192,6 +199,13 @@ void FileStream::Context::InvokeUserCallback() {
   temp_callback.Run(result_);
 }
 
+void FileStream::Context::DeleteOrphanedContext() {
+  async_in_progress_ = false;
+  callback_.Reset();
+  in_flight_buf_ = NULL;
+  CloseAndDelete();
+}
+
 // static
 void FileStream::Context::ReadAsync(
     FileStream::Context* context,
@@ -203,29 +217,34 @@ void FileStream::Context::ReadAsync(
   DWORD bytes_read = 0;
   BOOL ret = ::ReadFile(file, buf->data(), buf_len, &bytes_read, overlapped);
   origin_thread_loop->PostTask(
-      FROM_HERE, base::Bind(&FileStream::Context::ReadAsyncResult,
-                            base::Unretained(context), ret ? bytes_read : 0,
-                            ret ? 0 : ::GetLastError()));
+      FROM_HERE,
+      base::Bind(&FileStream::Context::ReadAsyncResult,
+                 base::Unretained(context), ret, bytes_read, ::GetLastError()));
 }
 
-void FileStream::Context::ReadAsyncResult(DWORD bytes_read, DWORD os_error) {
-  if (!os_error)
+void FileStream::Context::ReadAsyncResult(BOOL read_file_ret,
+                                          DWORD bytes_read,
+                                          DWORD os_error) {
+  // If the context is orphaned and we already received the io completion
+  // notification then we should delete the context and get out.
+  if (orphaned_ && io_complete_for_read_received_) {
+    DeleteOrphanedContext();
+    return;
+  }
+
+  async_read_completed_ = true;
+  if (read_file_ret) {
     result_ = bytes_read;
+    InvokeUserCallback();
+    return;
+  }
 
   IOResult error = IOResult::FromOSError(os_error);
-  if (error.os_error == ERROR_HANDLE_EOF) {
-    // Report EOF by returning 0 bytes read.
+  if (error.os_error == ERROR_IO_PENDING) {
+    InvokeUserCallback();
+  } else {
     OnIOCompleted(&io_context_, 0, error.os_error);
-  } else if (error.os_error != ERROR_IO_PENDING) {
-    // We don't need to inform the caller about ERROR_PENDING_IO as that was
-    // already done when the ReadFile call was queued to the worker pool.
-    if (error.os_error) {
-      LOG(WARNING) << "ReadFile failed: " << error.os_error;
-      OnIOCompleted(&io_context_, 0, error.os_error);
-    }
   }
-  async_read_completed_ = true;
-  InvokeUserCallback();
 }
 
 }  // namespace net