Fix a use after free crasher in the ReadAsync task initiated on Windows by the...

Fix a use after free crasher in the ReadAsync task initiated on Windows by the FileStream::Context::Read operation.

The crash was reported by the DrMemory bot and based on the stack happens because the OVERLAPPED structure passed into
the ReadFile call is invalid.

Proposed fix is the following:-
1. Have two flags io_complete_for_read_received_ and async_read_completed_ which track whether the IO completion was
   received for a Read and whether we received a notification on the calling thread that the ReadFile call returned.
   We invoke the user callback only when both these flags are true.

2. We have another flag async_read_initiated_ which is set to true if an asynchonous Read was initated. We use this
   to not set the async_in_progress_ flag to false until both notifications as per 1 above are received.

3. All flags above are reset when we invoke the user callback. That now happens in the InvokeUserCallback function.

4. We need to save the result in a member as the callback is invoked later.

5. Removed the Weak pointer member from the Context class as this is not needed because the Context instance should remain
   valid until the pending Read operation completes.

BUG=455066

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

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

net/base/file_stream_context.cc

@@ -77,12 +77,6 @@ void FileStream::Context::Orphan() {
 
   orphaned_ = true;
 
-#if defined(OS_WIN)
-  // Clean up weak pointers here to ensure that they are destroyed on the
-  // same thread where they were created.
-  weak_ptr_factory_.InvalidateWeakPtrs();
-#endif
-
   if (!async_in_progress_) {
     CloseAndDelete();
   } else if (file_.IsValid()) {
@@ -221,7 +215,10 @@ void FileStream::Context::OnOpenCompleted(const CompletionCallback& callback,
 }
 
 void FileStream::Context::CloseAndDelete() {
-  DCHECK(!async_in_progress_);
+  // TODO(ananta)
+  // Replace this CHECK with a DCHECK once we figure out the root cause of
+  // http://crbug.com/455066
+  CHECK(!async_in_progress_);
 
   if (file_.IsValid()) {
     bool posted = task_runner_.get()->PostTask(

net/base/file_stream_context.h

@@ -161,12 +161,16 @@ class FileStream::Context {
                              DWORD bytes_read,
                              DWORD error) override;
 
+  // Invokes the user callback.
+  void InvokeUserCallback();
+
   // 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
   // on a worker thread.
-  // The |context| parameter is a weak pointer instance passed to the worker
-  // pool.
+  // The |context| parameter is a pointer to the current Context instance. It
+  // is safe to pass this as is to the pool as the Context instance should
+  // remain valid until the pending Read operation completes.
   // The |file| parameter is the handle to the file being read.
   // The |buf| parameter is the buffer where we want the ReadFile to read the
   // data into.
@@ -176,7 +180,7 @@ class FileStream::Context {
   // The |origin_thread_loop| is a MessageLoopProxy instance used to post tasks
   // back to the originating thread.
   static void ReadAsync(
-      const base::WeakPtr<FileStream::Context>& context,
+      FileStream::Context* context,
       HANDLE file,
       scoped_refptr<net::IOBuffer> buf,
       int buf_len,
@@ -185,9 +189,11 @@ class FileStream::Context {
 
   // This callback executes on the main calling thread. It informs the caller
   // about the result 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 os_error);
+  void ReadAsyncResult(DWORD bytes_read, DWORD os_error);
 
 #elif defined(OS_POSIX)
   // ReadFileImpl() is a simple wrapper around read() that handles EINTR
@@ -209,8 +215,18 @@ class FileStream::Context {
   base::MessageLoopForIO::IOContext io_context_;
   CompletionCallback callback_;
   scoped_refptr<IOBuffer> in_flight_buf_;
-  // WeakPtrFactory for posting tasks back to |this|.
-  base::WeakPtrFactory<Context> weak_ptr_factory_;
+  // This flag is set to true when we receive a Read request which is queued to
+  // the thread pool.
+  bool async_read_initiated_;
+  // This flag is set to true when we receive a notification ReadAsyncResult()
+  // on the calling thread which indicates that the asynchronous Read
+  // operation is complete.
+  bool async_read_completed_;
+  // This flag is set to true when we receive an IO completion notification for
+  // an asynchonously initiated Read operaton. OnIOComplete().
+  bool io_complete_for_read_received_;
+  // Tracks the result of the IO completion operation. Set in OnIOComplete.
+  int result_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(Context);

net/base/file_stream_context_win.cc

@@ -41,7 +41,10 @@ FileStream::Context::Context(const scoped_refptr<base::TaskRunner>& task_runner)
       async_in_progress_(false),
       orphaned_(false),
       task_runner_(task_runner),
-      weak_ptr_factory_(this) {
+      async_read_initiated_(false),
+      async_read_completed_(false),
+      io_complete_for_read_received_(false),
+      result_(0) {
   io_context_.handler = this;
   memset(&io_context_.overlapped, 0, sizeof(io_context_.overlapped));
 }
@@ -53,7 +56,10 @@ FileStream::Context::Context(base::File file,
       async_in_progress_(false),
       orphaned_(false),
       task_runner_(task_runner),
-      weak_ptr_factory_(this) {
+      async_read_initiated_(false),
+      async_read_completed_(false),
+      io_complete_for_read_received_(false),
+      result_(0) {
   io_context_.handler = this;
   memset(&io_context_.overlapped, 0, sizeof(io_context_.overlapped));
   if (file_.IsValid()) {
@@ -72,24 +78,29 @@ int FileStream::Context::Read(IOBuffer* buf,
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("423948 FileStream::Context::Read"));
 
-  DCHECK(!async_in_progress_);
+  CHECK(!async_in_progress_);
+  DCHECK(!async_read_initiated_);
+  DCHECK(!async_read_completed_);
+  DCHECK(!io_complete_for_read_received_);
 
   IOCompletionIsPending(callback, buf);
 
-  base::WorkerPool::PostTask(
-      FROM_HERE,
-      base::Bind(&FileStream::Context::ReadAsync,
-                 weak_ptr_factory_.GetWeakPtr(), file_.GetPlatformFile(),
-                 make_scoped_refptr(buf), buf_len, &io_context_.overlapped,
-                 base::MessageLoop::current()->message_loop_proxy()),
-      false);
+  async_read_initiated_ = true;
 
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FileStream::Context::ReadAsync, base::Unretained(this),
+                 file_.GetPlatformFile(), make_scoped_refptr(buf), buf_len,
+                 &io_context_.overlapped,
+                 base::MessageLoop::current()->message_loop_proxy()));
   return ERR_IO_PENDING;
 }
 
 int FileStream::Context::Write(IOBuffer* buf,
                                int buf_len,
                                const CompletionCallback& callback) {
+  CHECK(!async_in_progress_);
+
   DWORD bytes_written = 0;
   if (!WriteFile(file_.GetPlatformFile(), buf->data(), buf_len,
                  &bytes_written, &io_context_.overlapped)) {
@@ -140,49 +151,72 @@ void FileStream::Context::OnIOCompleted(
   DCHECK(!callback_.is_null());
   DCHECK(async_in_progress_);
 
-  async_in_progress_ = false;
+  if (!async_read_initiated_)
+    async_in_progress_ = false;
+
   if (orphaned_) {
+    async_in_progress_ = false;
     callback_.Reset();
     in_flight_buf_ = NULL;
     CloseAndDelete();
     return;
   }
 
-  int result;
   if (error == ERROR_HANDLE_EOF) {
-    result = 0;
+    result_ = 0;
   } else if (error) {
     IOResult error_result = IOResult::FromOSError(error);
-    result = static_cast<int>(error_result.result);
+    result_ = static_cast<int>(error_result.result);
   } else {
-    result = bytes_read;
+    result_ = bytes_read;
     IncrementOffset(&io_context_.overlapped, bytes_read);
   }
 
+  if (async_read_initiated_)
+    io_complete_for_read_received_ = true;
+
+  InvokeUserCallback();
+}
+
+void FileStream::Context::InvokeUserCallback() {
+  // For an asynchonous Read operation don't invoke the user callback until
+  // we receive the IO completion notification and the asynchronous Read
+  // completion notification.
+  if (async_read_initiated_) {
+    if (!io_complete_for_read_received_ || !async_read_completed_)
+      return;
+    async_read_initiated_ = false;
+    io_complete_for_read_received_ = false;
+    async_read_completed_ = false;
+    async_in_progress_ = false;
+  }
   CompletionCallback temp_callback = callback_;
   callback_.Reset();
   scoped_refptr<IOBuffer> temp_buf = in_flight_buf_;
   in_flight_buf_ = NULL;
-  temp_callback.Run(result);
+  temp_callback.Run(result_);
 }
 
 // static
 void FileStream::Context::ReadAsync(
-    const base::WeakPtr<FileStream::Context>& context,
+    FileStream::Context* context,
     HANDLE file,
     scoped_refptr<net::IOBuffer> buf,
     int buf_len,
     OVERLAPPED* overlapped,
     scoped_refptr<base::MessageLoopProxy> origin_thread_loop) {
   DWORD bytes_read = 0;
-  if (!ReadFile(file, buf->data(), buf_len, &bytes_read, overlapped)) {
-    origin_thread_loop->PostTask(
-        FROM_HERE, base::Bind(&FileStream::Context::ReadAsyncResult, context,
-                              ::GetLastError()));
-  }
+  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()));
 }
 
-void FileStream::Context::ReadAsyncResult(DWORD os_error) {
+void FileStream::Context::ReadAsyncResult(DWORD bytes_read, DWORD os_error) {
+  if (!os_error)
+    result_ = bytes_read;
+
   IOResult error = IOResult::FromOSError(os_error);
   if (error.os_error == ERROR_HANDLE_EOF) {
     // Report EOF by returning 0 bytes read.
@@ -190,10 +224,13 @@ void FileStream::Context::ReadAsyncResult(DWORD 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)
+    if (error.os_error) {
       LOG(WARNING) << "ReadFile failed: " << error.os_error;
-    OnIOCompleted(&io_context_, 0, error.os_error);
+      OnIOCompleted(&io_context_, 0, error.os_error);
+    }
   }
+  async_read_completed_ = true;
+  InvokeUserCallback();
 }
 
 }  // namespace net

tools/valgrind/drmemory/suppressions.txt

@@ -722,14 +722,6 @@ name=http://crbug.com/455060
 ...
 *!content::RenderWidgetHostViewAura::Destroy
 
-UNADDRESSABLE ACCESS
-name=http://crbug.com/455066
-system call NtReadFile parameter #4
-KERNELBASE.dll!ReadFile
-KERNEL32.dll!ReadFile
-*!net::FileStream::Context::ReadAsync
-*!base::internal::RunnableAdapter<>::Run
-
 INVALID HEAP ARGUMENT
 name=http://crbug.com/455994
 drmemorylib.dll!replace_operator_delete