Harden against layout passes triggered when iterating through HTMLFormElement::associ...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jan 2018 08:19:16 +0000 (08:19 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Jan 2018 08:19:16 +0000 (08:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182037
<rdar://problem/36747812>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Observe that HTMLFormElement::associatedElements returns a const reference to a Vector of raw
FormAssociatedElement pointers. In various call sites that iterate through these associated elements using this
function, some require synchronous layout updates per iteration, which can lead to a bad time when combined with
the first observation.

To address this, we introduce HTMLFormElement::copyAssociatedElementsVector. This returns a new vector
containing strong Refs to each associated element. From each call site that may trigger synchronous layout and
execute arbitrary script while iterating over associated form elements, we instead use iterate over protected
FormAssociatedElements.

From each call site that currently doesn't (and shouldn't) require a layout update, we use the old version that
returns a list of raw FormAssociatedElement pointers, but add ScriptDisallowedScopes to ensure that we never
execute script there in the future.

Test: fast/forms/form-data-associated-element-iteration.html

* html/DOMFormData.cpp:
(WebCore::DOMFormData::DOMFormData):

Change to use copyAssociatedElementsVector().

* html/FormController.cpp:
(WebCore::recordFormStructure):
(WebCore::FormController::restoreControlStateIn):

Change to use copyAssociatedElementsVector().

* html/HTMLFieldSetElement.cpp:
(WebCore::HTMLFieldSetElement::copyAssociatedElementsVector const):
(WebCore:: const):
(WebCore::HTMLFieldSetElement::length const):

Refactor to use unsafeAssociatedElements().

* html/HTMLFieldSetElement.h:
* html/HTMLFormControlsCollection.cpp:
(WebCore:: const):
(WebCore::HTMLFormControlsCollection::copyFormControlElementsVector const):
(WebCore::HTMLFormControlsCollection::customElementAfter const):
(WebCore::HTMLFormControlsCollection::updateNamedElementCache const):

Refactor these to use unsafeAssociatedElements().

* html/HTMLFormControlsCollection.h:
* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::unsafeAssociatedElements const):
(WebCore::HTMLFormElement::copyAssociatedElementsVector const):
* html/HTMLFormElement.h:
* loader/FormSubmission.cpp:
(WebCore::FormSubmission::create):

Refactor to use copyAssociatedElementsVector().

Source/WebKitLegacy/mac:

Rename associatedElements() to unsafeAssociatedElements(), and add ScriptDisallowedScopes. See WebCore ChangeLog
for more details.

* WebView/WebHTMLRepresentation.mm:
(-[WebHTMLRepresentation elementWithName:inForm:]):
(-[WebHTMLRepresentation controlsInForm:]):

LayoutTests:

Add a new layout test covering these hardening changes. See WebCore ChangeLog for more details.

* fast/forms/form-data-associated-element-iteration-expected.txt: Added.
* fast/forms/form-data-associated-element-iteration.html: Added.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/forms/form-data-associated-element-iteration-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/form-data-associated-element-iteration.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/DOMFormData.cpp
Source/WebCore/html/FormController.cpp
Source/WebCore/html/HTMLFieldSetElement.cpp
Source/WebCore/html/HTMLFieldSetElement.h
Source/WebCore/html/HTMLFormControlsCollection.cpp
Source/WebCore/html/HTMLFormControlsCollection.h
Source/WebCore/html/HTMLFormElement.cpp
Source/WebCore/html/HTMLFormElement.h
Source/WebCore/loader/FormSubmission.cpp
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebView/WebHTMLRepresentation.mm

index f6500a1..22bc9de 100644 (file)
@@ -1,3 +1,16 @@
+2018-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Harden against layout passes triggered when iterating through HTMLFormElement::associatedElements
+        https://bugs.webkit.org/show_bug.cgi?id=182037
+        <rdar://problem/36747812>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a new layout test covering these hardening changes. See WebCore ChangeLog for more details.
+
+        * fast/forms/form-data-associated-element-iteration-expected.txt: Added.
+        * fast/forms/form-data-associated-element-iteration.html: Added.
+
 2018-01-23  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         Import WPT for modules
