Changeset 137632 in webkit

Timestamp:

Dec 13, 2012 11:48:26 AM (11 years ago)

Author:

Dave Barton

Message:

Heap-use-after-free in WebCore::RenderBlock::finishDelayUpdateScrollInfo
​https://bugs.webkit.org/show_bug.cgi?id=103750

Reviewed by Tony Chang.

Source/WebCore:

MathML sometimes creates and destroys renderers for descendants during layout (or even to calculate
preferred logical widths), e.g. for operator stretching. RenderBlock::finishDelayUpdateScrollInfo
must therefore leave gDelayedUpdateScrollInfoSet intact as it iterates over it, so
RenderBlock::willBeDestroyed can call gDelayedUpdateScrollInfoSet->remove(this) effectively if needed.
This also prevents duplicate entries from being added to gDelayedUpdateScrollInfoSet.

Test: mathml/mo-stretch-crash.html

• rendering/RenderBlock.cpp:

(WebCore::RenderBlock::startDelayUpdateScrollInfo):

• Allow gDelayedUpdateScrollInfoSet to be non-null when gDelayUpdateScrollInfo is 0 during RenderBlock::finishDelayUpdateScrollInfo.

(WebCore::RenderBlock::finishDelayUpdateScrollInfo):

• Remove blocks from gDelayedUpdateScrollInfoSet one at a time, waiting for each block until it is about to be updated.

LayoutTests:

• mathml/mo-stretch-crash-expected.txt: Added.
• mathml/mo-stretch-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/mathml/mo-stretch-crash-expected.txt (added)
• LayoutTests/mathml/mo-stretch-crash.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderBlock.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-12-13  David Barton  <dbarton@mathscribe.com>
+
+        Heap-use-after-free in WebCore::RenderBlock::finishDelayUpdateScrollInfo
+        https://bugs.webkit.org/show_bug.cgi?id=103750
+
+        Reviewed by Tony Chang.
+
+        * mathml/mo-stretch-crash-expected.txt: Added.
+        * mathml/mo-stretch-crash.html: Added.
+
 2012-12-13  Joanmarie Diggs  <jdiggs@igalia.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-12-13  David Barton  <dbarton@mathscribe.com>
+
+        Heap-use-after-free in WebCore::RenderBlock::finishDelayUpdateScrollInfo
+        https://bugs.webkit.org/show_bug.cgi?id=103750
+
+        Reviewed by Tony Chang.
+
+        MathML sometimes creates and destroys renderers for descendants during layout (or even to calculate
+        preferred logical widths), e.g. for operator stretching. RenderBlock::finishDelayUpdateScrollInfo
+        must therefore leave gDelayedUpdateScrollInfoSet intact as it iterates over it, so
+        RenderBlock::willBeDestroyed can call gDelayedUpdateScrollInfoSet->remove(this) effectively if needed.
+        This also prevents duplicate entries from being added to gDelayedUpdateScrollInfoSet.
+
+        Test: mathml/mo-stretch-crash.html
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::startDelayUpdateScrollInfo):
+            - Allow gDelayedUpdateScrollInfoSet to be non-null when gDelayUpdateScrollInfo is 0 during
+              RenderBlock::finishDelayUpdateScrollInfo.
+        (WebCore::RenderBlock::finishDelayUpdateScrollInfo):
+            - Remove blocks from gDelayedUpdateScrollInfoSet one at a time, waiting for each block until it is
+              about to be updated.
+
 2012-12-13  Alexey Proskuryakov  <ap@apple.com>
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -1290 +1290 @@
 void RenderBlock::startDelayUpdateScrollInfo()
 {
-    if (gDelayUpdateScrollInfo == 0) {
-        ASSERT(!gDelayedUpdateScrollInfoSet);
+    if (!gDelayedUpdateScrollInfoSet) {
+        ASSERT(!gDelayUpdateScrollInfo);
         gDelayedUpdateScrollInfoSet = new DelayedUpdateScrollInfoSet;
     }
@@ -1305 +1305 @@
         ASSERT(gDelayedUpdateScrollInfoSet);
 
-        OwnPtr<DelayedUpdateScrollInfoSet> infoSet(adoptPtr(gDelayedUpdateScrollInfoSet));
+        Vector<RenderBlock*> infoSet;
+        while (gDelayedUpdateScrollInfoSet && gDelayedUpdateScrollInfoSet->size()) {
+            copyToVector(*gDelayedUpdateScrollInfoSet, infoSet);
+            for (Vector<RenderBlock*>::iterator it = infoSet.begin(); it != infoSet.end(); ++it) {
+                RenderBlock* block = *it;
+                // |block| may have been destroyed at this point, but then it will have been removed from gDelayedUpdateScrollInfoSet.
+                if (gDelayedUpdateScrollInfoSet && gDelayedUpdateScrollInfoSet->contains(block)) {
+                    gDelayedUpdateScrollInfoSet->remove(block);
+                    if (block->hasOverflowClip())
+                        block->layer()->updateScrollInfoAfterLayout();
+                }
+            }
+        }
+        delete gDelayedUpdateScrollInfoSet;
         gDelayedUpdateScrollInfoSet = 0;
-
-        for (DelayedUpdateScrollInfoSet::iterator it = infoSet->begin(); it != infoSet->end(); ++it) {
-            RenderBlock* block = *it;
-            if (block->hasOverflowClip()) {
-                block->layer()->updateScrollInfoAfterLayout();
-            }
-        }
+        ASSERT(!gDelayUpdateScrollInfo);
     }
 }