NULL Reference Error in ElementRuleCollector
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 6 Aug 2016 05:37:28 +0000 (05:37 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 6 Aug 2016 05:37:28 +0000 (05:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160362

Patch by Jonathan Bedard <jbedard@apple.com> on 2016-08-05
Reviewed by Darin Adler.

No new tests, existing CSS tests cover this change.

Undefined behavior sanitizer found a reference bound to a NULL pointer.
The root cause of this issue was a discrepancy between whether an author style needed to be defined.  In some logic, an undefined author style was considered acceptable, but in other logic, author style was always assumed to be defined.  To fix this, a variable was added so that while author style is always defined, there is a flag indicating if this definition occurred in the constructor for use by functions which allow an undefined author style.

* css/DocumentRuleSets.cpp:
(WebCore::DocumentRuleSets::DocumentRuleSets): Define author style by default.
(WebCore::DocumentRuleSets::resetAuthorStyle): Switch author style flag.
* css/DocumentRuleSets.h: Added author style flag, changed authorStyle accessor to reference from pointer.
* css/ElementRuleCollector.cpp:
(WebCore::ElementRuleCollector::ElementRuleCollector): Original location of undefined behavior.
(WebCore::ElementRuleCollector::matchHostPseudoClassRules): Changed pointer to reference.
(WebCore::ElementRuleCollector::matchSlottedPseudoElementRules): Changed pointer to reference.
* css/PageRuleCollector.cpp:
(WebCore::PageRuleCollector::matchAllPageRules): Check new flag, changed pointer to reference.

* css/StyleResolver.cpp: Changed pointer to reference.
* dom/Document.cpp: Dito.
* style/AttributeChangeInvalidation.cpp: Dito.
* style/ClassChangeInvalidation.cpp: Dito.
* style/IdChangeInvalidation.cpp: Dito.
* style/StyleSharingResolver.cpp: Dito.

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

12 files changed:
Source/WebCore/ChangeLog
Source/WebCore/css/DocumentRuleSets.cpp
Source/WebCore/css/DocumentRuleSets.h
Source/WebCore/css/ElementRuleCollector.cpp
Source/WebCore/css/PageRuleCollector.cpp
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/dom/AuthorStyleSheets.cpp
Source/WebCore/dom/Document.cpp
Source/WebCore/style/AttributeChangeInvalidation.cpp
Source/WebCore/style/ClassChangeInvalidation.cpp
Source/WebCore/style/IdChangeInvalidation.cpp
Source/WebCore/style/StyleSharingResolver.cpp

index cf2470c..b7afbd4 100644 (file)
@@ -1,3 +1,33 @@
+2016-08-05  Jonathan Bedard  <jbedard@apple.com>
+
+        NULL Reference Error in ElementRuleCollector
+        https://bugs.webkit.org/show_bug.cgi?id=160362
+
+        Reviewed by Darin Adler.
+
+        No new tests, existing CSS tests cover this change.
+
+        Undefined behavior sanitizer found a reference bound to a NULL pointer.
+        The root cause of this issue was a discrepancy between whether an author style needed to be defined.  In some logic, an undefined author style was considered acceptable, but in other logic, author style was always assumed to be defined.  To fix this, a variable was added so that while author style is always defined, there is a flag indicating if this definition occurred in the constructor for use by functions which allow an undefined author style.
+
+        * css/DocumentRuleSets.cpp:
+        (WebCore::DocumentRuleSets::DocumentRuleSets): Define author style by default.
+        (WebCore::DocumentRuleSets::resetAuthorStyle): Switch author style flag.
+        * css/DocumentRuleSets.h: Added author style flag, changed authorStyle accessor to reference from pointer.
+        * css/ElementRuleCollector.cpp:
+        (WebCore::ElementRuleCollector::ElementRuleCollector): Original location of undefined behavior.
+        (WebCore::ElementRuleCollector::matchHostPseudoClassRules): Changed pointer to reference.
+        (WebCore::ElementRuleCollector::matchSlottedPseudoElementRules): Changed pointer to reference.
+        * css/PageRuleCollector.cpp:
+        (WebCore::PageRuleCollector::matchAllPageRules): Check new flag, changed pointer to reference.
+
+        * css/StyleResolver.cpp: Changed pointer to reference.
+        * dom/Document.cpp: Dito.
+        * style/AttributeChangeInvalidation.cpp: Dito.
+        * style/ClassChangeInvalidation.cpp: Dito.
+        * style/IdChangeInvalidation.cpp: Dito.
+        * style/StyleSharingResolver.cpp: Dito.
+
 2016-08-05  Chris Dumez  <cdumez@apple.com>
 
         DOMException should be constructible
index 82a43b6..88cc43e 100644 (file)
@@ -39,6 +39,8 @@ namespace WebCore {
 
 DocumentRuleSets::DocumentRuleSets()
 {
+    m_authorStyle = std::make_unique<RuleSet>();
+    m_authorStyle->disableAutoShrinkToFit();
 }
 
 DocumentRuleSets::~DocumentRuleSets()
@@ -78,6 +80,7 @@ static std::unique_ptr<RuleSet> makeRuleSet(const Vector<RuleFeature>& rules)
 
 void DocumentRuleSets::resetAuthorStyle()
 {
+    m_isAuthorStyleDefined = true;
     m_authorStyle = std::make_unique<RuleSet>();
     m_authorStyle->disableAutoShrinkToFit();
 }
index 07c7d91..c0ad9f8 100644 (file)
@@ -44,7 +44,9 @@ class DocumentRuleSets {
 public:
     DocumentRuleSets();
     ~DocumentRuleSets();
-    RuleSet* authorStyle() const { return m_authorStyle.get(); }
+    
+    bool isAuthorStyleDefined() const { return m_isAuthorStyleDefined; }
+    RuleSet& authorStyle() const { return *m_authorStyle.get(); }
     RuleSet* userStyle() const { return m_userStyle.get(); }
     const RuleFeatureSet& features() const;
     RuleSet* sibling() const { return m_siblingRuleSet.get(); }
@@ -69,6 +71,7 @@ private:
     void collectFeatures() const;
     void collectRulesFromUserStyleSheets(const Vector<RefPtr<CSSStyleSheet>>&, RuleSet& userStyle, const MediaQueryEvaluator&, StyleResolver&);
 
+    bool m_isAuthorStyleDefined { false };
     std::unique_ptr<RuleSet> m_authorStyle;
     std::unique_ptr<RuleSet> m_userStyle;
 
index d5a936e..b8630c3 100644 (file)
@@ -82,7 +82,7 @@ public:
 
 ElementRuleCollector::ElementRuleCollector(const Element& element, const DocumentRuleSets& ruleSets, const SelectorFilter* selectorFilter)
     : m_element(element)
-    , m_authorStyle(*ruleSets.authorStyle())
+    , m_authorStyle(ruleSets.authorStyle())
     , m_userStyle(ruleSets.userStyle())
     , m_selectorFilter(selectorFilter)
 {
@@ -234,7 +234,7 @@ void ElementRuleCollector::matchHostPseudoClassRules(MatchRequest& matchRequest,
 
     matchRequest.treeContextOrdinal++;
 
-    auto& shadowAuthorStyle = *m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
+    auto& shadowAuthorStyle = m_element.shadowRoot()->styleResolver().ruleSets().authorStyle();
     auto& shadowHostRules = shadowAuthorStyle.hostPseudoClassRules();
     if (shadowHostRules.isEmpty())
         return;
@@ -265,12 +265,11 @@ void ElementRuleCollector::matchSlottedPseudoElementRules(MatchRequest& matchReq
 
         // In nested case the slot may itself be assigned to a slot. Collect ::slotted rules from all the nested trees.
         maybeSlotted = slot;
-        auto* shadowAuthorStyle = hostShadowRoot->styleResolver().ruleSets().authorStyle();
-        if (!shadowAuthorStyle)
+        if (!hostShadowRoot->styleResolver().ruleSets().isAuthorStyleDefined())
             continue;
         // Find out if there are any ::slotted rules in the shadow tree matching the current slot.
         // FIXME: This is really part of the slot style and could be cached when resolving it.
-        ElementRuleCollector collector(*slot, *shadowAuthorStyle, nullptr);
+        ElementRuleCollector collector(*slot, hostShadowRoot->styleResolver().ruleSets().authorStyle(), nullptr);
         auto slottedPseudoElementRules = collector.collectSlottedPseudoElementRulesForSlot(matchRequest.includeEmptyRules);
         if (!slottedPseudoElementRules)
             continue;
index f97c8e2..b82c887 100644 (file)
@@ -70,7 +70,8 @@ void PageRuleCollector::matchAllPageRules(int pageIndex)
     matchPageRules(CSSDefaultStyleSheets::defaultPrintStyle, isLeft, isFirst, page);
     matchPageRules(m_ruleSets.userStyle(), isLeft, isFirst, page);
     // Only consider the global author RuleSet for @page rules, as per the HTML5 spec.
-    matchPageRules(m_ruleSets.authorStyle(), isLeft, isFirst, page);
+    if (m_ruleSets.isAuthorStyleDefined())
+        matchPageRules(&m_ruleSets.authorStyle(), isLeft, isFirst, page);
 }
 
 void PageRuleCollector::matchPageRules(RuleSet* rules, bool isLeftPage, bool isFirstPage, const String& pageName)
index 5855f37..2d88c3a 100644 (file)
@@ -1061,10 +1061,10 @@ void StyleResolver::adjustRenderStyle(RenderStyle& style, const RenderStyle& par
 
 bool StyleResolver::checkRegionStyle(const Element* regionElement)
 {
-    unsigned rulesSize = m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().size();
+    unsigned rulesSize = m_ruleSets.authorStyle().regionSelectorsAndRuleSets().size();
     for (unsigned i = 0; i < rulesSize; ++i) {
-        ASSERT(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).ruleSet.get());
-        if (checkRegionSelector(m_ruleSets.authorStyle()->regionSelectorsAndRuleSets().at(i).selector, regionElement))
+        ASSERT(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).ruleSet.get());
+        if (checkRegionSelector(m_ruleSets.authorStyle().regionSelectorsAndRuleSets().at(i).selector, regionElement))
             return true;
     }
 
index e1ea95e..0e5d178 100644 (file)
@@ -357,9 +357,9 @@ void AuthorStyleSheets::updateStyleResolver(Vector<RefPtr<CSSStyleSheet>>& activ
     }
 
     userAgentShadowTreeStyleResolver.ruleSets().resetAuthorStyle();
-    auto& authorRuleSet = *styleResolver.ruleSets().authorStyle();
+    auto& authorRuleSet = styleResolver.ruleSets().authorStyle();
     if (authorRuleSet.hasShadowPseudoElementRules())
-        userAgentShadowTreeStyleResolver.ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(authorRuleSet);
+        userAgentShadowTreeStyleResolver.ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(authorRuleSet);
 }
 
 const Vector<RefPtr<CSSStyleSheet>> AuthorStyleSheets::activeStyleSheetsForInspector() const
index 5642f25..77612df 100644 (file)
@@ -2205,9 +2205,9 @@ StyleResolver& Document::userAgentShadowTreeStyleResolver()
         m_userAgentShadowTreeStyleResolver = std::make_unique<StyleResolver>(*this);
 
         // FIXME: Filter out shadow pseudo elements we don't want to expose to authors.
-        auto& documentAuthorStyle = *ensureStyleResolver().ruleSets().authorStyle();
+        auto& documentAuthorStyle = ensureStyleResolver().ruleSets().authorStyle();
         if (documentAuthorStyle.hasShadowPseudoElementRules())
-            m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle()->copyShadowPseudoElementRulesFrom(documentAuthorStyle);
+            m_userAgentShadowTreeStyleResolver->ruleSets().authorStyle().copyShadowPseudoElementRulesFrom(documentAuthorStyle);
     }
 
     return *m_userAgentShadowTreeStyleResolver;
index 2a7ea03..1f8bd5d 100644 (file)
@@ -38,7 +38,7 @@ namespace Style {
 static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, bool isHTML, const QualifiedName& attributeName)
 {
     auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
-    if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
         return false;
 
     auto& nameSet = isHTML ? shadowRuleSets.features().attributeCanonicalLocalNamesInRules : shadowRuleSets.features().attributeLocalNamesInRules;
@@ -68,7 +68,7 @@ void AttributeChangeInvalidation::invalidateStyle(const QualifiedName& attribute
         return;
     }
 
-    if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+    if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
         m_element.setNeedsStyleRecalc(FullStyleChange);
         return;
     }
index 48c5b18..de36c9a 100644 (file)
@@ -88,7 +88,7 @@ static ClassChangeVector computeClassChange(const SpaceSplitString& oldClasses,
 static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, AtomicStringImpl* changedClass)
 {
     auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
-    if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
         return false;
     return shadowRuleSets.features().classesInRules.contains(changedClass);
 }
@@ -114,7 +114,7 @@ void ClassChangeInvalidation::invalidateStyle(const SpaceSplitString& oldClasses
     if (changedClassesAffectingStyle.isEmpty())
         return;
 
-    if (shadowRoot && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+    if (shadowRoot && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
         m_element.setNeedsStyleRecalc(FullStyleChange);
         return;
     }
index b2d4ce4..ae11fbb 100644 (file)
@@ -37,7 +37,7 @@ namespace Style {
 static bool mayBeAffectedByHostStyle(ShadowRoot& shadowRoot, const AtomicString& changedId)
 {
     auto& shadowRuleSets = shadowRoot.styleResolver().ruleSets();
-    if (shadowRuleSets.authorStyle()->hostPseudoClassRules().isEmpty())
+    if (shadowRuleSets.authorStyle().hostPseudoClassRules().isEmpty())
         return false;
 
     return shadowRuleSets.features().idsInRules.contains(changedId.impl());
@@ -59,7 +59,7 @@ void IdChangeInvalidation::invalidateStyle(const AtomicString& changedId)
     if (!mayAffectStyle)
         return;
 
-    if (m_element.shadowRoot() && ruleSets.authorStyle()->hasShadowPseudoElementRules()) {
+    if (m_element.shadowRoot() && ruleSets.authorStyle().hasShadowPseudoElementRules()) {
         m_element.setNeedsStyleRecalc(FullStyleChange);
         return;
     }
index 5111a30..241012d 100644 (file)
@@ -97,7 +97,7 @@ std::unique_ptr<RenderStyle> SharingResolver::resolve(const Element& searchEleme
         return nullptr;
     if (elementHasDirectionAuto(element))
         return nullptr;
-    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
+    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
         return nullptr;
 
     Context context {
@@ -286,7 +286,7 @@ bool SharingResolver::canShareStyleWithElement(const Context& context, const Sty
     if (candidateElement.matchesDefaultPseudoClass() != element.matchesDefaultPseudoClass())
         return false;
 
-    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle()->hostPseudoClassRules().isEmpty())
+    if (element.shadowRoot() && !element.shadowRoot()->styleResolver().ruleSets().authorStyle().hostPseudoClassRules().isEmpty())
         return false;
 
 #if ENABLE(FULLSCREEN_API)