REGRESSION (r167879): Heap-use-after-free in WebCore::RenderFlexibleBox
authorrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Apr 2014 17:34:34 +0000 (17:34 +0000)
committerrego@igalia.com <rego@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Apr 2014 17:34:34 +0000 (17:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132337

Reviewed by Simon Fraser.

From Blink r154582 by <jchaffraix@chromium.org>

Source/WebCore:
This is a regression from the changes in OrderIterator. The issue is
that we don't invalidate our iterator when a child is removed. This
means that we could hold onto free'd memory until the next layout
when we will recompute the iterator.

The solution is simple: just clear the memory when we remove a child.

Note that RenderGrid is not impacted by this bug as we don't use the
iterator outside layout yet, but if we do it at some point the very same
problem will arise, so the same treatment was applied to the class.

Test: fast/flexbox/order-iterator-crash.html

* rendering/OrderIterator.cpp:
(WebCore::OrderIterator::invalidate): Clear m_children Vector.
* rendering/OrderIterator.h:
(WebCore::OrderIteratorPopulator::OrderIteratorPopulator): Use
invalidate() method.
* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::removeChild): Invalidate m_orderIterator.
* rendering/RenderFlexibleBox.h: Add removeChild() signature.
* rendering/RenderGrid.cpp: Invalidate m_orderIterator.
(WebCore::RenderGrid::removeChild):
* rendering/RenderGrid.h: Add removeChild() header.

LayoutTests:
Add new layout test to check that removing a child doesn't cause a crash
in OrderIterator.

* fast/flexbox/order-iterator-crash-expected.txt: Added.
* fast/flexbox/order-iterator-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/flexbox/order-iterator-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/flexbox/order-iterator-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/OrderIterator.cpp
Source/WebCore/rendering/OrderIterator.h
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/RenderFlexibleBox.h
Source/WebCore/rendering/RenderGrid.cpp
Source/WebCore/rendering/RenderGrid.h

index cd0b540..24d039d 100644 (file)
@@ -1,3 +1,18 @@
+2014-04-29  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        REGRESSION (r167879): Heap-use-after-free in WebCore::RenderFlexibleBox
+        https://bugs.webkit.org/show_bug.cgi?id=132337
+
+        Reviewed by Simon Fraser.
+
+        From Blink r154582 by <jchaffraix@chromium.org>
+
+        Add new layout test to check that removing a child doesn't cause a crash
+        in OrderIterator.
+
+        * fast/flexbox/order-iterator-crash-expected.txt: Added.
+        * fast/flexbox/order-iterator-crash.html: Added.
+
 2014-04-29  Hans Muller  <hmuller@adobe.com>
 
         [CSS Shapes] off-by-one error in Shape::createRasterShape()
