Changeset 225878 in webkit

Timestamp:

Dec 13, 2017 4:05:42 PM (6 years ago)

Author:

rniwa@webkit.org

Message:

Crash inside ImageLoader::updateFromElement()
​https://bugs.webkit.org/show_bug.cgi?id=180769
<rdar://problem/35278782>

Reviewed by Antti Koivisto.

Fixed the crash by moving all call sites of ImageLoader::updateFromElement() to be post insertion callbacks
where it's safe to execute arbitrary scripts.

No new test since existing tests cover this with a newly added release assert in ImageLoader.

• html/HTMLImageElement.cpp:

(WebCore::HTMLImageElement::insertedIntoAncestor):
(WebCore::HTMLImageElement::didFinishInsertingNode): Extracted from insertedIntoAncestor to call
selectImageSource or updateFromElement.

• html/HTMLImageElement.h: Made many member functions final.
• html/HTMLInputElement.cpp:

(WebCore::HTMLInputElement::didAttachRenderers): Delay the call to ImageLoader::updateFromElement() in
ImageInputType using a post style resolution callback.

• html/HTMLMetaElement.h:
• html/HTMLPictureElement.cpp:

(WebCore::HTMLPictureElement::sourcesChanged): Store the list of child image elements into a vector before
calling selectImageSource since each call may execute arbitrary scripts.

• html/HTMLSourceElement.cpp:

(WebCore::HTMLSourceElement::insertedIntoAncestor): Delay the call to ImageLoader::updateFromElement()
using a post style resolution callback.
(WebCore::HTMLSourceElement::didFinishInsertingNode): Extracted from insertedIntoAncestor.

• html/HTMLSourceElement.h:
• html/HTMLVideoElement.cpp:

(WebCore::HTMLVideoElement::didAttachRenderers):
(WebCore::HTMLVideoElement::updateAfterStyleResolution): Extracted from didAttachRenderers.

• html/HTMLVideoElement.h:
• html/ImageInputType.cpp:

(WebCore::ImageInputType::needsPostStyleResolutionCallback): Added. Returns true so that HTMLInputElement's
didAttachRenderers would register a post style resolution callback.
(WebCore::ImageInputType::updateAfterStyleResolution): Extracted from attach.
(WebCore::ImageInputType::attach): Deleted.

• html/ImageInputType.h:
• html/InputType.cpp:

(WebCore::InputType::needsPostStyleResolutionCallback): Added. All but ImageInputType returns false.
(WebCore::InputType::updateAfterStyleResolution): Added.
(WebCore::InputType::attach): Deleted.

• html/InputType.h:
• loader/ImageLoader.cpp:

(WebCore::ImageLoader::updateFromElement): Added a release assertion. There is no direct security implication
so there is no need to use RELEASE_ASSERT_WITH_SECURITY_IMPLICATION here.

• svg/SVGImageElement.cpp:

(WebCore::SVGImageElement::insertedIntoAncestor):
(WebCore::SVGImageElement::didFinishInsertingNode):

