Crash loading local file with WebPageProxy::loadAlternateHTMLString

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

WebPageProxy::loadAlternateHTMLString needs to assume read access to the "unreachable file" as well as the file it actually loads, because the unreachable file will get added to the back/forward list, causing us to crash. There is already a cross-platform test for this but in that test the unreachable file is the same as the file with the alternate HTML, so it didn't catch this.

[Michael Catanzaro]

Created attachment 247050 [details]
Patch

[Michael Catanzaro]

Comment on attachment 247050 [details]
Patch

>Subversion Revision: 180216
>diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
>index 7eeba7a1a7bb215fb21f51bd74f15142a1a58cef..88ac00dbbed670c4a7b16fbfa5fa353f13ab1e7e 100644
>--- a/Source/WebKit2/ChangeLog
>+++ b/Source/WebKit2/ChangeLog
>@@ -1,3 +1,17 @@
>[phone]  Michael Catanzaro  <[e-mail]>
>+
>+        Crash loading local file with WebPageProxy::loadAlternateHTMLString
>+        https://bugs.webkit.org/show_bug.cgi?id=141867
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebPageProxy::loadAlternateHTMLString needs to assume read access to unreachableURL as well
>+        as baseURL, because unreachableURL will get added to the back/forward list, causing us to
>+        crash later on when we notice the unexpected URL received in checkURLReceivedFromWebProcess.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::loadAlternateHTMLString):
>+
> 2015-02-16  Carlos Garcia Campos  <[e-mail]>
> 
>         [GTK] WebKitFrame objects are never released
>diff --git a/Source/WebKit2/UIProcess/WebPageProxy.cpp b/Source/WebKit2/UIProcess/WebPageProxy.cpp
>index 07dad60edf2d83fd7ccd5cc2a903f2dbc19f152a..25f124f1e8c39c33c7391f53e7b21df9b175172a 100644
>--- a/Source/WebKit2/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit2/UIProcess/WebPageProxy.cpp
>@@ -922,6 +922,7 @@ void WebPageProxy::loadAlternateHTMLString(const String& htmlString, const Strin
>         m_mainFrame->setUnreachableURL(unreachableURL);
> 
>     m_process->assumeReadAccessToBaseURL(baseURL);
>+    m_process->assumeReadAccessToBaseURL(unreachableURL);
>     m_process->send(Messages::WebPage::LoadAlternateHTMLString(htmlString, baseURL, unreachableURL, UserData(process().transformObjectsToHandles(userData).get())), m_pageID);
>     m_process->responsivenessTimer()->start();
> }
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index d4cc5b036f1634021a0b9f2cdec93d85844f94e3..1cb0e842903d04d954e67e9cfc0947c510eff7d4 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,18 @@
>[phone]  Michael Catanzaro  <[e-mail]>
>+
>+        Crash loading local file with WebPageProxy::loadAlternateHTMLString
>+        https://bugs.webkit.org/show_bug.cgi?id=141867
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp:
>+        (TestWebKitAPI::loadAlternateHTMLString): Split most of this test into a function so it can
>+        be shared with the new test.
>+        (TestWebKitAPI::TEST): Add a cross-platform test for this crash.
>+        * TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp: Add a GTK+ test for this crash.
>+        (testLoadAlternateHTMLForLocalPage):
>+        (beforeAll):
>+
> 2015-02-17  Carlos Garcia Campos  <[e-mail]>
> 
>         Unreviewed. Fix /webkit2/WebKitDOMNode/dom-cache after r180214.
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp b/Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp
>index 73721dad06351a9b81166b581fedf9937099ad4e..668307a9a29bb84bb4cfa7b499bc79396e0fca5b 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2011 Apple Inc. All rights reserved.
>+ * Copyright (C) 2015 Igalia S.L.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -44,7 +45,7 @@ static void didFinishLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef us
>     didFinishLoad = true;
> }
> 
>-TEST(WebKit2, LoadAlternateHTMLStringWithNonDirectoryURL)
>+static void loadAlternateHTMLString(WKURLRef baseURL, WKURLRef unreachableURL)
> {
>     WKRetainPtr<WKContextRef> context(AdoptWK, WKContextCreate());
>     PlatformWebView webView(context.get());
>@@ -56,16 +57,29 @@ TEST(WebKit2, LoadAlternateHTMLStringWithNonDirectoryURL)
>     loaderClient.didFinishLoadForFrame = didFinishLoadForFrame;
>     WKPageSetPageLoaderClient(webView.page(), &loaderClient.base);
> 
>-    WKRetainPtr<WKURLRef> fileURL(AdoptWK, Util::createURLForResource("simple", "html"));
>     WKRetainPtr<WKStringRef> alternateHTMLString(AdoptWK, WKStringCreateWithUTF8CString("<html><body><img src='icon.png'></body></html>"));
>-    
>-    // Call WKPageLoadAlternateHTMLString() with fileURL which does not point to a directory
>-    WKPageLoadAlternateHTMLString(webView.page(), alternateHTMLString.get(), fileURL.get(), fileURL.get());
>-    
>+    WKPageLoadAlternateHTMLString(webView.page(), alternateHTMLString.get(), baseURL, unreachableURL);
>+
>     // If we can finish loading the html without resulting in an invalid message being sent from the WebProcess, this test passes.
>     Util::run(&didFinishLoad);
> }
> 
>+TEST(WebKit2, LoadAlternateHTMLStringWithNonDirectoryURL)
>+{
>+    // Call WKPageLoadAlternateHTMLString() with fileURL which does not point to a directory.
>+    WKRetainPtr<WKURLRef> fileURL(AdoptWK, Util::createURLForResource("simple", "html"));
>+    loadAlternateHTMLString(fileURL.get(), fileURL.get());
>+}
>+
>+TEST(WebKit2, LoadAlternateHTMLStringWithEmptyBaseURL)
>+{
>+    // Call WKPageLoadAlternateHTMLString() with empty baseURL to make sure this test works
>+    // when baseURL does not grant read access to the unreachableURL. We use a separate test
>+    // to ensure the previous test does not pollute the result.
>+    WKRetainPtr<WKURLRef> unreachableURL(AdoptWK, Util::URLForNonExistentResource());
>+    loadAlternateHTMLString(nullptr, unreachableURL.get());
>+}
>+
> } // namespace TestWebKitAPI
> 
> #endif
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp b/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp
>index e1856c7d43c17b9bad254ccfafd34ba57c623bc6..416fc95bfaf4d562832dd57f28ebd39ce965214e 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp
>@@ -82,6 +82,13 @@ static void testLoadAlternateHTML(LoadTrackingTest* test, gconstpointer)
>     assertNormalLoadHappened(test->m_loadEvents);
> }
> 
>+static void testLoadAlternateHTMLForLocalPage(LoadTrackingTest* test, gconstpointer)
>+{
>+    test->loadAlternateHTML("<html><body>Alternate page</body></html>", "file:///not/actually/loaded.html", 0);
>+    test->waitUntilLoadFinished();
>+    assertNormalLoadHappened(test->m_loadEvents);
>+}
>+
> static void testLoadPlainText(LoadTrackingTest* test, gconstpointer)
> {
>     test->loadPlainText("Hello WebKit-GTK+");
>@@ -477,6 +484,7 @@ void beforeAll()
>     LoadTrackingTest::add("WebKitWebView", "loading-error", testLoadingError);
>     LoadTrackingTest::add("WebKitWebView", "load-html", testLoadHtml);
>     LoadTrackingTest::add("WebKitWebView", "load-alternate-html", testLoadAlternateHTML);
>+    LoadTrackingTest::add("WebKitWebView", "load-alternate-html-for-local-page", testLoadAlternateHTMLForLocalPage);
>     LoadTrackingTest::add("WebKitWebView", "load-plain-text", testLoadPlainText);
>     LoadTrackingTest::add("WebKitWebView", "load-bytes", testLoadBytes);
>     LoadTrackingTest::add("WebKitWebView", "load-request", testLoadRequest);

[Michael Catanzaro]

Created attachment 247052 [details]
patch

So the "Edit Attachment As Comment" button doesn't edit the attachment, but it does leave a comment. OK....

[Alexey Proskuryakov]

Comment on attachment 247052 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247052&action=review

> Source/WebKit2/ChangeLog:10
> +        WebPageProxy::loadAlternateHTMLString needs to assume read access to unreachableURL as well
> +        as baseURL, because unreachableURL will get added to the back/forward list, causing us to
> +        crash later on when we notice the unexpected URL received in checkURLReceivedFromWebProcess.

What is the case for loadAlternateHTMLString being used without trying to load unreachableURL first? To know that it is unreachable, we need to try loading it first, which will assume the read access.

> Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp:77
> +    // when baseURL does not grant read access to the unreachableURL. We use a seperate test

Typo, should be "separate".

[Michael Catanzaro]

(In reply to comment #4)
> What is the case for loadAlternateHTMLString being used without trying to
> load unreachableURL first? To know that it is unreachable, we need to try
> loading it first, which will assume the read access.

In the GTK+ wrapper API we call this parameter "content_uri" rather than "unreachableURI" since the functionality is useful for more than just network error pages. In Epiphany we use it to load a blank HTML page in each web view when the browser starts, with the content_uri set to the page that will be loaded when the tab is switched to.

It's easy to fix this in the GTK+ wrapper function instead, if you prefer; I just figured you might prefer it changed here.

[Alexey Proskuryakov]

I'll defer to Sam and Anders - I don't fully understand the unreachableURL logic, and can't tell if that's the right way to do it. The patch seems safe though.

[Michael Catanzaro]

Created attachment 247130 [details]
Fixed the typo

[WebKit Commit Bot]

Comment on attachment 247130 [details]
Fixed the typo

Clearing flags on attachment: 247130

Committed r180565: <http://trac.webkit.org/changeset/180565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.