[android_webview] Fix UAF in request interception code.

It was possible for any of the tasks posted by the
AndroidStreamReaderURLRequestJob to access the InterceptedRequestData
after the URLRequest owning that data structure was deleted
The fix is to make the newly created job's Delgate own the
InterceptedRequestData since the AndroidStreamReaderURLRequestJob takes
care to not delete the Delegate before all async tasks have finished.

BUG=internal b/11520856
TEST=AndroidWebViewTest
Android-only CL, trybots happy.
NOTRY=true

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@233937 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 108 insertions, 57 deletions

diff --git a/android_webview/browser/aw_request_interceptor.cc b/android_webview/browser/aw_request_interceptor.cc
index 0e5e283..72673d2 100644
--- a/android_webview/browser/aw_request_interceptor.cc
+++ b/android_webview/browser/aw_request_interceptor.cc
@@ -24,27 +24,7 @@ namespace android_webview {
 
 namespace {
 
-const void* kURLRequestUserDataKey = &kURLRequestUserDataKey;
-
-class URLRequestUserData : public base::SupportsUserData::Data {
- public:
-    URLRequestUserData(
-        scoped_ptr<InterceptedRequestData> intercepted_request_data)
-        : intercepted_request_data_(intercepted_request_data.Pass()) {
-    }
-
-    static URLRequestUserData* Get(net::URLRequest* request) {
-      return reinterpret_cast<URLRequestUserData*>(
-          request->GetUserData(kURLRequestUserDataKey));
-    }
-
-    const InterceptedRequestData* intercepted_request_data() const {
-      return intercepted_request_data_.get();
-    }
-
- private:
-  scoped_ptr<InterceptedRequestData> intercepted_request_data_;
-};
+const void* kRequestAlreadyQueriedDataKey = &kRequestAlreadyQueriedDataKey;
 
 } // namespace
 
@@ -82,26 +62,23 @@ net::URLRequestJob* AwRequestInterceptor::MaybeCreateJob(
   // request.
   // This is done not only for efficiency reasons, but also for correctness
   // as it is possible for the Interceptor chain to be invoked more than once
-  // (in which case we don't want to query the embedder multiple times).
-  URLRequestUserData* user_data = URLRequestUserData::Get(request);
-
-  if (!user_data) {
-    // To ensure we only query the embedder once, we rely on the fact that the
-    // user_data object will be created and attached to the URLRequest after a
-    // call to QueryForInterceptedRequestData is made (regardless of whether
-    // the result of that call is a valid InterceptedRequestData* pointer or
-    // NULL.
-    user_data = new URLRequestUserData(
-        QueryForInterceptedRequestData(request->url(), request));
-    request->SetUserData(kURLRequestUserDataKey, user_data);
-  }
-
-  const InterceptedRequestData* intercepted_request_data =
-      user_data->intercepted_request_data();
+  // in which case we don't want to query the embedder multiple times.
+  // Note: The Interceptor chain is not invoked more than once if we create a
+  // URLRequestJob in this method, so this is only caching negative hits.
+  if (request->GetUserData(kRequestAlreadyQueriedDataKey))
+    return NULL;
+  request->SetUserData(kRequestAlreadyQueriedDataKey,
+                       new base::SupportsUserData::Data());
+
+  scoped_ptr<InterceptedRequestData> intercepted_request_data =
+      QueryForInterceptedRequestData(request->url(), request);
 
   if (!intercepted_request_data)
     return NULL;
-  return intercepted_request_data->CreateJobFor(request, network_delegate);
+
+  // The newly created job will own the InterceptedRequestData.
+  return InterceptedRequestData::CreateJobFor(
+      intercepted_request_data.Pass(), request, network_delegate);
 }
 
 } // namespace android_webview