• svg/SVGImageElement.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTMLImageElement.cpp (modified) (4 diffs)
• html/HTMLImageElement.h (modified) (3 diffs)
• html/HTMLInputElement.cpp (modified) (2 diffs)
• html/HTMLMetaElement.h (modified) (1 diff)
• html/HTMLPictureElement.cpp (modified) (1 diff)
• html/HTMLSourceElement.cpp (modified) (1 diff)
• html/HTMLSourceElement.h (modified) (1 diff)
• html/HTMLVideoElement.cpp (modified) (2 diffs)
• html/HTMLVideoElement.h (modified) (1 diff)
• html/ImageInputType.cpp (modified) (1 diff)
• html/ImageInputType.h (modified) (1 diff)
• html/InputType.cpp (modified) (1 diff)
• html/InputType.h (modified) (1 diff)
• loader/ImageLoader.cpp (modified) (2 diffs)
• svg/SVGImageElement.cpp (modified) (1 diff)
• svg/SVGImageElement.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash inside ImageLoader::updateFromElement()
+        https://bugs.webkit.org/show_bug.cgi?id=180769
+        <rdar://problem/35278782>
+
+        Reviewed by Antti Koivisto.
+
+        Fixed the crash by moving all call sites of ImageLoader::updateFromElement() to be post insertion callbacks
+        where it's safe to execute arbitrary scripts.
+
+        No new test since existing tests cover this with a newly added release assert in ImageLoader.
+
+        * html/HTMLImageElement.cpp:
+        (WebCore::HTMLImageElement::insertedIntoAncestor):
+        (WebCore::HTMLImageElement::didFinishInsertingNode): Extracted from insertedIntoAncestor to call
+        selectImageSource or updateFromElement.
+        * html/HTMLImageElement.h: Made many member functions final.
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::didAttachRenderers): Delay the call to ImageLoader::updateFromElement() in
+        ImageInputType using a post style resolution callback.
+        * html/HTMLMetaElement.h:
+        * html/HTMLPictureElement.cpp:
+        (WebCore::HTMLPictureElement::sourcesChanged): Store the list of child image elements into a vector before
+        calling selectImageSource since each call may execute arbitrary scripts.
+        * html/HTMLSourceElement.cpp:
+        (WebCore::HTMLSourceElement::insertedIntoAncestor): Delay the call to ImageLoader::updateFromElement()
+        using a post style resolution callback.
+        (WebCore::HTMLSourceElement::didFinishInsertingNode): Extracted from insertedIntoAncestor.
+        * html/HTMLSourceElement.h:
+        * html/HTMLVideoElement.cpp:
+        (WebCore::HTMLVideoElement::didAttachRenderers):
+        (WebCore::HTMLVideoElement::updateAfterStyleResolution): Extracted from didAttachRenderers.
+        * html/HTMLVideoElement.h:
+        * html/ImageInputType.cpp:
+        (WebCore::ImageInputType::needsPostStyleResolutionCallback): Added. Returns true so that HTMLInputElement's
+        didAttachRenderers would register a post style resolution callback.
+        (WebCore::ImageInputType::updateAfterStyleResolution): Extracted from attach.
+        (WebCore::ImageInputType::attach): Deleted.
+        * html/ImageInputType.h:
+        * html/InputType.cpp:
+        (WebCore::InputType::needsPostStyleResolutionCallback): Added. All but ImageInputType returns false.
+        (WebCore::InputType::updateAfterStyleResolution): Added.
+        (WebCore::InputType::attach): Deleted.
+        * html/InputType.h:
+        * loader/ImageLoader.cpp:
+        (WebCore::ImageLoader::updateFromElement): Added a release assertion. There is no direct security implication
+        so there is no need to use RELEASE_ASSERT_WITH_SECURITY_IMPLICATION here.
+        * svg/SVGImageElement.cpp:
+        (WebCore::SVGImageElement::insertedIntoAncestor):
+        (WebCore::SVGImageElement::didFinishInsertingNode):
+        * svg/SVGImageElement.h:
+
 2017-12-13  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/html/HTMLImageElement.cpp

@@ -296 +296 @@
 Node::InsertedIntoAncestorResult HTMLImageElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
 {
+    HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
+
     if (m_formSetByParser) {
         m_form = m_formSetByParser;
@@ -312 +314 @@
             m_form->registerImgElement(this);
     }
-    // Insert needs to complete first, before we start updating the loader. Loader dispatches events which could result
-    // in callbacks back to this node.
-    Node::InsertedIntoAncestorResult insertNotificationRequest = HTMLElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
 
     if (insertionType.connectedToDocument && !m_parsedUsemap.isNull())
@@ -321 +320 @@
     if (is<HTMLPictureElement>(parentNode())) {
         setPictureElement(&downcast<HTMLPictureElement>(*parentNode()));
-        selectImageSource();
+        return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
     }
 
@@ -327 +326 @@
     // our loader may have not fetched the image, so do it now.
     if (insertionType.connectedToDocument && !m_imageLoader.image())
+        return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
+
+    return InsertedIntoAncestorResult::Done;
+}
+
+void HTMLImageElement::didFinishInsertingNode()
+{
+    if (pictureElement())
+        selectImageSource();
+    else if (isConnected())
         m_imageLoader.updateFromElement();
-
-    return insertNotificationRequest;
 }
 

trunk/Source/WebCore/html/HTMLImageElement.h

@@ -91 +91 @@
     bool hasPendingActivity() const { return m_imageLoader.hasPendingActivity(); }
 
-    bool canContainRangeEndPoint() const override { return false; }
+    bool canContainRangeEndPoint() const final { return false; }
 
-    const AtomicString& imageSourceURL() const override;
+    const AtomicString& imageSourceURL() const final;
 
     bool hasShadowControls() const { return m_experimentalImageMenuEnabled; }
