Title: [116827] trunk
Revision
116827
Author
commit-qu...@webkit.org
Date
2012-05-11 17:28:32 -0700 (Fri, 11 May 2012)

Log Message

use after free in WebCore::RenderObject::document
https://bugs.webkit.org/show_bug.cgi?id=84891

Patch by David Barton <dbar...@mathscribe.com> on 2012-05-11
Reviewed by Julien Chaffraix.

Source/WebCore:

Change RenderMathMLFenced::addChild() to use the beforeChild parameter. When beforeChild
is 0, insert child renderers before the closing fence, which might not be the same as
this->lastChild(), e.g. possibly due to anonymous blocks or generated content.

Tests: mathml/presentation/mfenced-add-child1-expected.html
       mathml/presentation/mfenced-add-child1.html
       mathml/presentation/mfenced-add-child2-expected.html
       mathml/presentation/mfenced-add-child2.html

* rendering/mathml/RenderMathMLFenced.cpp:
(WebCore::RenderMathMLFenced::RenderMathMLFenced):
(WebCore::RenderMathMLFenced::makeFences):
(WebCore::RenderMathMLFenced::addChild):
* rendering/mathml/RenderMathMLFenced.h:
(RenderMathMLFenced):

LayoutTests:

* mathml/presentation/mfenced-add-child1-expected.html: Added.
* mathml/presentation/mfenced-add-child1.html: Added.
* mathml/presentation/mfenced-add-child2-expected.html: Added.
* mathml/presentation/mfenced-add-child2.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (116826 => 116827)


--- trunk/LayoutTests/ChangeLog	2012-05-12 00:17:45 UTC (rev 116826)
+++ trunk/LayoutTests/ChangeLog	2012-05-12 00:28:32 UTC (rev 116827)
@@ -1,3 +1,15 @@
+2012-05-11  David Barton  <dbar...@mathscribe.com>
+
+        use after free in WebCore::RenderObject::document
+        https://bugs.webkit.org/show_bug.cgi?id=84891
+
+        Reviewed by Julien Chaffraix.
+
+        * mathml/presentation/mfenced-add-child1-expected.html: Added.
+        * mathml/presentation/mfenced-add-child1.html: Added.
+        * mathml/presentation/mfenced-add-child2-expected.html: Added.
+        * mathml/presentation/mfenced-add-child2.html: Added.
+
 2012-05-10  Timothy Hatcher  <timo...@apple.com>
 
         Instrument timer function calls so they show up in the Web Inspector Timeline.

Added: trunk/LayoutTests/mathml/presentation/mfenced-add-child1-expected.html (0 => 116827)


--- trunk/LayoutTests/mathml/presentation/mfenced-add-child1-expected.html	                        (rev 0)
+++ trunk/LayoutTests/mathml/presentation/mfenced-add-child1-expected.html	2012-05-12 00:28:32 UTC (rev 116827)
@@ -0,0 +1,9 @@
+<html>
+<body>
+<p>For <a href="" 84891</a> -
+mfenced add/remove child confusion.</p>
+
+<p>This test passes if the mfenced is completely removed from the render tree,
+so e.g. no parentheses or commas are showing after this paragraph.</p>
+</body>
+</html>

Added: trunk/LayoutTests/mathml/presentation/mfenced-add-child1.html (0 => 116827)


--- trunk/LayoutTests/mathml/presentation/mfenced-add-child1.html	                        (rev 0)
+++ trunk/LayoutTests/mathml/presentation/mfenced-add-child1.html	2012-05-12 00:28:32 UTC (rev 116827)
@@ -0,0 +1,30 @@
+<html>
+<body>
+<p>For <a href="" 84891</a> -
+mfenced add/remove child confusion.</p>
+
+<p>This test passes if the mfenced is completely removed from the render tree,
+so e.g. no parentheses or commas are showing after this paragraph.</p>
+
+<math>
+  <mfenced id="mfenced">
+    <mi id="mi1">b</mi>
+    <mi id="mi2">c</mi>
+  </mfenced>
+</math>
+</body>
+
+<script>
+var mfenced = document.getElementById("mfenced");
+
+document.getElementById("mi1").appendChild(document.createElement("div"));
+mfenced.appendChild(document.createElement("span"));
+
+var head = document.getElementsByTagName("head")[0];
+var style = document.createElement("style");
+style.appendChild(document.createTextNode("script{}"));
+head.appendChild(style);
+
+mfenced.parentNode.removeChild(mfenced);
+</script>
+</html>

