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

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
Committed: https://crrev.com/806016e8bf4e217f2ebe5ad052481699f2287c63
Cr-Commit-Position: refs/heads/master@{#315098}

[ananta, 2015-02-05 23:10:44]

ananta@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2015-02-06 00:07:03]

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
net/base/file_stream_context_win.cc:87: 
nit: remove this line

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
net/base/file_stream_context_win.cc:163: result_ = 0;
nit: not needed

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
net/base/file_stream_context_win.cc:208: ReadFile(file, buf->data(), buf_len,
&bytes_read, overlapped);
if the function succeeds, pass bytes_read to the other thread.

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
net/base/file_stream_context_win.cc:81: DCHECK(!async_in_progress_);
CHECK?

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
net/base/file_stream_context_win.cc:102: DWORD bytes_written = 0;
+ CHECK?

[ananta, 2015-02-06 01:18:19]

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
net/base/file_stream_context_win.cc:87: 
On 2015/02/06 00:07:02, rvargas wrote:
> nit: remove this line

Done.

https://chromiumcodereview.appspot.com/888143003/diff/40001/net/base/file_str...
net/base/file_stream_context_win.cc:208: ReadFile(file, buf->data(), buf_len,
&bytes_read, overlapped);
On 2015/02/06 00:07:02, rvargas wrote:
> if the function succeeds, pass bytes_read to the other thread.

Done.

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
net/base/file_stream_context_win.cc:81: DCHECK(!async_in_progress_);
On 2015/02/06 00:07:03, rvargas wrote:
> CHECK?

Done.

https://chromiumcodereview.appspot.com/888143003/diff/60001/net/base/file_str...
net/base/file_stream_context_win.cc:102: DWORD bytes_written = 0;
On 2015/02/06 00:07:02, rvargas wrote:
> + CHECK?

Done.

[rvargas (doing something else), 2015-02-06 01:46:33]

https://chromiumcodereview.appspot.com/888143003/diff/120001/net/base/file_st...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/120001/net/base/file_st...
net/base/file_stream_context_win.cc:213: base::Unretained(context), bytes_read,
::GetLastError()));
bytes read is only valid if ReadFile returns true so the return value cannot be
ignored.

[ananta, 2015-02-06 02:20:01]

https://chromiumcodereview.appspot.com/888143003/diff/120001/net/base/file_st...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/120001/net/base/file_st...
net/base/file_stream_context_win.cc:213: base::Unretained(context), bytes_read,
::GetLastError()));
On 2015/02/06 01:46:33, rvargas wrote:
> bytes read is only valid if ReadFile returns true so the return value cannot
be
> ignored.

Done.

[rvargas (doing something else), 2015-02-06 02:47:20]

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
net/base/file_stream_context_win.cc:209: BOOL ret = ReadFile(file, buf->data(),
buf_len, &bytes_read, overlapped);
nit: if we are using ::GetLastError we should use ::ReadFile

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
net/base/file_stream_context_win.cc:217: if (bytes_read)
This is slightly problematic as it allows for some abuse: if I ask to read 0
bytes, ReadFile will return 0 bytes and it will not be EOF (and technically the
error should not be inspected because there was no error, but yes, in practice
it works).

Maybe a better way to deal with this is to force the error to zero if (ret) and
check for os_error here instead of byres_read.

[rvargas (doing something else), 2015-02-06 02:50:29]

Given that this is an experimental CL anyway, lgtm.

[ananta, 2015-02-06 03:16:37]

New patchsets have been uploaded after l-g-t-m from rvargas@chromium.org

[ananta, 2015-02-06 03:16:50]

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
File net/base/file_stream_context_win.cc (right):

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
net/base/file_stream_context_win.cc:209: BOOL ret = ReadFile(file, buf->data(),
buf_len, &bytes_read, overlapped);
On 2015/02/06 02:47:20, rvargas wrote:
> nit: if we are using ::GetLastError we should use ::ReadFile

Done.

https://chromiumcodereview.appspot.com/888143003/diff/140001/net/base/file_st...
net/base/file_stream_context_win.cc:217: if (bytes_read)
On 2015/02/06 02:47:20, rvargas wrote:
> This is slightly problematic as it allows for some abuse: if I ask to read 0
> bytes, ReadFile will return 0 bytes and it will not be EOF (and technically
the
> error should not be inspected because there was no error, but yes, in practice
> it works).
> 
> Maybe a better way to deal with this is to force the error to zero if (ret)
and
> check for os_error here instead of byres_read.

Done.

[ananta, 2015-02-06 19:05:04]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-02-06 19:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888143003/180001

[commit-bot: I haz the power, 2015-02-06 19:12:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-06 19:12:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ananta, 2015-02-06 19:25:13]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-02-06 19:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/888143003/200001

[commit-bot: I haz the power, 2015-02-06 20:30:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-02-06 20:30:46]

Patchset 11 (id:??) landed as
https://crrev.com/806016e8bf4e217f2ebe5ad052481699f2287c63
Cr-Commit-Position: refs/heads/master@{#315098}