[android_webview] Fix use after free in intercepted requests.

android_webview/browser/net/android_stream_reader_url_request_job.cc

Index: android_webview/browser/net/android_stream_reader_url_request_job.cc
diff --git a/android_webview/browser/net/android_stream_reader_url_request_job.cc b/android_webview/browser/net/android_stream_reader_url_request_job.cc
index 030f80a687afbe10bfe60e0c003a5ab9ad5c6eaa..1ab9a45f59bda87378c475e8975c4978f4bf1815 100644
--- a/android_webview/browser/net/android_stream_reader_url_request_job.cc
+++ b/android_webview/browser/net/android_stream_reader_url_request_job.cc
@@ -30,6 +30,40 @@ using base::android::AttachCurrentThread;
 using base::PostTaskAndReplyWithResult;
 using content::BrowserThread;
 
+// The requests posted to the worker thread might outlive the job.
+// Thread-safe ref counting is used to ensure that the data is still there

[joth, 2012/11/29 17:44:09]

nit: by 'the data' you specifically mean the InputStream object?

[mkosiba (inactive), 2012/11/29 18:54:11]

umm.. I mean the InputStream and InputStreamReader instances, will update
comment.

+// when the closure is run on the worker thread.
+class InputStreamReaderWrapper :
+    public base::RefCountedThreadSafe<InputStreamReaderWrapper> {

[joth, 2012/11/29 17:44:09]

URLRequestJob is already refcounted thread safe. Did you consider just adding
the methods below as private methods in AndroidStreamReaderURLRequestJob?

[mkosiba (inactive), 2012/11/29 18:54:11]

unfortunately URLRequestJob is only RefCounted (not threadsafe).

+ public:
+  InputStreamReaderWrapper(
+      scoped_ptr<InputStream> input_stream,
+      scoped_ptr<InputStreamReader> input_stream_reader)
+    : input_stream_(input_stream.Pass()),
+      input_stream_reader_(input_stream_reader.Pass()) {

[joth, 2012/11/29 17:44:09]

nit: indent initializers again

[mkosiba (inactive), 2012/11/29 18:54:11]

Done.

+    DCHECK(input_stream_);
+    DCHECK(input_stream_reader_);
+  }
+
+  android_webview::InputStream& input_stream() {
+    return *input_stream_;
+  }
+
+  int Seek(const net::HttpByteRange& byte_range) {
+    return input_stream_reader_->Seek(byte_range);
+  }
+
+  int ReadRawData(net::IOBuffer* buffer, int buffer_size) {
+    return input_stream_reader_->ReadRawData(buffer, buffer_size);
+  }
+ private:

[joth, 2012/11/29 17:44:09]

nit: \n before

[mkosiba (inactive), 2012/11/29 18:54:11]

Done.

+  friend class base::RefCountedThreadSafe<InputStreamReaderWrapper>;
+  ~InputStreamReaderWrapper() {}
+
+  scoped_ptr<android_webview::InputStream> input_stream_;
+  scoped_ptr<android_webview::InputStreamReader> input_stream_reader_;

[mnaganov (inactive), 2012/11/29 17:05:22]

nit: please add DISALLOW_COPY_AND_ASSIGN (sorry for not pointing this out at the
first review)

[mkosiba (inactive), 2012/11/29 18:54:11]

Done.

+};
+
 AndroidStreamReaderURLRequestJob::AndroidStreamReaderURLRequestJob(
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate,
@@ -46,7 +80,8 @@ AndroidStreamReaderURLRequestJob::~AndroidStreamReaderURLRequestJob() {
 void AndroidStreamReaderURLRequestJob::Start() {
   // Start reading asynchronously so that all error reporting and data
   // callbacks happen as they would for network requests.
-  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING,
+                                  net::ERR_IO_PENDING));
   MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(
@@ -59,9 +94,9 @@ void AndroidStreamReaderURLRequestJob::Kill() {
   URLRequestJob::Kill();
 }
 
-scoped_refptr<InputStreamReader>
+scoped_ptr<InputStreamReader>
 AndroidStreamReaderURLRequestJob::CreateStreamReader(InputStream* stream) {
-  return make_scoped_refptr(new InputStreamReader(stream));
+  return make_scoped_ptr(new InputStreamReader(stream));
 }
 
 void AndroidStreamReaderURLRequestJob::StartAsync() {
@@ -70,27 +105,34 @@ void AndroidStreamReaderURLRequestJob::StartAsync() {
 
   // This could be done in the InputStreamReader but would force more
   // complex synchronization in the delegate.
-  stream_ = delegate_->OpenInputStream(env, request());
-  if (!stream_) {
-    NotifyDone(
-        net::URLRequestStatus(net::URLRequestStatus::FAILED, net::ERR_FAILED));
+  scoped_ptr<android_webview::InputStream> stream(
+      delegate_->OpenInputStream(env, request()));
+
+  if (!stream) {
+    NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED,
+                                     net::ERR_FAILED));
     return;
   }
 
-  DCHECK(!input_stream_reader_);
-  input_stream_reader_ = CreateStreamReader(stream_.get());
-  CHECK(input_stream_reader_);
+  scoped_ptr<InputStreamReader> input_stream_reader(
+      CreateStreamReader(stream.get()));
+  DCHECK(input_stream_reader);
+
+  DCHECK(!input_stream_reader_wrapper_);
+  input_stream_reader_wrapper_ =
+      new InputStreamReaderWrapper(stream.Pass(), input_stream_reader.Pass());
 
   PostTaskAndReplyWithResult(
       GetWorkerThreadRunner(),
       FROM_HERE,
-      base::Bind(&InputStreamReader::Seek, input_stream_reader_, byte_range_),
+      base::Bind(&InputStreamReaderWrapper::Seek,
+                 input_stream_reader_wrapper_,
+                 byte_range_),
       base::Bind(&AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted,
                  weak_factory_.GetWeakPtr()));
 }
 
-void AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted(
-    int result) {
+void AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted(int result) {
   // Clear the IO_PENDING status set in Start().
   SetStatus(net::URLRequestStatus());
   if (result >= 0) {
@@ -101,8 +143,7 @@ void AndroidStreamReaderURLRequestJob::OnReaderSeekCompleted(
   }
 }
 
-void AndroidStreamReaderURLRequestJob::OnReaderReadCompleted(
-    int result) {
+void AndroidStreamReaderURLRequestJob::OnReaderReadCompleted(int result) {
   // The URLRequest API contract requires that:
   // * NotifyDone be called once, to set the status code, indicate the job is
   //   finished (there will be no further IO),
@@ -129,19 +170,20 @@ base::TaskRunner* AndroidStreamReaderURLRequestJob::GetWorkerThreadRunner() {
 bool AndroidStreamReaderURLRequestJob::ReadRawData(net::IOBuffer* dest,
                                                    int dest_size,
                                                    int* bytes_read) {
-  DCHECK(input_stream_reader_);
+  DCHECK(input_stream_reader_wrapper_);
 
   PostTaskAndReplyWithResult(
       GetWorkerThreadRunner(),
       FROM_HERE,
-      base::Bind(&InputStreamReader::ReadRawData,
-                 input_stream_reader_,
+      base::Bind(&InputStreamReaderWrapper::ReadRawData,
+                 input_stream_reader_wrapper_,
                  base::Unretained(dest),

[joth, 2012/11/29 17:44:09]

if the job may be killed before the task runs, how do we know that |dest| is
going to still be alive then too?

[mkosiba (inactive), 2012/11/29 18:54:11]

I was mislead by the docs on net::URLRequest which sort of imply that dest will
be valid until the job calls NotifyReadComplete. The docs on net::IOBuffer say
it's refcountedthreadsafe exactly for this reason so dropping the Unretained.

                  dest_size),
       base::Bind(&AndroidStreamReaderURLRequestJob::OnReaderReadCompleted,
                  weak_factory_.GetWeakPtr()));
 
-  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING,
+                                  net::ERR_IO_PENDING));
   return false;
 }
 
@@ -150,21 +192,30 @@ bool AndroidStreamReaderURLRequestJob::GetMimeType(
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
 
-  if (!stream_)
+  if (!input_stream_reader_wrapper_)
     return false;
 
-  return delegate_->GetMimeType(env, request(), *stream_, mime_type);
+  // Since it's possible for this call to alter the InputStream a
+  // Seek or ReadRawData operation running in the background is not permitted.
+  DCHECK(!request_->status().is_io_pending());
+
+  return delegate_->GetMimeType(
+      env, request(), input_stream_reader_wrapper_->input_stream(), mime_type);
 }
 
-bool AndroidStreamReaderURLRequestJob::GetCharset(
-    std::string* charset) {
+bool AndroidStreamReaderURLRequestJob::GetCharset(std::string* charset) {
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
 
-  if (!stream_)
+  if (!input_stream_reader_wrapper_)
     return false;
 
-  return delegate_->GetCharset(env, request(), *stream_, charset);
+  // Since it's possible for this call to alter the InputStream a
+  // Seek or ReadRawData operation running in the background is not permitted.
+  DCHECK(!request_->status().is_io_pending());
+
+  return delegate_->GetCharset(
+      env, request(), input_stream_reader_wrapper_->input_stream(), charset);
 }
 
 void AndroidStreamReaderURLRequestJob::SetExtraRequestHeaders(