Added: trunk/LayoutTests/mathml/presentation/mfenced-add-child2-expected.html (0 => 116827)


--- trunk/LayoutTests/mathml/presentation/mfenced-add-child2-expected.html	                        (rev 0)
+++ trunk/LayoutTests/mathml/presentation/mfenced-add-child2-expected.html	2012-05-12 00:28:32 UTC (rev 116827)
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>For <a href="" 84891</a> -
+mfenced add/remove child confusion.</p>
+
+<p>This test passes if the math element is changed to (x1y).</p>
+
+<math>
+  <mfenced id="mfenced" separators="1">
+    <mi id="mi1">x</mi>
+    <mi id="mi2">y</mi>
+  </mfenced>
+</math>
+</body>
+</html>

Added: trunk/LayoutTests/mathml/presentation/mfenced-add-child2.html (0 => 116827)


--- trunk/LayoutTests/mathml/presentation/mfenced-add-child2.html	                        (rev 0)
+++ trunk/LayoutTests/mathml/presentation/mfenced-add-child2.html	2012-05-12 00:28:32 UTC (rev 116827)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>For <a href="" 84891</a> -
+mfenced add/remove child confusion.</p>
+
+<p>This test passes if the math element is changed to (x1y).</p>
+
+<math>
+  <mfenced id="mfenced" separators="12"></mfenced>
+  <mi id="mi1">x</mi>
+  <mi id="mi2">y</mi>
+</math>
+
+<script>
+var mfenced = document.getElementById("mfenced");
+var mi1 = document.getElementById("mi1"), mi2 = document.getElementById("mi2");
+mfenced.appendChild(mi2);
+mfenced.insertBefore(mi1, mi2);
+</script>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (116826 => 116827)


--- trunk/Source/WebCore/ChangeLog	2012-05-12 00:17:45 UTC (rev 116826)
+++ trunk/Source/WebCore/ChangeLog	2012-05-12 00:28:32 UTC (rev 116827)
@@ -1,3 +1,26 @@
+2012-05-11  David Barton  <dbar...@mathscribe.com>
+
+        use after free in WebCore::RenderObject::document
+        https://bugs.webkit.org/show_bug.cgi?id=84891
+
+        Reviewed by Julien Chaffraix.
+
+        Change RenderMathMLFenced::addChild() to use the beforeChild parameter. When beforeChild
+        is 0, insert child renderers before the closing fence, which might not be the same as
+        this->lastChild(), e.g. possibly due to anonymous blocks or generated content.
+
+        Tests: mathml/presentation/mfenced-add-child1-expected.html
+               mathml/presentation/mfenced-add-child1.html
+               mathml/presentation/mfenced-add-child2-expected.html
+               mathml/presentation/mfenced-add-child2.html
+
+        * rendering/mathml/RenderMathMLFenced.cpp:
+        (WebCore::RenderMathMLFenced::RenderMathMLFenced):
+        (WebCore::RenderMathMLFenced::makeFences):
+        (WebCore::RenderMathMLFenced::addChild):
+        * rendering/mathml/RenderMathMLFenced.h:
+        (RenderMathMLFenced):
+
 2012-05-11  Anders Carlsson  <ander...@apple.com>
 
         Can't scroll on webpage after following links from Blogger

Modified: trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp (116826 => 116827)


--- trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp	2012-05-12 00:17:45 UTC (rev 116826)
+++ trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp	2012-05-12 00:28:32 UTC (rev 116827)
@@ -48,6 +48,7 @@
     : RenderMathMLRow(element)
     , m_open(OpeningBraceChar)
     , m_close(ClosingBraceChar)
+    , m_closeFenceRenderer(0)
 {
 }
 