diff --git a/android_webview/browser/intercepted_request_data.h b/android_webview/browser/intercepted_request_data.h
index 672833c..c4e01c3 100644
--- a/android_webview/browser/intercepted_request_data.h
+++ b/android_webview/browser/intercepted_request_data.h
@@ -26,10 +26,11 @@ class InterceptedRequestData {
   // This creates a URLRequestJob for the |request| wich will read data from
   // the |intercepted_request_data| structure (instead of going to the network
   // or to the cache).
-  // The newly created job does not take ownership of |this|.
-  virtual net::URLRequestJob* CreateJobFor(
+  // The newly created job takes ownership of |intercepted_request_data|.
+  static net::URLRequestJob* CreateJobFor(
+      scoped_ptr<InterceptedRequestData> intercepted_request_data,
       net::URLRequest* request,
-      net::NetworkDelegate* network_delegate) const = 0;
+      net::NetworkDelegate* network_delegate);
 
  protected:
   InterceptedRequestData() {}
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java
index 9efd185..ba4ddb1 100644
--- a/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java
+++ b/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldInterceptRequestTest.java
@@ -25,7 +25,9 @@ import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
 import java.util.List;
 import java.util.Random;
 
@@ -255,6 +257,70 @@ public class AwContentsClientShouldInterceptRequestTest extends AwTestBase {
         mContentsClient.getOnPageFinishedHelper().waitForCallback(onPageFinishedCallCount);
     }
 
+    private static class SlowInterceptedRequestData extends InterceptedRequestData {
+        private CallbackHelper mReadStartedCallbackHelper = new CallbackHelper();
+        private CountDownLatch mLatch = new CountDownLatch(1);
+
+        public SlowInterceptedRequestData(String mimeType, String encoding, InputStream data) {
+            super(mimeType, encoding, data);
+        }
+
+        @Override
+        public InputStream getData() {
+            mReadStartedCallbackHelper.notifyCalled();
+            try {
+                mLatch.await();
+            } catch (InterruptedException e) {
+                // ignore
+            }
+            return super.getData();
+        }
+
+        public void unblockReads() {
+            mLatch.countDown();
+        }
+
+        public CallbackHelper getReadStartedCallbackHelper() {
+            return mReadStartedCallbackHelper;
+        }
+    }
+
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    public void testDoesNotCrashOnSlowStream() throws Throwable {
+        final String aboutPageUrl = addAboutPageToTestServer(mWebServer);
+        final String aboutPageData = makePageWithTitle("some title");
+        final String encoding = "UTF-8";
+        final SlowInterceptedRequestData slowInterceptedRequestData =
+            new SlowInterceptedRequestData("text/html", encoding,
+                    new ByteArrayInputStream(aboutPageData.getBytes(encoding)));
+
+        mShouldInterceptRequestHelper.setReturnValue(slowInterceptedRequestData);
+        int callCount = slowInterceptedRequestData.getReadStartedCallbackHelper().getCallCount();
+        loadUrlAsync(mAwContents, aboutPageUrl);
+        slowInterceptedRequestData.getReadStartedCallbackHelper().waitForCallback(callCount);
+
+        // Now the AwContents is "stuck" waiting for the SlowInputStream to finish reading so we
+        // delete it to make sure that the dangling 'read' task doesn't cause a crash. Unfortunately
+        // this will not always lead to a crash but it should happen often enough for us to notice.
+
+        runTestOnUiThread(new Runnable() {
+            @Override
+            public void run() {
+                getActivity().removeAllViews();
+            }
+        });
+        destroyAwContentsOnMainSync(mAwContents);
+        pollOnUiThread(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return AwContents.getNativeInstanceCount() == 0;
+            }
+        });
+
+        slowInterceptedRequestData.unblockReads();
+    }
+
     @SmallTest
     @Feature({"AndroidWebView"})
     public void testHttpStatusField() throws Throwable {
diff --git a/android_webview/native/intercepted_request_data_impl.cc b/android_webview/native/intercepted_request_data_impl.cc
index dc5fe22..03b9f42 100644
--- a/android_webview/native/intercepted_request_data_impl.cc
+++ b/android_webview/native/intercepted_request_data_impl.cc
@@ -22,8 +22,8 @@ class StreamReaderJobDelegateImpl :
     public AndroidStreamReaderURLRequestJob::Delegate {
  public:
     StreamReaderJobDelegateImpl(
-        const InterceptedRequestDataImpl* intercepted_request_data)
-        : intercepted_request_data_impl_(intercepted_request_data) {
+        scoped_ptr<InterceptedRequestDataImpl> intercepted_request_data)
+        : intercepted_request_data_impl_(intercepted_request_data.Pass()) {
       DCHECK(intercepted_request_data_impl_);
     }
 
@@ -53,11 +53,31 @@ class StreamReaderJobDelegateImpl :
     }
 
  private:
-    const InterceptedRequestDataImpl* intercepted_request_data_impl_;
+    scoped_ptr<InterceptedRequestDataImpl> intercepted_request_data_impl_;
 };
 
 } // namespace
 
+// static
+net::URLRequestJob* InterceptedRequestData::CreateJobFor(
+    scoped_ptr<InterceptedRequestData> intercepted_request_data,
+    net::URLRequest* request,
+    net::NetworkDelegate* network_delegate) {
+  DCHECK(intercepted_request_data);
+  DCHECK(request);
+  DCHECK(network_delegate);
+
+  return new AndroidStreamReaderURLRequestJob(
+      request,
+      network_delegate,
+      scoped_ptr<AndroidStreamReaderURLRequestJob::Delegate>(
+          new StreamReaderJobDelegateImpl(
+              // PassAs rightfully doesn't support downcasts.
+              scoped_ptr<InterceptedRequestDataImpl>(
+                  static_cast<InterceptedRequestDataImpl*>(
+                      intercepted_request_data.release())))));
+}
+
 InterceptedRequestDataImpl::InterceptedRequestDataImpl(
     const base::android::JavaRef<jobject>& obj)
   : java_object_(obj) {
@@ -99,13 +119,4 @@ bool RegisterInterceptedRequestData(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-net::URLRequestJob* InterceptedRequestDataImpl::CreateJobFor(
-    net::URLRequest* request,
-    net::NetworkDelegate* network_delegate) const {
-  scoped_ptr<AndroidStreamReaderURLRequestJob::Delegate>
-      stream_reader_job_delegate_impl(new StreamReaderJobDelegateImpl(this));
-  return new AndroidStreamReaderURLRequestJob(
-      request, network_delegate, stream_reader_job_delegate_impl.Pass());
-}
-
 } // namespace android_webview
diff --git a/android_webview/native/intercepted_request_data_impl.h b/android_webview/native/intercepted_request_data_impl.h
index 457bda9..0246569 100644
--- a/android_webview/native/intercepted_request_data_impl.h
+++ b/android_webview/native/intercepted_request_data_impl.h
@@ -25,10 +25,6 @@ class InterceptedRequestDataImpl : public InterceptedRequestData {
   virtual bool GetMimeType(JNIEnv* env, std::string* mime_type) const;
   virtual bool GetCharset(JNIEnv* env, std::string* charset) const;
 
-  virtual net::URLRequestJob* CreateJobFor(
-      net::URLRequest* request,
-      net::NetworkDelegate* network_delegate) const OVERRIDE;
-
  private:
   base::android::ScopedJavaGlobalRef<jobject> java_object_;