Changeset 242917 in webkit

Timestamp:

Mar 13, 2019, 4:18:26 PM (7 years ago)

Author:

Wenson Hsieh

Message:

Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
​https://bugs.webkit.org/show_bug.cgi?id=195663
<rdar://problem/48576391>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
control element is ref'd while carrying out its destructor's logic. This may happen in two places in
HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
protected reference to the form control element since they should never result in any script execution.

To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
we don't change these codepaths in the future, such that they trigger arbitrary script execution.

Test: fast/forms/remove-associated-element-after-gc.html

• html/HTMLFormControlElement.cpp:

(WebCore::HTMLFormControlElement::didChangeForm):

• html/HTMLFormElement.cpp:

(WebCore::HTMLFormElement::resetDefaultButton):

LayoutTests:

Add a layout test to exercise the scenario described in the WebCore ChangeLog.

• fast/forms/remove-associated-element-after-gc-expected.txt: Added.
• fast/forms/remove-associated-element-after-gc.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/forms/remove-associated-element-after-gc-expected.txt (added)
• LayoutTests/fast/forms/remove-associated-element-after-gc.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLFormControlElement.cpp (modified) (2 diffs)
• Source/WebCore/html/HTMLFormElement.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-03-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+        https://bugs.webkit.org/show_bug.cgi?id=195663
+        <rdar://problem/48576391>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a layout test to exercise the scenario described in the WebCore ChangeLog.
+
+        * fast/forms/remove-associated-element-after-gc-expected.txt: Added.
+        * fast/forms/remove-associated-element-after-gc.html: Added.
+
 2019-03-13  Nikita Vasilyev  <nvasilyev@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Fix an edge case where HTMLFormElement::removeFormElement is invoked twice with the same element
+        https://bugs.webkit.org/show_bug.cgi?id=195663
+        <rdar://problem/48576391>
+
+        Reviewed by Ryosuke Niwa.
+
+        Currently, it's possible for HTMLFormControlElement's destructor to be reentrant. This may happen if the form
+        control element is ref'd while carrying out its destructor's logic. This may happen in two places in
+        HTMLFormControlElement (didChangeForm and resetDefaultButton), both of which actually don't require ensuring a
+        protected reference to the form control element since they should never result in any script execution.
+
+        To fix the bug, convert these strong references into raw pointers, and add ScriptDisallowedScope to ensure that
+        we don't change these codepaths in the future, such that they trigger arbitrary script execution.
+
+        Test: fast/forms/remove-associated-element-after-gc.html
+
+        * html/HTMLFormControlElement.cpp:
+        (WebCore::HTMLFormControlElement::didChangeForm):
+        * html/HTMLFormElement.cpp:
+        (WebCore::HTMLFormElement::resetDefaultButton):
+
 2019-03-13  Daniel Bates  <dabates@apple.com>
 

trunk/Source/WebCore/html/HTMLFormControlElement.cpp

@@ -42 +42 @@
 #include "RenderBox.h"
 #include "RenderTheme.h"
+#include "ScriptDisallowedScope.h"
 #include "Settings.h"
 #include "StyleTreeResolver.h"
@@ -558 +559 @@
 void HTMLFormControlElement::didChangeForm()
 {
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
     FormAssociatedElement::didChangeForm();
-    if (RefPtr<HTMLFormElement> form = this->form()) {
+    if (auto* form = this->form()) {
         if (m_willValidateInitialized && m_willValidate && !isValidFormControlElement())
             form->registerInvalidAssociatedFormControl(*this);

trunk/Source/WebCore/html/HTMLFormElement.cpp

@@ -705 +705 @@
     }
 
-    RefPtr<HTMLFormControlElement> oldDefault = m_defaultButton;
+    ScriptDisallowedScope::InMainThread scriptDisallowedScope;
+
+    auto* oldDefault = m_defaultButton;
     m_defaultButton = nullptr;
     defaultButton();