Merge r165821 - Mutating rules returned by getMatchedCSSRules can res…

…ult in crash https://bugs.webkit.org/show_bug.cgi?id=130209 Source/WebCore: Reviewed by Andreas Kling. The non-standard getMatchedCSSRules API returns CSSStyleRule objects that don't have parent stylesheet pointer (as we don't know which sheet the rule originated from). Mutating the rule via such wrapper can lead to crashes later as we fail to invalidate the underlying stylesheet. Fix by disallowing mutation of style rules that don't have parent sheet pointer. CSSStyleRule has two mutable properties selectorText and style. The latter gives back CSSStyleDeclaration. This patch disallows mutations in both cases for CSSStyleRules that don't have parent stylesheet pointer. While it is technically possible to have CSSRules that are legitimately disconnected from stylesheet (by removing rule from sheet while holding a reference to it) it never makes sense to mutate such rule as there is no way to do anything with it afterwards. Tests: fast/css/getMatchedCSSProperties-rule-mutation.html fast/css/getMatchedCSSRules-crash.html * css/CSSStyleRule.cpp: (WebCore::CSSStyleRule::setSelectorText): Bail out if parent stylesheet is null. * css/PropertySetCSSStyleDeclaration.cpp: (WebCore::PropertySetCSSStyleDeclaration::setCssText): (WebCore::PropertySetCSSStyleDeclaration::setProperty): (WebCore::PropertySetCSSStyleDeclaration::removeProperty): (WebCore::PropertySetCSSStyleDeclaration::setPropertyInternal): Allow StyleRuleCSSStyleDeclaration subclass cancel the mutation via boolean return value from willMutate. (WebCore::StyleRuleCSSStyleDeclaration::willMutate): Disallow mutation if the owning CSSStyleRule is null or has null stylesheet. (WebCore::StyleRuleCSSStyleDeclaration::didMutate): We never get here with null rule or stylesheet anymore. * css/PropertySetCSSStyleDeclaration.h: (WebCore::PropertySetCSSStyleDeclaration::willMutate): LayoutTests: Reviewed by Andreas Kling. * fast/css/getMatchedCSSProperties-rule-mutation-expected.txt: Added. * fast/css/getMatchedCSSProperties-rule-mutation.html: Added. * fast/css/getMatchedCSSRules-crash-expected.txt: Added. * fast/css/getMatchedCSSRules-crash.html: Added.
WebKit · Apr 30, 2014 · 6e4f449 · 6e4f449
1 parent 1e9d57f
commit 6e4f449
Show file tree

Hide file tree

Showing 9 changed files with 192 additions and 12 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2014-03-18  Antti Koivisto  <antti@apple.com>
+
+        Mutating rules returned by getMatchedCSSRules can result in crash
+        https://bugs.webkit.org/show_bug.cgi?id=130209
+
+        Reviewed by Andreas Kling.
+
+        * fast/css/getMatchedCSSProperties-rule-mutation-expected.txt: Added.
+        * fast/css/getMatchedCSSProperties-rule-mutation.html: Added.
+        * fast/css/getMatchedCSSRules-crash-expected.txt: Added.
+        * fast/css/getMatchedCSSRules-crash.html: Added.
+
 2014-03-08  Oliver Hunt  <oliver@apple.com>
 
         SerializedScriptValue may move Identifiers between worlds