diff --git a/LayoutTests/fast/forms/form-data-associated-element-iteration-expected.txt b/LayoutTests/fast/forms/form-data-associated-element-iteration-expected.txt
new file mode 100644 (file)
index 0000000..9378856
--- /dev/null
@@ -0,0 +1,2 @@
+This test passes if the web content process does not crash.
+
diff --git a/LayoutTests/fast/forms/form-data-associated-element-iteration.html b/LayoutTests/fast/forms/form-data-associated-element-iteration.html
new file mode 100644 (file)
index 0000000..ea6cf60
--- /dev/null
@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<html>
+<form id="form"></form>
+<div>This test passes if the web content process does not crash.</div>
+<script>
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    textAreaCount = 100;
+    for (let i = 1; i <= textAreaCount; ++i) {
+        let textarea = document.createElement("textarea");
+        textarea.id = textarea.name = `textarea-${i}`;
+        form.appendChild(textarea);
+    }
+
+    layoutCount = 1;
+    frame = document.body.appendChild(document.createElement("iframe"));
+    frame.contentWindow.matchMedia("(max-width: 100px)").addListener(listener);
+    new FormData(form);
+
+    function listener() {
+        if (layoutCount == textAreaCount - 1) {
+            for (let i = 0; i <= textAreaCount; ++i) {
+                let textarea = document.getElementById(`textarea-${i}`);
+                if (!textarea)
+                    continue;
+
+                textarea.remove();
+                textarea = null;
+            }
+        }
+
+        if (layoutCount <= textAreaCount) {
+            frame.contentWindow.matchMedia(`(max-width: ${layoutCount + 1}px)`).addListener(listener);
+            frame.width = layoutCount++;
+        }
+    }
+
+    form.remove();
+</script>
+</html>
\ No newline at end of file
index f02e623..d049d1f 100644 (file)
@@ -1,3 +1,64 @@
+2018-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Harden against layout passes triggered when iterating through HTMLFormElement::associatedElements
+        https://bugs.webkit.org/show_bug.cgi?id=182037
+        <rdar://problem/36747812>
+
+        Reviewed by Ryosuke Niwa.
+
+        Observe that HTMLFormElement::associatedElements returns a const reference to a Vector of raw
+        FormAssociatedElement pointers. In various call sites that iterate through these associated elements using this
+        function, some require synchronous layout updates per iteration, which can lead to a bad time when combined with
+        the first observation.
+
+        To address this, we introduce HTMLFormElement::copyAssociatedElementsVector. This returns a new vector
+        containing strong Refs to each associated element. From each call site that may trigger synchronous layout and
+        execute arbitrary script while iterating over associated form elements, we instead use iterate over protected
+        FormAssociatedElements.
+
+        From each call site that currently doesn't (and shouldn't) require a layout update, we use the old version that
+        returns a list of raw FormAssociatedElement pointers, but add ScriptDisallowedScopes to ensure that we never
+        execute script there in the future.
+
+        Test: fast/forms/form-data-associated-element-iteration.html
+
+        * html/DOMFormData.cpp:
+        (WebCore::DOMFormData::DOMFormData):
+
+        Change to use copyAssociatedElementsVector().
+
+        * html/FormController.cpp:
+        (WebCore::recordFormStructure):
+        (WebCore::FormController::restoreControlStateIn):
+
+        Change to use copyAssociatedElementsVector().
+
+        * html/HTMLFieldSetElement.cpp:
+        (WebCore::HTMLFieldSetElement::copyAssociatedElementsVector const):
+        (WebCore:: const):
+        (WebCore::HTMLFieldSetElement::length const):
+
+        Refactor to use unsafeAssociatedElements().
+
+        * html/HTMLFieldSetElement.h:
+        * html/HTMLFormControlsCollection.cpp:
+        (WebCore:: const):
+        (WebCore::HTMLFormControlsCollection::copyFormControlElementsVector const):
+        (WebCore::HTMLFormControlsCollection::customElementAfter const):
+        (WebCore::HTMLFormControlsCollection::updateNamedElementCache const):
+
+        Refactor these to use unsafeAssociatedElements().
+
+        * html/HTMLFormControlsCollection.h:
+        * html/HTMLFormElement.cpp:
+        (WebCore::HTMLFormElement::unsafeAssociatedElements const):
+        (WebCore::HTMLFormElement::copyAssociatedElementsVector const):
+        * html/HTMLFormElement.h:
+        * loader/FormSubmission.cpp:
+        (WebCore::FormSubmission::create):
+
+        Refactor to use copyAssociatedElementsVector().
+
 2018-01-23  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 
         [Curl] Fix wrong redirection with relative url when it happens from