diff --git a/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt b/LayoutTests/fast/flexbox/order-iterator-crash-expected.txt
new file mode 100644 (file)
index 0000000..293e685
--- /dev/null
@@ -0,0 +1,3 @@
+This test has passed if it doesn't crash.
+* { display: -webkit-flex; }
+if (window.testRunner) testRunner.dumpAsText(); crashy.offsetLeft; crashy.parentNode.removeChild(crashy);
diff --git a/LayoutTests/fast/flexbox/order-iterator-crash.html b/LayoutTests/fast/flexbox/order-iterator-crash.html
new file mode 100644 (file)
index 0000000..037121b
--- /dev/null
@@ -0,0 +1,12 @@
+<div>This test has passed if it doesn't crash.</div>
+<style>
+* { display: -webkit-flex; }
+</style>
+<table><td id="crashy"></td></table>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+crashy.offsetLeft;
+crashy.parentNode.removeChild(crashy);
+</script>
index 3d69858..85608ee 100644 (file)
@@ -1,3 +1,37 @@
+2014-04-29  Manuel Rego Casasnovas  <rego@igalia.com>
+
+        REGRESSION (r167879): Heap-use-after-free in WebCore::RenderFlexibleBox
+        https://bugs.webkit.org/show_bug.cgi?id=132337
+
+        Reviewed by Simon Fraser.
+
+        From Blink r154582 by <jchaffraix@chromium.org>
+
+        This is a regression from the changes in OrderIterator. The issue is
+        that we don't invalidate our iterator when a child is removed. This
+        means that we could hold onto free'd memory until the next layout
+        when we will recompute the iterator.
+
+        The solution is simple: just clear the memory when we remove a child.
+
+        Note that RenderGrid is not impacted by this bug as we don't use the
+        iterator outside layout yet, but if we do it at some point the very same
+        problem will arise, so the same treatment was applied to the class.
+
+        Test: fast/flexbox/order-iterator-crash.html
+
+        * rendering/OrderIterator.cpp:
+        (WebCore::OrderIterator::invalidate): Clear m_children Vector.
+        * rendering/OrderIterator.h:
+        (WebCore::OrderIteratorPopulator::OrderIteratorPopulator): Use
+        invalidate() method.
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::removeChild): Invalidate m_orderIterator.
+        * rendering/RenderFlexibleBox.h: Add removeChild() signature.
+        * rendering/RenderGrid.cpp: Invalidate m_orderIterator.
+        (WebCore::RenderGrid::removeChild):
+        * rendering/RenderGrid.h: Add removeChild() header.
+
 2014-04-29  Enrica Casucci  <enrica@apple.com>
 
         iOS build fix after http://trac.webkit.org/changeset/167937.
index 987f552..7630136 100644 (file)
@@ -56,6 +56,11 @@ RenderBox* OrderIterator::next()
     return currentChild();
 }
 
+void OrderIterator::invalidate()
+{
+    m_children.clear();
+}
+
 static bool compareByOrderValueAndIndex(std::pair<RenderBox*, int> childAndIndex1, std::pair<RenderBox*, int> childAndIndex2)
 {
     if (childAndIndex1.first->style().order() != childAndIndex2.first->style().order())
index 1d1f58a..27f89bf 100644 (file)
@@ -47,6 +47,8 @@ public:
     RenderBox* first();
     RenderBox* next();
 
+    void invalidate();
+
 private:
     void reset();
 
@@ -61,7 +63,7 @@ public:
         , m_childIndex(0)
         , m_allChildrenHaveDefaultOrderValue(true)
     {
-        m_iterator.m_children.clear();
+        m_iterator.invalidate();
     }
 
     ~OrderIteratorPopulator();
index 612c4d3..fb41e8c 100644 (file)
@@ -1392,4 +1392,10 @@ bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
     return isHorizontalFlow();
 }
 
+void RenderFlexibleBox::removeChild(RenderObject& child)
+{
+    RenderBlock::removeChild(child);
+    m_orderIterator.invalidate();
+}
+
 }
index 35852c9..a3594f6 100644 (file)
@@ -60,6 +60,8 @@ public:
     bool isTopLayoutOverflowAllowed() const override;
     bool isLeftLayoutOverflowAllowed() const override;
 
+    void removeChild(RenderObject&) override;
+
 protected:
     virtual void computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const override;
     virtual void computePreferredLogicalWidths() override;
index 13ae483..9c17b09 100644 (file)
@@ -1186,6 +1186,12 @@ const char* RenderGrid::renderName() const
     return "RenderGrid";
 }
 
+void RenderGrid::removeChild(RenderObject& child)
+{
+    RenderBlock::removeChild(child);
+    m_orderIterator.invalidate();
+}
+
 } // namespace WebCore
 
 #endif /* ENABLE(CSS_GRID_LAYOUT) */
index 8184ecf..d1a3e8f 100644 (file)
@@ -58,6 +58,8 @@ public:
     const Vector<LayoutUnit>& columnPositions() const { return m_columnPositions; }
     const Vector<LayoutUnit>& rowPositions() const { return m_rowPositions; }
 
+    void removeChild(RenderObject&) override;
+
 private:
     virtual const char* renderName() const override;
     virtual bool isRenderGrid() const override { return true; }