@@ -106 +106 @@
 
 private:
-    void parseAttribute(const QualifiedName&, const AtomicString&) override;
-    bool isPresentationAttribute(const QualifiedName&) const override;
-    void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) override;
+    void parseAttribute(const QualifiedName&, const AtomicString&) final;
+    bool isPresentationAttribute(const QualifiedName&) const final;
+    void collectStyleForPresentationAttribute(const QualifiedName&, const AtomicString&, MutableStyleProperties&) final;
 
-    void didAttachRenderers() override;
-    RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) override;
+    void didAttachRenderers() final;
+    RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) final;
     void setBestFitURLAndDPRFromImageCandidate(const ImageCandidate&);
 
-    bool canStartSelection() const override;
+    bool canStartSelection() const final;
 
-    bool isURLAttribute(const Attribute&) const override;
-    bool attributeContainsURL(const Attribute&) const override;
-    String completeURLsInAttributeValue(const URL& base, const Attribute&) const override;
+    bool isURLAttribute(const Attribute&) const final;
+    bool attributeContainsURL(const Attribute&) const final;
+    String completeURLsInAttributeValue(const URL& base, const Attribute&) const final;
 
-    bool draggable() const override;
+    bool draggable() const final;
 
-    void addSubresourceAttributeURLs(ListHashSet<URL>&) const override;
+    void addSubresourceAttributeURLs(ListHashSet<URL>&) const final;
 
-    InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) override;
-    void removedFromAncestor(RemovalType, ContainerNode&) override;
+    InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;
+    void didFinishInsertingNode() final;
+    void removedFromAncestor(RemovalType, ContainerNode&) final;
 
     bool isFormAssociatedElement() const final { return false; }
@@ -153 +154 @@
     void destroyImageControls();
     bool hasImageControls() const;
-    bool childShouldCreateRenderer(const Node&) const override;
+    bool childShouldCreateRenderer(const Node&) const final;
 #endif
 

trunk/Source/WebCore/html/HTMLInputElement.cpp

@@ -59 +59 @@
 #include "Settings.h"
 #include "StyleResolver.h"
+#include "StyleTreeResolver.h"
 #include "TextControlInnerElements.h"
 #include <wtf/Language.h>
@@ -846 +847 @@
     HTMLTextFormControlElement::didAttachRenderers();
 
-    m_inputType->attach();
+    if (m_inputType->needsPostStyleResolutionCallback()) {
+        Style::queuePostResolutionCallback([protectedElement = makeRef(*this)] {
+            protectedElement->m_inputType->updateAfterStyleResolution();
+        });
+    }
 
     if (document().focusedElement() == this) {

trunk/Source/WebCore/html/HTMLMetaElement.h

@@ -41 +41 @@
     void parseAttribute(const QualifiedName&, const AtomicString&) final;
     InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;
-    void didFinishInsertingNode();
+    void didFinishInsertingNode() final;
 
     void process();

trunk/Source/WebCore/html/HTMLPictureElement.cpp

@@ -57 +57 @@
 void HTMLPictureElement::sourcesChanged()
 {
+    Vector<Ref<HTMLImageElement>, 4> imageElements;
     for (auto& element : childrenOfType<HTMLImageElement>(*this))
-        element.selectImageSource();
+        imageElements.append(element);
+    for (auto& element : imageElements)
+        element->selectImageSource();
 }
 

trunk/Source/WebCore/html/HTMLSourceElement.cpp

@@ -75 +75 @@
 #endif
         if (is<HTMLPictureElement>(*parent))
-            downcast<HTMLPictureElement>(*parent).sourcesChanged();
+            return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
+
     }
     return InsertedIntoAncestorResult::Done;
+}
+
+void HTMLSourceElement::didFinishInsertingNode()
+{
+    auto* parent = parentElement();
+    if (is<HTMLPictureElement>(*parent))
+        downcast<HTMLPictureElement>(*parent).sourcesChanged();
 }
 

trunk/Source/WebCore/html/HTMLSourceElement.h

@@ -47 +47 @@
     
     InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;
+    void didFinishInsertingNode() final;
     void removedFromAncestor(RemovalType, ContainerNode&) final;
     bool isURLAttribute(const Attribute&) const final;

trunk/Source/WebCore/html/HTMLVideoElement.cpp

@@ -44 +44 @@
 #include "ScriptController.h"
 #include "Settings.h"
+#include "StyleTreeResolver.h"
 #include <wtf/text/TextStream.h>
 
