Merge r222005 - Submitting a form can cause HTMLFormElement's associa…

…ted elements vector to be mutated during iteration https://bugs.webkit.org/show_bug.cgi?id=176368 <rdar://problem/34254998> Reviewed by Ryosuke Niwa. Source/WebCore: In the process of iterating over form.associatedElements() during form submission in FormSubmission::create, the page may cause us to clobber the vector of FormAssociatedElements* we're currently iterating over by inserting new form controls beneath the form element we're in the process of submitting. This happens because FormSubmission::create calls HTMLTextAreaElement::appendFormData, which requires layout to be up to date, which in turn makes us updateLayout() and set focus, which fires a `change` event, upon which the page's JavaScript inserts additonal DOM nodes into the form, modifying the vector of associated elements. To mitigate this, instead of iterating over HTMLFormElement::associatedElements(), which returns a reference to the HTMLFormElement's actual m_associatedElements vector, we iterate over a new vector of Ref<FormAssociatedElement>s created from m_associatedElements. This patch also removes an event dispatch assertion added in r212026. This assertion was added to catch any other events dispatched in this scope, since dispatching events there would have had security implications, but after making iteration over associated elements robust, this NoEventDispatchAssertion is no longer useful. Test: fast/forms/append-children-during-form-submission.html * loader/FormSubmission.cpp: (WebCore::FormSubmission::create): LayoutTests: Adds a new test to make sure we don't crash when mutating a form's associated elements during form submission. * fast/forms/append-children-during-form-submission-expected.txt: Added. * fast/forms/append-children-during-form-submission.html: Added.
WebKit · Oct 16, 2017 · c460b82 · c460b82
1 parent 217a6f1
commit c460b82
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 15 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2017-09-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Submitting a form can cause HTMLFormElement's associated elements vector to be mutated during iteration
+        https://bugs.webkit.org/show_bug.cgi?id=176368
+        <rdar://problem/34254998>
+
+        Reviewed by Ryosuke Niwa.
+
+        Adds a new test to make sure we don't crash when mutating a form's associated elements during form submission.
+
+        * fast/forms/append-children-during-form-submission-expected.txt: Added.
+        * fast/forms/append-children-during-form-submission.html: Added.
+
 2017-09-12  Myles C. Maxfield  <mmaxfield@apple.com>
 
         ASSERTION FAILED: !m_valueOrException under FontFaceSet::completedLoading loading a Serious Eats page

diff --git a/LayoutTests/fast/forms/append-children-during-form-submission-expected.txt b/LayoutTests/fast/forms/append-children-during-form-submission-expected.txt
@@ -0,0 +1,2 @@
+
+To manually test, load this page. The web process should not crash.
diff --git a/LayoutTests/fast/forms/append-children-during-form-submission.html b/LayoutTests/fast/forms/append-children-during-form-submission.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<script>
+function loaded() {
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    area1.setRangeText("foo");
+    area1.name = "foo";
+    area2.autofocus = true;
+    form.insertBefore(area2, form.firstChild);
+    form.submit();
+}
+
+function changed() {
+    for (let i = 0; i < 100; i++)
+        form.appendChild(document.createElement("input"));
+}
+</script>
+
+<body onload=loaded()>
+<form id="form" onchange=changed()>
+    <textarea id="area1">a</textarea>
+    <object></object>
+    <textarea id="area2">b</textarea>
+</form>
+<p>To manually test, load this page. The web process should not crash.</p>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2017-09-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Submitting a form can cause HTMLFormElement's associated elements vector to be mutated during iteration
+        https://bugs.webkit.org/show_bug.cgi?id=176368
+        <rdar://problem/34254998>
+
+        Reviewed by Ryosuke Niwa.
+
+        In the process of iterating over form.associatedElements() during form submission in FormSubmission::create, the
+        page may cause us to clobber the vector of FormAssociatedElements* we're currently iterating over by inserting
+        new form controls beneath the form element we're in the process of submitting. This happens because
+        FormSubmission::create calls HTMLTextAreaElement::appendFormData, which requires layout to be up to date, which
+        in turn makes us updateLayout() and set focus, which fires a `change` event, upon which the page's JavaScript
+        inserts additonal DOM nodes into the form, modifying the vector of associated elements.
+
+        To mitigate this, instead of iterating over HTMLFormElement::associatedElements(), which returns a reference to
+        the HTMLFormElement's actual m_associatedElements vector, we iterate over a new vector of
+        Ref<FormAssociatedElement>s created from m_associatedElements.
+
+        This patch also removes an event dispatch assertion added in r212026. This assertion was added to catch any
+        other events dispatched in this scope, since dispatching events there would have had security implications, but
+        after making iteration over associated elements robust, this NoEventDispatchAssertion is no longer useful.
+
+        Test: fast/forms/append-children-during-form-submission.html
+
+        * loader/FormSubmission.cpp:
+        (WebCore::FormSubmission::create):
+
 2017-09-13  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] Fails to build because 'Float32Array' has not been declared in AudioContext.h

diff --git a/Source/WebCore/loader/FormSubmission.cpp b/Source/WebCore/loader/FormSubmission.cpp
@@ -191,22 +191,22 @@ Ref<FormSubmission> FormSubmission::create(HTMLFormElement& form, const Attribut
     StringPairVector formValues;
 
     bool containsPasswordData = false;
-    {
-        NoEventDispatchAssertion noEventDispatchAssertion;
-
-        for (auto& control : form.associatedElements()) {
-            auto& element = control->asHTMLElement();
-            if (!element.isDisabledFormControl())
-                control->appendFormData(domFormData, isMultiPartForm);
-            if (is<HTMLInputElement>(element)) {
-                auto& input = downcast<HTMLInputElement>(element);
-                if (input.isTextField()) {
-                    formValues.append({ input.name().string(), input.value() });
-                    input.addSearchResult();
-                }
-                if (input.isPasswordField() && !input.value().isEmpty())
-                    containsPasswordData = true;
+    auto protectedAssociatedElements = form.associatedElements().map([] (FormAssociatedElement* rawElement) -> Ref<FormAssociatedElement> {
+        return *rawElement;
+    });
+
+    for (auto& control : protectedAssociatedElements) {
+        auto& element = control->asHTMLElement();
+        if (!element.isDisabledFormControl())
+            control->appendFormData(domFormData, isMultiPartForm);
+        if (is<HTMLInputElement>(element)) {
+            auto& input = downcast<HTMLInputElement>(element);
+            if (input.isTextField()) {
+                formValues.append({ input.name(), input.value() });
+                input.addSearchResult();
             }
+            if (input.isPasswordField() && !input.value().isEmpty())
+                containsPasswordData = true;
         }
     }