diff --git a/LayoutTests/fast/css/getMatchedCSSProperties-rule-mutation-expected.txt b/LayoutTests/fast/css/getMatchedCSSProperties-rule-mutation-expected.txt
@@ -0,0 +1,20 @@
+Test that CSSStyleRules returned by getMatchedCSSRules can't be mutated
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS rules.length is 2
+PASS rules[0].selectorText is originalText
+PASS rules[1].selectorText is originalText
+PASS rules[0].style.cssText is originalText
+PASS rules[1].style.cssText is originalText
+PASS rules[0].style.color is originalText
+PASS rules[1].style.color is originalText
+PASS rules[0].style.getPropertyValue('color') is originalText
+PASS rules[1].style.getPropertyValue('color') is originalText
+PASS rules[0].style.color is originalText
+PASS rules[1].style.color is originalText
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/getMatchedCSSProperties-rule-mutation.html b/LayoutTests/fast/css/getMatchedCSSProperties-rule-mutation.html
@@ -0,0 +1,67 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+#test { color: blue; }
+@media all {
+    #test { color: blue; }
+}
+</style>
+</head>
+<body>
+<div id=test>
+</div>
+<script>
+
+description("Test that CSSStyleRules returned by getMatchedCSSRules can't be mutated");
+
+var testDiv = document.getElementById('test');
+var rules = getMatchedCSSRules(testDiv);
+
+function tryMutateSelector(index) {
+    originalText = rules[index].selectorText;
+    rules[index].selectorText = "#mutated";
+    shouldBe("rules["+index+"].selectorText", "originalText");
+}
+
+function tryMutateCSSText(index) {
+    originalText = rules[index].style.cssText;
+    rules[index].style.cssText = "color: red";
+    shouldBe("rules["+index+"].style.cssText", "originalText");
+}
+
+function tryMutateProperty(index) {
+    originalText = rules[index].style.color;
+    rules[index].style.color = "green";
+    shouldBe("rules["+index+"].style.color", "originalText");
+}
+
+function tryMutateProperty2(index) {
+    originalText = rules[index].style.getPropertyValue("color");
+    rules[index].style.setProperty("color", "white");
+    shouldBe("rules["+index+"].style.getPropertyValue('color')", "originalText");
+}
+
+function tryRemoveProperty(index) {
+    originalText = rules[index].style.color;
+    rules[index].style.removeProperty("color");
+    shouldBe("rules["+index+"].style.color", "originalText");
+}
+
+shouldBe("rules.length", "2");
+tryMutateSelector(0);
+tryMutateSelector(1);
+tryMutateCSSText(0);
+tryMutateCSSText(1);
+tryMutateProperty(0);
+tryMutateProperty(1);
+tryMutateProperty2(0);
+tryMutateProperty2(1);
+tryRemoveProperty(0);
+tryRemoveProperty(1);
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/css/getMatchedCSSRules-crash-expected.txt b/LayoutTests/fast/css/getMatchedCSSRules-crash-expected.txt
@@ -0,0 +1 @@
+This test passes if it doesn't crash. 
diff --git a/LayoutTests/fast/css/getMatchedCSSRules-crash.html b/LayoutTests/fast/css/getMatchedCSSRules-crash.html
@@ -0,0 +1,18 @@
+<html>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+<style>html,tr,img, table,media,body, li, em:nth-child(5){
+    height: 500px
+}
+</style>
+<script>
+function load() {
+    var cssRules = window.getMatchedCSSRules(document.documentElement);
+    cssRules[0].selectorText = 'a,td';
+}
+</script>
+This test passes if it doesn't crash.
+<iframe onload=load()>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,52 @@
+2014-03-18  Antti Koivisto  <antti@apple.com>
+
+        Mutating rules returned by getMatchedCSSRules can result in crash
+        https://bugs.webkit.org/show_bug.cgi?id=130209
+
+        Reviewed by Andreas Kling.
+
+        The non-standard getMatchedCSSRules API returns CSSStyleRule objects that don't
+        have parent stylesheet pointer (as we don't know which sheet the rule originated from).
+        Mutating the rule via such wrapper can lead to crashes later as we fail to invalidate
+        the underlying stylesheet.
+
+        Fix by disallowing mutation of style rules that don't have parent sheet pointer. CSSStyleRule
+        has two mutable properties selectorText and style. The latter gives back CSSStyleDeclaration.
+        This patch disallows mutations in both cases for CSSStyleRules that don't have parent stylesheet
+        pointer.
+
+        While it is technically possible to have CSSRules that are legitimately disconnected
+        from stylesheet (by removing rule from sheet while holding a reference to it) it never
+        makes sense to mutate such rule as there is no way to do anything with it afterwards.
+
+        Tests: fast/css/getMatchedCSSProperties-rule-mutation.html
+               fast/css/getMatchedCSSRules-crash.html
+
+        * css/CSSStyleRule.cpp:
+        (WebCore::CSSStyleRule::setSelectorText):
+
+            Bail out if parent stylesheet is null.
+
+        * css/PropertySetCSSStyleDeclaration.cpp:
+        (WebCore::PropertySetCSSStyleDeclaration::setCssText):
+        (WebCore::PropertySetCSSStyleDeclaration::setProperty):
+        (WebCore::PropertySetCSSStyleDeclaration::removeProperty):
+        (WebCore::PropertySetCSSStyleDeclaration::setPropertyInternal):
+
+            Allow StyleRuleCSSStyleDeclaration subclass cancel the mutation via
+            boolean return value from willMutate.
+
+        (WebCore::StyleRuleCSSStyleDeclaration::willMutate):
+
+            Disallow mutation if the owning CSSStyleRule is null or has null stylesheet.
+
+        (WebCore::StyleRuleCSSStyleDeclaration::didMutate):
+
+            We never get here with null rule or stylesheet anymore.
+
+        * css/PropertySetCSSStyleDeclaration.h:
+        (WebCore::PropertySetCSSStyleDeclaration::willMutate):
+
 2014-03-08  Oliver Hunt  <oliver@apple.com>
 
         SerializedScriptValue may move Identifiers between worlds

diff --git a/Source/WebCore/css/CSSStyleRule.cpp b/Source/WebCore/css/CSSStyleRule.cpp
@@ -93,6 +93,11 @@ String CSSStyleRule::selectorText() const
 
 void CSSStyleRule::setSelectorText(const String& selectorText)
 {
+    // FIXME: getMatchedCSSRules can return CSSStyleRules that are missing parent stylesheet pointer while
+    // referencing StyleRules that are part of stylesheet. Disallow mutations in this case.
+    if (!parentStyleSheet())
+        return;
+
     CSSParser p(parserContext());
     CSSSelectorList selectorList;
     p.parseSelector(selectorText, selectorList);

diff --git a/Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp b/Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp
@@ -150,7 +150,8 @@ String PropertySetCSSStyleDeclaration::cssText() const
 void PropertySetCSSStyleDeclaration::setCssText(const String& text, ExceptionCode& ec)
 {
     StyleAttributeMutationScope mutationScope(this);
-    willMutate();
+    if (!willMutate())
+        return;
 
     ec = 0;
     // FIXME: Detect syntax errors and set ec.
@@ -208,9 +209,10 @@ void PropertySetCSSStyleDeclaration::setProperty(const String& propertyName, con
     if (!propertyID)
         return;
 
-    bool important = priority.find("important", 0, false) != notFound;
+    if (!willMutate())
+        return;
 
-    willMutate();
+    bool important = priority.find("important", 0, false) != notFound;
 
     ec = 0;
     bool changed = m_propertySet->setProperty(propertyID, value, important, contextStyleSheet());
@@ -231,7 +233,8 @@ String PropertySetCSSStyleDeclaration::removeProperty(const String& propertyName
     if (!propertyID)
         return String();
 
-    willMutate();
+    if (!willMutate())
+        return String();
 
     ec = 0;
     String result;
@@ -257,7 +260,8 @@ String PropertySetCSSStyleDeclaration::getPropertyValueInternal(CSSPropertyID pr
 void PropertySetCSSStyleDeclaration::setPropertyInternal(CSSPropertyID propertyID, const String& value, bool important, ExceptionCode& ec)
 { 
     StyleAttributeMutationScope mutationScope(this);
-    willMutate();
+    if (!willMutate())
+        return;
 
     ec = 0;
     bool changed = m_propertySet->setProperty(propertyID, value, important, contextStyleSheet());
@@ -320,20 +324,24 @@ void StyleRuleCSSStyleDeclaration::deref()
         delete this;
 }
 
-void StyleRuleCSSStyleDeclaration::willMutate()
+bool StyleRuleCSSStyleDeclaration::willMutate()
 {
-    if (m_parentRule && m_parentRule->parentStyleSheet())
-        m_parentRule->parentStyleSheet()->willMutateRules();
+    if (!m_parentRule || !m_parentRule->parentStyleSheet())
+        return false;
+    m_parentRule->parentStyleSheet()->willMutateRules();
+    return true;
 }
 
 void StyleRuleCSSStyleDeclaration::didMutate(MutationType type)
 {
+    ASSERT(m_parentRule);
+    ASSERT(m_parentRule->parentStyleSheet());
+
     if (type == PropertyChanged)
         m_cssomCSSValueClones.clear();
 
     // Style sheet mutation needs to be signaled even if the change failed. willMutate*/didMutate* must pair.
-    if (m_parentRule && m_parentRule->parentStyleSheet())
-        m_parentRule->parentStyleSheet()->didMutateRuleFromCSSStyleDeclaration();
+    m_parentRule->parentStyleSheet()->didMutateRuleFromCSSStyleDeclaration();
 }
 
 CSSStyleSheet* StyleRuleCSSStyleDeclaration::parentStyleSheet() const

diff --git a/Source/WebCore/css/PropertySetCSSStyleDeclaration.h b/Source/WebCore/css/PropertySetCSSStyleDeclaration.h
@@ -74,7 +74,7 @@ class PropertySetCSSStyleDeclaration : public CSSStyleDeclaration {
 
 protected:
     enum MutationType { NoChanges, PropertyChanged };
-    virtual void willMutate() { }
+    virtual bool willMutate() WARN_UNUSED_RETURN { return true; }
     virtual void didMutate(MutationType) { }
 
     MutableStylePropertySet* m_propertySet;
@@ -104,7 +104,7 @@ class StyleRuleCSSStyleDeclaration : public PropertySetCSSStyleDeclaration
 
     virtual CSSRule* parentRule() const OVERRIDE { return m_parentRule;  }
 
-    virtual void willMutate() OVERRIDE;
+    virtual bool willMutate() OVERRIDE WARN_UNUSED_RETURN;
     virtual void didMutate(MutationType) OVERRIDE;
 
     unsigned m_refCount;