@@ -91 +92 @@
     updateDisplayState();
     if (shouldDisplayPosterImage()) {
-        if (!m_imageLoader)
-            m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
-        m_imageLoader->updateFromElement();
-        if (auto* renderer = this->renderer())
-            renderer->imageResource().setCachedImage(m_imageLoader->image());
-    }
+        Style::queuePostResolutionCallback([protectedThis = makeRef(*this)] {
+            protectedThis->updateAfterStyleResolution();
+        });
+    }
+}
+
+void HTMLVideoElement::updateAfterStyleResolution()
+{
+    if (!m_imageLoader)
+        m_imageLoader = std::make_unique<HTMLImageLoader>(*this);
+    m_imageLoader->updateFromElement();
+    if (auto* renderer = this->renderer())
+        renderer->imageResource().setCachedImage(m_imageLoader->image());
 }
 

trunk/Source/WebCore/html/HTMLVideoElement.h

@@ -97 +97 @@
     bool rendererIsNeeded(const RenderStyle&) final;
     void didAttachRenderers() final;
+    void updateAfterStyleResolution();
     void parseAttribute(const QualifiedName&, const AtomicString&) final;
     bool isPresentationAttribute(const QualifiedName&) const final;

trunk/Source/WebCore/html/ImageInputType.cpp

@@ -127 +127 @@
 }
 
-void ImageInputType::attach()
-{
-    BaseButtonInputType::attach();
-
+bool ImageInputType::needsPostStyleResolutionCallback()
+{
+    return true;
+}
+
+void ImageInputType::updateAfterStyleResolution()
+{
     HTMLImageLoader& imageLoader = element().ensureImageLoader();
     imageLoader.updateFromElement();

trunk/Source/WebCore/html/ImageInputType.h

@@ -51 +51 @@
     void altAttributeChanged() override;
     void srcAttributeChanged() override;
-    void attach() override;
+    bool needsPostStyleResolutionCallback() override;
+    void updateAfterStyleResolution() override;
     bool shouldRespectAlignAttribute() override;
     bool canBeSuccessfulSubmitButton() override;

trunk/Source/WebCore/html/InputType.cpp

@@ -588 +588 @@
 }
 
-void InputType::attach()
+bool InputType::needsPostStyleResolutionCallback()
+{
+    return false;
+}
+
+void InputType::updateAfterStyleResolution()
 {
 }

trunk/Source/WebCore/html/InputType.h

@@ -236 +236 @@
     virtual RenderPtr<RenderElement> createInputRenderer(RenderStyle&&);
     virtual void addSearchResult();
-    virtual void attach();
+    virtual bool needsPostStyleResolutionCallback();
+    virtual void updateAfterStyleResolution();
     virtual void detach();
     virtual void minOrMaxAttributeChanged();

trunk/Source/WebCore/loader/ImageLoader.cpp

@@ -37 +37 @@
 #include "HTMLObjectElement.h"
 #include "HTMLParserIdioms.h"
+#include "NoEventDispatchAssertion.h"
 #include "Page.h"
 #include "RenderImage.h"
@@ -158 +159 @@
 void ImageLoader::updateFromElement()
 {
+    RELEASE_ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed());
     // If we're not making renderers for the page, then don't load images. We don't want to slow
     // down the raw HTML parsing case by loading images we don't intend to display.

trunk/Source/WebCore/svg/SVGImageElement.cpp

@@ -190 +190 @@
 {
     SVGGraphicsElement::insertedIntoAncestor(insertionType, parentOfInsertedTree);
-    if (!insertionType.connectedToDocument)
-        return InsertedIntoAncestorResult::Done;
-    // Update image loader, as soon as we're living in the tree.
-    // We can only resolve base URIs properly, after that!
+    if (insertionType.connectedToDocument)
+        return InsertedIntoAncestorResult::NeedsPostInsertionCallback;
+    return InsertedIntoAncestorResult::Done;
+}
+
+void SVGImageElement::didFinishInsertingNode()
+{
     m_imageLoader.updateFromElement();
-    return InsertedIntoAncestorResult::Done;
 }
 

trunk/Source/WebCore/svg/SVGImageElement.h

@@ -51 +51 @@
     void didAttachRenderers() final;
     InsertedIntoAncestorResult insertedIntoAncestor(InsertionType, ContainerNode&) final;
+    void didFinishInsertingNode() final;
 
     RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) final;