GuardMalloc crash in WebCore::HTMLFrameElementBase::marginHeight()
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Apr 2016 16:45:13 +0000 (16:45 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Apr 2016 16:45:13 +0000 (16:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157020
<rdar://problem/25148315>

Reviewed by Darin Adler.

Calls to setIntegralAttribute triggers event handling code, which can cause
the underlying m_frameOwnerElement member to be deleted. We could clone this
object, but since we only want the width and height we should just read them
while we know the object is in a good state, then execute the potentially
mutating methods.

Tested by imported/blink/fast/dom/HTMLBodyElement/body-inserting-iframe-crash.html.

* html/HTMLBodyElement.cpp:
(WebCore::HTMLBodyElement::insertedInto): Read margin width and height before
calling setIntegralAttribute.

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

Source/WebCore/ChangeLog
Source/WebCore/html/HTMLBodyElement.cpp

index 219b9a3..1c82ca1 100644 (file)
@@ -1,3 +1,23 @@
+2016-04-26  Brent Fulgham  <bfulgham@apple.com>
+
+        GuardMalloc crash in WebCore::HTMLFrameElementBase::marginHeight() 
+        https://bugs.webkit.org/show_bug.cgi?id=157020
+        <rdar://problem/25148315>
+
+        Reviewed by Darin Adler.
+
+        Calls to setIntegralAttribute triggers event handling code, which can cause
+        the underlying m_frameOwnerElement member to be deleted. We could clone this
+        object, but since we only want the width and height we should just read them
+        while we know the object is in a good state, then execute the potentially
+        mutating methods.
+
+        Tested by imported/blink/fast/dom/HTMLBodyElement/body-inserting-iframe-crash.html.
+
+        * html/HTMLBodyElement.cpp:
+        (WebCore::HTMLBodyElement::insertedInto): Read margin width and height before
+        calling setIntegralAttribute.
+
 2016-04-25  Filip Pizlo  <fpizlo@apple.com>
 
         WebCore on Mac ignores the user's preferred region (country) while getting the language
index fc6062c..e3468a0 100644 (file)
@@ -193,16 +193,21 @@ Node::InsertionNotificationRequest HTMLBodyElement::insertedInto(ContainerNode&
     // FIXME: It's surprising this is web compatible since it means a marginwidth and marginheight attribute can
     // magically appear on the <body> of all documents embedded through <iframe> or <frame>.
     // FIXME: Perhaps this code should be in attach() instead of here.
-    HTMLFrameOwnerElement* ownerElement = document().ownerElement();
-    if (is<HTMLFrameElementBase>(ownerElement)) {
-        HTMLFrameElementBase& ownerFrameElement = downcast<HTMLFrameElementBase>(*ownerElement);
-        int marginWidth = ownerFrameElement.marginWidth();
-        if (marginWidth != -1)
-            setIntegralAttribute(marginwidthAttr, marginWidth);
-        int marginHeight = ownerFrameElement.marginHeight();
-        if (marginHeight != -1)
-            setIntegralAttribute(marginheightAttr, marginHeight);
-    }
+    auto* ownerElement = document().ownerElement();
+    if (!is<HTMLFrameElementBase>(ownerElement))
+        return InsertionDone;
+    
+    auto& ownerFrameElement = downcast<HTMLFrameElementBase>(*ownerElement);
+
+    // Read values from the owner before setting any attributes, since setting an attribute can run arbitrary
+    // JavaScript, which might delete the owner element.
+    int marginWidth = ownerFrameElement.marginWidth();
+    int marginHeight = ownerFrameElement.marginHeight();
+
+    if (marginWidth != -1)
+        setIntegralAttribute(marginwidthAttr, marginWidth);
+    if (marginHeight != -1)
+        setIntegralAttribute(marginheightAttr, marginHeight);
 
     return InsertionDone;
 }