ASSERTION FAILED: url.containsOnlyASCII() in WebCore::checkEncodedString() when parsi...
authoraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 3 Oct 2016 20:27:33 +0000 (20:27 +0000)
committeraestes@apple.com <aestes@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 3 Oct 2016 20:27:33 +0000 (20:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162763
<rdar://problem/28572758>

Reviewed by Youenn Fablet.

Source/WebCore:

CSSCursorImageValue copies the URL of its underlying CSSImageValue by using the
ParsedURLString URL constructor on the String returned by CSSImageValue::url(). While
CSSImageValues were always being constructed from a URL implicitly converted to a String,
nothing ensured that the URL was valid. For invalid URLs, URL::string() returns the string
it was constructed with, which might still represent a relative URL or contain non-ASCII
characters, violating the preconditions of the ParsedURLString URL constructor and causing
an assertion to fail in Debug builds.

Fix this by having CSSImageValue store its image URL using a WebCore::URL rather than a
String. CSSCursorImageValue can then copy this URL instead of attempting to re-parse a
possibly-invalid URL string.

Test: fast/css/cursor-with-invalid-url.html

* css/CSSCursorImageValue.cpp:
(WebCore::CSSCursorImageValue::CSSCursorImageValue): Copied m_imageValue.url() into
m_originalURL instead of using the ParsedURLString URL constructor, since
CSSImageValue::url() now returns a WebCore::URL.
(WebCore::CSSCursorImageValue::loadImage): Created a URL from cursorElement->href() by
calling Document::completeURL().
* css/CSSImageValue.cpp:
(WebCore::CSSImageValue::CSSImageValue): Changed to take a URL&& instead of a const String&.
(WebCore::CSSImageValue::loadImage): Stopped calling Document::completeURL(), since m_url is
now a WebCore::URL.
* css/CSSImageValue.h: Changed url() to return a const URL&, and changed m_url to be a URL.
* html/HTMLBodyElement.cpp:
(WebCore::HTMLBodyElement::collectStyleForPresentationAttribute): Removed a call to
URL::string().
* html/HTMLTableElement.cpp:
(WebCore::HTMLTableElement::collectStyleForPresentationAttribute): Ditto.
* html/HTMLTablePartElement.cpp:
(WebCore::HTMLTablePartElement::collectStyleForPresentationAttribute): Ditto.

LayoutTests:

* fast/css/cursor-with-invalid-url.html: Added.
* fast/css/cursor-with-invalid-url-expected.txt: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@206744 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/cursor-with-invalid-url-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/cursor-with-invalid-url.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSCursorImageValue.cpp
Source/WebCore/css/CSSImageValue.cpp
Source/WebCore/css/CSSImageValue.h
Source/WebCore/html/HTMLBodyElement.cpp
Source/WebCore/html/HTMLTableElement.cpp
Source/WebCore/html/HTMLTablePartElement.cpp

index c0280cf..62b72ee 100644 (file)
@@ -1,5 +1,16 @@
 2016-10-03  Andy Estes  <aestes@apple.com>
 
+        ASSERTION FAILED: url.containsOnlyASCII() in WebCore::checkEncodedString() when parsing an invalid CSS cursor URL
+        https://bugs.webkit.org/show_bug.cgi?id=162763
+        <rdar://problem/28572758>
+
+        Reviewed by Youenn Fablet.
+
+        * fast/css/cursor-with-invalid-url.html: Added.
+        * fast/css/cursor-with-invalid-url-expected.txt: Added.
+
+2016-10-03  Andy Estes  <aestes@apple.com>
+
         ASSERTION FAILED: result in WebCore::CSSParser::parseURI
         https://bugs.webkit.org/show_bug.cgi?id=141638
         <rdar://problem/27709952>
diff --git a/LayoutTests/fast/css/cursor-with-invalid-url-expected.txt b/LayoutTests/fast/css/cursor-with-invalid-url-expected.txt
new file mode 100644 (file)
index 0000000..433a38b
--- /dev/null
@@ -0,0 +1 @@
+Test that a cursor with an invalid CSS URL does not trigger an assertion in a Debug build.
diff --git a/LayoutTests/fast/css/cursor-with-invalid-url.html b/LayoutTests/fast/css/cursor-with-invalid-url.html
new file mode 100644 (file)
index 0000000..1467fe7
--- /dev/null
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.dumpAsText();
+    </script>
+    <style>
+        body {
+            cursor: url(scheme://host:80\ff);
+        }
+    </style>
+</head>
+<body>
+Test that a cursor with an invalid CSS URL does not trigger an assertion in a Debug build.
+</body>
+</html>
index 2be8631..d6fb353 100644 (file)
@@ -1,3 +1,44 @@
+2016-10-03  Andy Estes  <aestes@apple.com>
+
+        ASSERTION FAILED: url.containsOnlyASCII() in WebCore::checkEncodedString() when parsing an invalid CSS cursor URL
+        https://bugs.webkit.org/show_bug.cgi?id=162763
+        <rdar://problem/28572758>
+
+        Reviewed by Youenn Fablet.
+
+        CSSCursorImageValue copies the URL of its underlying CSSImageValue by using the
+        ParsedURLString URL constructor on the String returned by CSSImageValue::url(). While
+        CSSImageValues were always being constructed from a URL implicitly converted to a String,
+        nothing ensured that the URL was valid. For invalid URLs, URL::string() returns the string
+        it was constructed with, which might still represent a relative URL or contain non-ASCII
+        characters, violating the preconditions of the ParsedURLString URL constructor and causing
+        an assertion to fail in Debug builds.
+
+        Fix this by having CSSImageValue store its image URL using a WebCore::URL rather than a
+        String. CSSCursorImageValue can then copy this URL instead of attempting to re-parse a
+        possibly-invalid URL string.
+
+        Test: fast/css/cursor-with-invalid-url.html
+
+        * css/CSSCursorImageValue.cpp:
+        (WebCore::CSSCursorImageValue::CSSCursorImageValue): Copied m_imageValue.url() into
+        m_originalURL instead of using the ParsedURLString URL constructor, since
+        CSSImageValue::url() now returns a WebCore::URL.
+        (WebCore::CSSCursorImageValue::loadImage): Created a URL from cursorElement->href() by
+        calling Document::completeURL().
+        * css/CSSImageValue.cpp:
+        (WebCore::CSSImageValue::CSSImageValue): Changed to take a URL&& instead of a const String&.
+        (WebCore::CSSImageValue::loadImage): Stopped calling Document::completeURL(), since m_url is
+        now a WebCore::URL.
+        * css/CSSImageValue.h: Changed url() to return a const URL&, and changed m_url to be a URL.
+        * html/HTMLBodyElement.cpp:
+        (WebCore::HTMLBodyElement::collectStyleForPresentationAttribute): Removed a call to
+        URL::string().
+        * html/HTMLTableElement.cpp:
+        (WebCore::HTMLTableElement::collectStyleForPresentationAttribute): Ditto.
+        * html/HTMLTablePartElement.cpp:
+        (WebCore::HTMLTablePartElement::collectStyleForPresentationAttribute): Ditto.
+
 2016-10-03  Zalan Bujtas  <zalan@apple.com>
 
         Log an error to stderr when FrameView::layout() fails to clean all the renderers.
index 222351f..1a09202 100644 (file)
@@ -44,7 +44,7 @@ CSSCursorImageValue::CSSCursorImageValue(Ref<CSSValue>&& imageValue, bool hasHot
     , m_hotSpot(hotSpot)
 {
     if (is<CSSImageValue>(m_imageValue.get()))
-        m_originalURL = { ParsedURLString, downcast<CSSImageValue>(m_imageValue.get()).url() };
+        m_originalURL = downcast<CSSImageValue>(m_imageValue.get()).url();
 }
 
 CSSCursorImageValue::~CSSCursorImageValue()
@@ -107,7 +107,7 @@ std::pair<CachedImage*, float> CSSCursorImageValue::loadImage(CachedResourceLoad
 
     if (auto* cursorElement = updateCursorElement(*loader.document())) {
         if (cursorElement->href() != downcast<CSSImageValue>(m_imageValue.get()).url())
-            m_imageValue = CSSImageValue::create(cursorElement->href());
+            m_imageValue = CSSImageValue::create(loader.document()->completeURL(cursorElement->href()));
     }
 
     return { downcast<CSSImageValue>(m_imageValue.get()).loadImage(loader, options), 1 };
index 254bb1a..9dd8ef2 100644 (file)
@@ -35,9 +35,9 @@
 
 namespace WebCore {
 
-CSSImageValue::CSSImageValue(const String& url)
+CSSImageValue::CSSImageValue(URL&& url)
     : CSSValue(ImageClass)
-    , m_url(url)
+    , m_url(WTFMove(url))
     , m_accessedImage(false)
 {
 }
@@ -65,7 +65,7 @@ CachedImage* CSSImageValue::loadImage(CachedResourceLoader& loader, const Resour
     if (!m_accessedImage) {
         m_accessedImage = true;
 
-        CachedResourceRequest request(ResourceRequest(loader.document()->completeURL(m_url)), options);
+        CachedResourceRequest request(ResourceRequest(m_url), options);
         if (m_initiatorName.isEmpty())
             request.setInitiator(cachedResourceRequestInitiators().css);
         else
index 8b8076e..6de6c74 100644 (file)
  * Boston, MA 02110-1301, USA.
  */
 
-#ifndef CSSImageValue_h
-#define CSSImageValue_h
+#pragma once
 
 #include "CSSValue.h"
 #include "CachedResourceHandle.h"
-#include <wtf/RefPtr.h>
+#include <wtf/Ref.h>
 
 namespace WebCore {
 
 class CachedImage;
 class CachedResourceLoader;
-class Element;
 class RenderElement;
 struct ResourceLoaderOptions;
 
 class CSSImageValue final : public CSSValue {
 public:
-    static Ref<CSSImageValue> create(const String& url) { return adoptRef(*new CSSImageValue(url)); }
+    static Ref<CSSImageValue> create(URL&& url) { return adoptRef(*new CSSImageValue(WTFMove(url))); }
     static Ref<CSSImageValue> create(CachedImage& image) { return adoptRef(*new CSSImageValue(image)); }
     ~CSSImageValue();
 
@@ -43,7 +41,7 @@ public:
     CachedImage* loadImage(CachedResourceLoader&, const ResourceLoaderOptions&);
     CachedImage* cachedImage() const { return m_cachedImage.get(); }
 
-    const String& url() const { return m_url; }
+    const URL& url() const { return m_url; }
 
     String customCSSText() const;
 
@@ -58,10 +56,10 @@ public:
     void setInitiator(const AtomicString& name) { m_initiatorName = name; }
 
 private:
-    explicit CSSImageValue(const String& url);
+    explicit CSSImageValue(URL&&);
     explicit CSSImageValue(CachedImage&);
 
-    String m_url;
+    URL m_url;
     CachedResourceHandle<CachedImage> m_cachedImage;
     bool m_accessedImage;
     AtomicString m_initiatorName;
@@ -70,5 +68,3 @@ private:
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSImageValue, isImageValue())
-
-#endif // CSSImageValue_h
index f72818b..1da084c 100644 (file)
@@ -82,7 +82,7 @@ void HTMLBodyElement::collectStyleForPresentationAttribute(const QualifiedName&
     if (name == backgroundAttr) {
         String url = stripLeadingAndTrailingHTMLSpaces(value);
         if (!url.isEmpty()) {
-            auto imageValue = CSSImageValue::create(document().completeURL(url).string());
+            auto imageValue = CSSImageValue::create(document().completeURL(url));
             imageValue.get().setInitiator(localName());
             style.setProperty(CSSProperty(CSSPropertyBackgroundImage, WTFMove(imageValue)));
         }
index ff9b036..9940029 100644 (file)
@@ -332,7 +332,7 @@ void HTMLTableElement::collectStyleForPresentationAttribute(const QualifiedName&
     else if (name == backgroundAttr) {
         String url = stripLeadingAndTrailingHTMLSpaces(value);
         if (!url.isEmpty())
-            style.setProperty(CSSProperty(CSSPropertyBackgroundImage, CSSImageValue::create(document().completeURL(url).string())));
+            style.setProperty(CSSProperty(CSSPropertyBackgroundImage, CSSImageValue::create(document().completeURL(url))));
     } else if (name == valignAttr) {
         if (!value.isEmpty())
             addPropertyToPresentationAttributeStyle(style, CSSPropertyVerticalAlign, value);
index 9bc5bda..007182c 100644 (file)
@@ -52,7 +52,7 @@ void HTMLTablePartElement::collectStyleForPresentationAttribute(const QualifiedN
     else if (name == backgroundAttr) {
         String url = stripLeadingAndTrailingHTMLSpaces(value);
         if (!url.isEmpty())
-            style.setProperty(CSSProperty(CSSPropertyBackgroundImage, CSSImageValue::create(document().completeURL(url).string())));
+            style.setProperty(CSSProperty(CSSPropertyBackgroundImage, CSSImageValue::create(document().completeURL(url))));
     } else if (name == valignAttr) {
         if (equalLettersIgnoringASCIICase(value, "top"))
             addPropertyToPresentationAttributeStyle(style, CSSPropertyVerticalAlign, CSSValueTop);