[android_webview] Fix UAF in request interception code.
Cherry-pick http://crrev.com/61653004
> 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
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233937
BUG: 11520856
Change-Id: I2063f9e13aecd24091d5f5d9361473b3656fd752
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 {
-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 @@
// 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);
+ // 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());
- 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();
+ 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 @@
// 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 8e4cef5..83db1db 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.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 @@
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 @@
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 @@
}
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 @@
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 @@
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_;