index ebe8f8c..afe7c13 100644 (file)
@@ -47,7 +47,7 @@ DOMFormData::DOMFormData(HTMLFormElement* form)
     if (!form)
         return;
 
-    for (auto& element : form->associatedElements()) {
+    for (auto& element : form->copyAssociatedElementsVector()) {
         if (!element->asHTMLElement().isDisabledFormControl())
             element->appendFormData(*this, true);
     }
index 66529ba..0f5b775 100644 (file)
@@ -23,6 +23,7 @@
 
 #include "HTMLFormElement.h"
 #include "HTMLInputElement.h"
+#include "ScriptDisallowedScope.h"
 #include <wtf/NeverDestroyed.h>
 #include <wtf/text/StringBuilder.h>
 #include <wtf/text/StringConcatenateNumbers.h>
@@ -279,9 +280,10 @@ private:
 
 static inline void recordFormStructure(const HTMLFormElement& form, StringBuilder& builder)
 {
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
     // 2 is enough to distinguish forms in webkit.org/b/91209#c0
     const size_t namedControlsToBeRecorded = 2;
-    const Vector<FormAssociatedElement*>& controls = form.associatedElements();
+    auto& controls = form.unsafeAssociatedElements();
     builder.appendLiteral(" [");
     for (size_t i = 0, namedControls = 0; i < controls.size() && namedControls < namedControlsToBeRecorded; ++i) {
         if (!controls[i]->isFormControlElementWithState())
@@ -454,17 +456,17 @@ void FormController::restoreControlStateFor(HTMLFormControlElementWithState& con
 
 void FormController::restoreControlStateIn(HTMLFormElement& form)
 {
-    for (auto& element : form.associatedElements()) {
-        if (!is<HTMLFormControlElementWithState>(*element))
+    for (auto& element : form.copyAssociatedElementsVector()) {
+        if (!is<HTMLFormControlElementWithState>(element.get()))
             continue;
-        auto control = makeRef(downcast<HTMLFormControlElementWithState>(*element));
-        if (!control->shouldSaveAndRestoreFormControlState())
+        auto& control = downcast<HTMLFormControlElementWithState>(element.get());
+        if (!control.shouldSaveAndRestoreFormControlState())
             continue;
         if (ownerFormForState(control) != &form)
             continue;
         auto state = takeStateForFormElement(control);
         if (!state.isEmpty())
-            control->restoreFormControlState(state);
+            control.restoreFormControlState(state);
     }
 }
 
index 6fbd38e..6860237 100644 (file)
@@ -32,6 +32,7 @@
 #include "HTMLObjectElement.h"
 #include "NodeRareData.h"
 #include "RenderElement.h"
+#include "ScriptDisallowedScope.h"
 #include <wtf/StdLibExtras.h>
 
 namespace WebCore {
@@ -191,16 +192,26 @@ void HTMLFieldSetElement::updateAssociatedElements() const
     }
 }
 
-const Vector<FormAssociatedElement*>& HTMLFieldSetElement::associatedElements() const
+Vector<Ref<FormAssociatedElement>> HTMLFieldSetElement::copyAssociatedElementsVector() const
 {
     updateAssociatedElements();
+    return WTF::map(m_associatedElements, [] (auto* rawElement) {
+        return Ref<FormAssociatedElement>(*rawElement);
+    });
+}
+
+const Vector<FormAssociatedElement*>& HTMLFieldSetElement::unsafeAssociatedElements() const
+{
+    ASSERT(!ScriptDisallowedScope::InMainThread::isScriptAllowed());
+    updateAssociatedElements();
     return m_associatedElements;
 }
 
 unsigned HTMLFieldSetElement::length() const
 {
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
     unsigned length = 0;
-    for (auto* element : associatedElements()) {
+    for (auto* element : unsafeAssociatedElements()) {
         if (element->isEnumeratable())
             ++length;
     }
index dd0554c..d0b4a3b 100644 (file)
@@ -40,7 +40,8 @@ public:
     Ref<HTMLFormControlsCollection> elements();
     Ref<HTMLCollection> elementsForNativeBindings();
 
-    const Vector<FormAssociatedElement*>& associatedElements() const;
+    const Vector<FormAssociatedElement*>& unsafeAssociatedElements() const;
+    Vector<Ref<FormAssociatedElement>> copyAssociatedElementsVector() const;
     unsigned length() const;
 
     void addInvalidDescendant(const HTMLFormControlElement&);
index 7f0e7ca..69a54b8 100644 (file)
@@ -27,6 +27,7 @@
 #include "HTMLFormElement.h"
 #include "HTMLImageElement.h"
 #include "HTMLNames.h"
+#include "ScriptDisallowedScope.h"
 
 namespace WebCore {
 
@@ -62,12 +63,20 @@ std::optional<Variant<RefPtr<RadioNodeList>, RefPtr<Element>>> HTMLFormControlsC
     return Variant<RefPtr<RadioNodeList>, RefPtr<Element>> { RefPtr<RadioNodeList> { ownerNode().radioNodeList(name) } };
 }
 
-const Vector<FormAssociatedElement*>& HTMLFormControlsCollection::formControlElements() const
+const Vector<FormAssociatedElement*>& HTMLFormControlsCollection::unsafeFormControlElements() const
 {
     ASSERT(is<HTMLFormElement>(ownerNode()) || is<HTMLFieldSetElement>(ownerNode()));
     if (is<HTMLFormElement>(ownerNode()))
-        return downcast<HTMLFormElement>(ownerNode()).associatedElements();
-    return downcast<HTMLFieldSetElement>(ownerNode()).associatedElements();
+        return downcast<HTMLFormElement>(ownerNode()).unsafeAssociatedElements();
+    return downcast<HTMLFieldSetElement>(ownerNode()).unsafeAssociatedElements();
+}
+
+Vector<Ref<FormAssociatedElement>> HTMLFormControlsCollection::copyFormControlElementsVector() const
+{
+    ASSERT(is<HTMLFormElement>(ownerNode()) || is<HTMLFieldSetElement>(ownerNode()));
+    if (is<HTMLFormElement>(ownerNode()))
+        return downcast<HTMLFormElement>(ownerNode()).copyAssociatedElementsVector();
+    return downcast<HTMLFieldSetElement>(ownerNode()).copyAssociatedElementsVector();
 }
 
 const Vector<HTMLImageElement*>& HTMLFormControlsCollection::formImageElements() const
@@ -88,7 +97,8 @@ static unsigned findFormAssociatedElement(const Vector<FormAssociatedElement*>&
 
 HTMLElement* HTMLFormControlsCollection::customElementAfter(Element* current) const
 {
-    const Vector<FormAssociatedElement*>& elements = formControlElements();
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+    auto& elements = unsafeFormControlElements();
     unsigned start;
     if (!current)
         start = 0;
@@ -118,7 +128,8 @@ void HTMLFormControlsCollection::updateNamedElementCache() const
     bool ownerIsFormElement = is<HTMLFormElement>(ownerNode());
     HashSet<AtomicStringImpl*> foundInputElements;
 
-    for (auto& elementPtr : formControlElements()) {
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+    for (auto& elementPtr : unsafeFormControlElements()) {
         FormAssociatedElement& associatedElement = *elementPtr;
         if (associatedElement.isEnumeratable()) {
             HTMLElement& element = associatedElement.asHTMLElement();
index f6bb142..17ce503 100644 (file)
@@ -51,7 +51,8 @@ private:
     void invalidateCacheForDocument(Document&) override;
     void updateNamedElementCache() const override;
 
-    const Vector<FormAssociatedElement*>& formControlElements() const;
+    const Vector<FormAssociatedElement*>& unsafeFormControlElements() const;
+    Vector<Ref<FormAssociatedElement>> copyFormControlElementsVector() const;
     const Vector<HTMLImageElement*>& formImageElements() const;
 
     mutable Element* m_cachedElement;
index 6ff7e87..1e4389d 100644 (file)
@@ -47,6 +47,7 @@
 #include "Page.h"
 #include "RadioNodeList.h"
 #include "RenderTextControl.h"
+#include "ScriptDisallowedScope.h"
 #include "Settings.h"
 #include "UserGestureIndicator.h"
 #include <limits>
@@ -851,6 +852,19 @@ void HTMLFormElement::finishParsingChildren()
     document().formController().restoreControlStateIn(*this);
 }
 
+const Vector<FormAssociatedElement*>& HTMLFormElement::unsafeAssociatedElements() const
+{
+    ASSERT(!ScriptDisallowedScope::InMainThread::isScriptAllowed());
+    return m_associatedElements;
+}
+
+Vector<Ref<FormAssociatedElement>> HTMLFormElement::copyAssociatedElementsVector() const
+{
+    return WTF::map(m_associatedElements, [] (auto* rawElement) {
+        return Ref<FormAssociatedElement>(*rawElement);
+    });
+}
+
 void HTMLFormElement::copyNonAttributePropertiesFromElement(const Element& source)
 {
     m_wasDemoted = static_cast<const HTMLFormElement&>(source).m_wasDemoted;
index d6a4608..add8581 100644 (file)
@@ -116,7 +116,8 @@ public:
 
     RadioButtonGroups& radioButtonGroups() { return m_radioButtonGroups; }
 
-    const Vector<FormAssociatedElement*>& associatedElements() const { return m_associatedElements; }
+    WEBCORE_EXPORT const Vector<FormAssociatedElement*>& unsafeAssociatedElements() const;
+    Vector<Ref<FormAssociatedElement>> copyAssociatedElementsVector() const;
     const Vector<HTMLImageElement*>& imageElements() const { return m_imageElements; }
 
     StringPairVector textFieldValues() const;
index 3aa6e56..abb385f 100644 (file)
@@ -182,11 +182,7 @@ Ref<FormSubmission> FormSubmission::create(HTMLFormElement& form, const Attribut
     StringPairVector formValues;
 
     bool containsPasswordData = false;
-    auto protectedAssociatedElements = WTF::map(form.associatedElements(), [] (auto* rawElement) {
-        return Ref<FormAssociatedElement>(*rawElement);
-    });
-
-    for (auto& control : protectedAssociatedElements) {
+    for (auto& control : form.copyAssociatedElementsVector()) {
         auto& element = control->asHTMLElement();
         if (!element.isDisabledFormControl())
             control->appendFormData(domFormData, isMultiPartForm);
index 9afa175..23f9dfe 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-23  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Harden against layout passes triggered when iterating through HTMLFormElement::associatedElements
+        https://bugs.webkit.org/show_bug.cgi?id=182037
+        <rdar://problem/36747812>
+
+        Reviewed by Ryosuke Niwa.
+
+        Rename associatedElements() to unsafeAssociatedElements(), and add ScriptDisallowedScopes. See WebCore ChangeLog
+        for more details.
+
+        * WebView/WebHTMLRepresentation.mm:
+        (-[WebHTMLRepresentation elementWithName:inForm:]):
+        (-[WebHTMLRepresentation controlsInForm:]):
+
 2018-01-23  Alex Christensen  <achristensen@webkit.org>
 
         Use CompletionHandlers for ResourceHandleClient::didReceiveResponseAsync
index b7bca45..8718992 100644 (file)
@@ -58,6 +58,7 @@
 #import <WebCore/NodeTraversal.h>
 #import <WebCore/Range.h>
 #import <WebCore/RenderElement.h>
+#import <WebCore/ScriptDisallowedScope.h>
 #import <WebCore/TextResourceDecoder.h>
 #import <WebKitLegacy/DOMHTMLInputElement.h>
 #import <yarr/RegularExpression.h>
@@ -281,7 +282,9 @@ static HTMLFormElement* formElementFromDOMElement(DOMElement *element)
     HTMLFormElement* formElement = formElementFromDOMElement(form);
     if (!formElement)
         return nil;
-    const Vector<FormAssociatedElement*>& elements = formElement->associatedElements();
+
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+    auto& elements = formElement->unsafeAssociatedElements();
     AtomicString targetName = name;
     for (unsigned i = 0; i < elements.size(); i++) {
         FormAssociatedElement& element = *elements[i];
@@ -329,7 +332,9 @@ static HTMLInputElement* inputElementFromDOMElement(DOMElement* element)
     if (!formElement)
         return nil;
     NSMutableArray *results = nil;
-    const Vector<FormAssociatedElement*>& elements = formElement->associatedElements();
+
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+    auto& elements = formElement->unsafeAssociatedElements();
     for (unsigned i = 0; i < elements.size(); i++) {
         if (elements[i]->isEnumeratable()) { // Skip option elements, other duds
             DOMElement *element = kit(&elements[i]->asHTMLElement());