@@ -94,49 +95,67 @@
     RenderObject* openFence = new (renderArena()) RenderMathMLOperator(node(), m_open);
     openFence->setStyle(createOperatorStyle());
     RenderBlock::addChild(openFence, firstChild());
-    RenderObject* closeFence = new (renderArena()) RenderMathMLOperator(node(), m_close);
-    closeFence->setStyle(createOperatorStyle());
-    RenderBlock::addChild(closeFence);
+    m_closeFenceRenderer = new (renderArena()) RenderMathMLOperator(node(), m_close);
+    m_closeFenceRenderer->setStyle(createOperatorStyle());
+    RenderBlock::addChild(m_closeFenceRenderer);
 }
 
-void RenderMathMLFenced::addChild(RenderObject* child, RenderObject*)
+void RenderMathMLFenced::addChild(RenderObject* child, RenderObject* beforeChild)
 {
     // make the fences if the render object is empty
     if (isEmpty())
         updateFromElement();
     
+    // FIXME: Adding or removing a child should possibly cause all later separators to shift places if they're different,
+    // as later child positions change by +1 or -1.
+    
+    RenderObject* separatorRenderer = 0;
     if (m_separators.get()) {
         unsigned int count = 0;
         for (Node* position = child->node(); position; position = position->previousSibling()) {
-            if (position->nodeType() == Node::ELEMENT_NODE)
+            if (position->isElementNode())
                 count++;
         }
-                
-        if (count > 1) {
+        if (!beforeChild) {
+            // We're adding at the end (before the closing fence), so a new separator would go before the new child, not after it.
+            --count;
+        }
+        // |count| is now the number of element children that will be before our new separator, i.e. it's the 1-based index of the separator.
+        
+        if (count > 0) {
             UChar separator;
             
             // Use the last separator if we've run out of specified separators.
-            if ((count - 1) >= m_separators.get()->length())
+            if (count > m_separators.get()->length())
                 separator = (*m_separators.get())[m_separators.get()->length() - 1];
             else
-                separator = (*m_separators.get())[count - 2];
+                separator = (*m_separators.get())[count - 1];
                 
-            RenderObject* separatorObj = new (renderArena()) RenderMathMLOperator(node(), separator);
-            separatorObj->setStyle(createOperatorStyle());
-            RenderBlock::addChild(separatorObj, lastChild());
+            separatorRenderer = new (renderArena()) RenderMathMLOperator(node(), separator);
+            separatorRenderer->setStyle(createOperatorStyle());
         }
     }
     
     // If we have a block, we'll wrap it in an inline-block.
     if (child->isBlockFlow() && child->style()->display() != INLINE_BLOCK) {
         // Block objects wrapper.
-
         RenderBlock* block = createAlmostAnonymousBlock(INLINE_BLOCK);
         
-        RenderBlock::addChild(block, lastChild());
-        block->addChild(child);    
-    } else
-        RenderBlock::addChild(child, lastChild());
+        block->addChild(child);
+        child = block;
+    }
+    
+    if (beforeChild) {
+        // Adding |x| before an existing |y| e.g. in element (y) - first insert our new child |x|, then its separator, to get (x, y).
+        RenderBlock::addChild(child, beforeChild);
+        if (separatorRenderer)
+            RenderBlock::addChild(separatorRenderer, beforeChild);
+    } else {
+        // Adding |y| at the end of an existing element e.g. (x) - insert the separator first before the closing fence, then |y|, to get (x, y).
+        if (separatorRenderer)
+            RenderBlock::addChild(separatorRenderer, m_closeFenceRenderer);
+        RenderBlock::addChild(child, m_closeFenceRenderer);
+    }
 }
 
 }    

Modified: trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.h (116826 => 116827)


--- trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.h	2012-05-12 00:17:45 UTC (rev 116826)
+++ trunk/Source/WebCore/rendering/mathml/RenderMathMLFenced.h	2012-05-12 00:28:32 UTC (rev 116827)
@@ -47,6 +47,8 @@
     UChar m_open;
     UChar m_close;
     RefPtr<StringImpl> m_separators;
+    
+    RenderObject* m_closeFenceRenderer;
 };
     
 }
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to