Crash in ElementDescendantIterator::operator--() when calling m_ances…

…torSiblingStack.last() https://bugs.webkit.org/show_bug.cgi?id=156715 <rdar://problem/25750864> Reviewed by Antti Koivisto. Source/WebCore: Fix correctness of ElementDescendantIterator::operator--(). The last element in the m_ancestorSiblingStack stack is nullptr. However, if our parent does not have a sibling, m_current->nextSibling() == m_ancestorSiblingStack.last() would be true and we would end up removing the nullptr element from m_ancestorSiblingStack. We would crash on a follow-up call to operator--() because m_ancestorSiblingStack.last() would do an out-of-bound access, given that m_ancestorSiblingStack is empty. Test: fast/dom/collection-backward-traversal-crash.html * dom/ElementDescendantIterator.h: (WebCore::ElementDescendantIterator::operator--): LayoutTests: Add regression test that reproduced the crash. * fast/dom/collection-backward-traversal-crash-expected.txt: Added. * fast/dom/collection-backward-traversal-crash.html: Added. Canonical link: https://commits.webkit.org/174828@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199693 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Apr 18, 2016 · 06201a0014763ee516e654c13c7f9d82a37c7ef8 · 06201a0
1 parent ac82cbf
commit 06201a0014763ee516e654c13c7f9d82a37c7ef8
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2016-04-18  Chris Dumez  <cdumez@apple.com>
+
+        Crash in ElementDescendantIterator::operator--() when calling m_ancestorSiblingStack.last()
+        https://bugs.webkit.org/show_bug.cgi?id=156715
+        <rdar://problem/25750864>
+
+        Reviewed by Antti Koivisto.
+
+        Add regression test that reproduced the crash.
+
+        * fast/dom/collection-backward-traversal-crash-expected.txt: Added.
+        * fast/dom/collection-backward-traversal-crash.html: Added.
+
 2016-04-18  Brent Fulgham  <bfulgham@apple.com>
 
         CSP: Remove stubs for dynamically-added favicons (via link rel="icon")

diff --git a/LayoutTests/fast/dom/collection-backward-traversal-crash-expected.txt b/LayoutTests/fast/dom/collection-backward-traversal-crash-expected.txt
@@ -0,0 +1,13 @@
+Tests that backward traversal of an HTMLCollection works as expected and does not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS divs.item(3).id is "D"
+PASS divs.item(2).id is "C"
+PASS divs.item(1).id is "B"
+PASS divs.item(0).id is "A"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/collection-backward-traversal-crash.html b/LayoutTests/fast/dom/collection-backward-traversal-crash.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<div id="testArea"><div id="A"><div id="B"><div id="C"><div id="D"></div></div></div></div></div>
+<script>
+description("Tests that backward traversal of an HTMLCollection works as expected and does not crash.");
+
+var testArea = document.getElementById("testArea");
+var divs = testArea.getElementsByTagName("div");
+shouldBeEqualToString("divs.item(3).id", "D");
+shouldBeEqualToString("divs.item(2).id", "C");
+shouldBeEqualToString("divs.item(1).id", "B");
+shouldBeEqualToString("divs.item(0).id", "A");
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
+2016-04-18  Chris Dumez  <cdumez@apple.com>
+
+        Crash in ElementDescendantIterator::operator--() when calling m_ancestorSiblingStack.last()
+        https://bugs.webkit.org/show_bug.cgi?id=156715
+        <rdar://problem/25750864>
+
+        Reviewed by Antti Koivisto.
+
+        Fix correctness of ElementDescendantIterator::operator--(). The last element
+        in the m_ancestorSiblingStack stack is nullptr. However, if our parent does
+        not have a sibling, m_current->nextSibling() == m_ancestorSiblingStack.last()
+        would be true and we would end up removing the nullptr element from
+        m_ancestorSiblingStack. We would crash on a follow-up call to operator--()
+        because m_ancestorSiblingStack.last() would do an out-of-bound access, given
+        that m_ancestorSiblingStack is empty.
+
+        Test: fast/dom/collection-backward-traversal-crash.html
+
+        * dom/ElementDescendantIterator.h:
+        (WebCore::ElementDescendantIterator::operator--):
+
 2016-04-18  Anders Carlsson  <andersca@apple.com>
 
         Fix build with newer versions of clang.

diff --git a/Source/WebCore/dom/ElementDescendantIterator.h b/Source/WebCore/dom/ElementDescendantIterator.h
@@ -171,7 +171,7 @@ ALWAYS_INLINE ElementDescendantIterator& ElementDescendantIterator::operator--()
     if (!previousSibling) {
         m_current = m_current->parentElement();
         // The stack optimizes for forward traversal only, this just maintains consistency.
-        if (m_current->nextSibling() == m_ancestorSiblingStack.last())
+        if (m_current->nextSibling() && m_current->nextSibling() == m_ancestorSiblingStack.last())
             m_ancestorSiblingStack.removeLast();
         return *this;
     }