[SVG] OOB access in SVGListProperty::replaceItemValues()

https://bugs.webkit.org/show_bug.cgi?id=109293

Source/WebCore:

Replacing a list property item with itself should be a no-op. This patch updates the related
APIs and logic to detect the self-replace case and prevent removal of the item from the list.

To avoid scanning the list multiple times, removeItemFromList() is updated to operate on
indices and a findItem() method is added to resolve an item to an index.

Reviewed by Dirk Schulze.

No new tests: updated existing tests cover the change.

* svg/properties/SVGAnimatedListPropertyTearOff.h:
(WebCore::SVGAnimatedListPropertyTearOff::findItem):
(SVGAnimatedListPropertyTearOff):
(WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
* svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
(WebCore::SVGAnimatedPathSegListPropertyTearOff::findItem):
(SVGAnimatedPathSegListPropertyTearOff):
(WebCore::SVGAnimatedPathSegListPropertyTearOff::removeItemFromList):
Add a findItem() delegating method, and update removeItemFromList() to use the new
index-based API.

* svg/properties/SVGListProperty.h:
(WebCore::SVGListProperty::insertItemBeforeValues):
(WebCore::SVGListProperty::insertItemBeforeValuesAndWrappers):
(WebCore::SVGListProperty::replaceItemValues):
(WebCore::SVGListProperty::replaceItemValuesAndWrappers):
(SVGListProperty):
Updated to handle the no-op case for insertItemBefore() & replaceItem().

* svg/properties/SVGListPropertyTearOff.h:
(WebCore::SVGListPropertyTearOff::findItem):
(WebCore::SVGListPropertyTearOff::removeItemFromList):
Index-based API updates.

(WebCore::SVGListPropertyTearOff::processIncomingListItemValue):
(WebCore::SVGListPropertyTearOff::processIncomingListItemWrapper):
* svg/properties/SVGPathSegListPropertyTearOff.cpp:
(WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemValue):
Detect the self-replace case and return without removing the item from the list.

* svg/properties/SVGPathSegListPropertyTearOff.h:
(WebCore::SVGPathSegListPropertyTearOff::findItem):
(WebCore::SVGPathSegListPropertyTearOff::removeItemFromList):
(SVGPathSegListPropertyTearOff):
(WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemWrapper):
* svg/properties/SVGStaticListPropertyTearOff.h:
(WebCore::SVGStaticListPropertyTearOff::processIncomingListItemValue):
(WebCore::SVGStaticListPropertyTearOff::processIncomingListItemWrapper):
Index-based API updates.

Change-Id: I6afa8ff964eaccd1337156af41c5ded07e3b4b7d
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@142759 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Reviewed-by: Jocelyn Turcotte <jocelyn.turcotte@digia.com>

7 files changed, 107 insertions, 54 deletions

diff --git a/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h
index 6be9b1c31..65ee9e58d 100644
--- a/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h
@@ -56,12 +56,19 @@ public:
 
     virtual bool isAnimatedListTearOff() const { return true; }
 
-    int removeItemFromList(SVGProperty* property, bool shouldSynchronizeWrappers)
+    int findItem(SVGProperty* property) const
     {
         // This should ever be called for our baseVal, as animVal can't modify the list.
         // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
         typedef SVGPropertyTearOff<typename SVGPropertyTraits<PropertyType>::ListItemType> ListItemTearOff;
-        return static_cast<ListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(static_cast<ListItemTearOff*>(property), shouldSynchronizeWrappers);
+        return static_cast<ListPropertyTearOff*>(m_baseVal.get())->findItem(static_cast<ListItemTearOff*>(property));
+    }
+
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        // This should ever be called for our baseVal, as animVal can't modify the list.
+        // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
+        static_cast<ListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     }
 
     void detachListWrappers(unsigned newListSize)
diff --git a/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
index db910fe90..f1268f8fd 100644
--- a/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
@@ -46,11 +46,18 @@ public:
         return static_cast<SVGListProperty<SVGPathSegList>*>(m_animVal.get());
     }
 
-    int removeItemFromList(const RefPtr<SVGPathSeg>& segment, bool shouldSynchronizeWrappers)
+    int findItem(const RefPtr<SVGPathSeg>& segment) const
     {
         // This should ever be called for our baseVal, as animVal can't modify the list.
         ASSERT(m_baseVal);
-        return static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(segment, shouldSynchronizeWrappers);
+        return static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->findItem(segment);
+    }
+
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        // This should ever be called for our baseVal, as animVal can't modify the list.
+        ASSERT(m_baseVal);
+        static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     }
 
     static PassRefPtr<SVGAnimatedPathSegListPropertyTearOff> create(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType, SVGPathSegList& values)
diff --git a/Source/WebCore/svg/properties/SVGListProperty.h b/Source/WebCore/svg/properties/SVGListProperty.h
index 9db2f4ceb..5a202dfaf 100644
--- a/Source/WebCore/svg/properties/SVGListProperty.h
+++ b/Source/WebCore/svg/properties/SVGListProperty.h
@@ -221,7 +221,10 @@ public:
             index = m_values->size();
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
-        processIncomingListItemValue(newItem, &index);
+        if (!processIncomingListItemValue(newItem, &index)) {
+            // Inserting the item before itself is a no-op.
+            return newItem;
+        }
 
         // Spec: Inserts a new item into the list at the specified position. The index of the item before which the new item is to be
         // inserted. The first item is number 0. If the index is equal to 0, then the new item is inserted at the front of the list.
@@ -251,7 +254,8 @@ public:
         ASSERT(m_values->size() == m_wrappers->size());
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
-        processIncomingListItemWrapper(newItem, &index);
+        if (!processIncomingListItemWrapper(newItem, &index))
+            return newItem.release();
 
         // Spec: Inserts a new item into the list at the specified position. The index of the item before which the new item is to be
         // inserted. The first item is number 0. If the index is equal to 0, then the new item is inserted at the front of the list.
@@ -285,7 +289,10 @@ public:
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // Spec: If the item is already in this list, note that the index of the item to replace is before the removal of the item.
-        processIncomingListItemValue(newItem, &index);
+        if (!processIncomingListItemValue(newItem, &index)) {
+            // Replacing the item with itself is a no-op.
+            return newItem;
+        }
 
         if (m_values->isEmpty()) {
             // 'newItem' already lived in our list, we removed it, and now we're empty, which means there's nothing to replace.
@@ -317,7 +324,8 @@ public:
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // Spec: If the item is already in this list, note that the index of the item to replace is before the removal of the item.
-        processIncomingListItemWrapper(newItem, &index);
+        if (!processIncomingListItemWrapper(newItem, &index))
+            return newItem.release();
 
         if (m_values->isEmpty()) {
             ASSERT(m_wrappers->isEmpty());
@@ -461,8 +469,8 @@ protected:
         commitChange();
     }
 
-    virtual void processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) = 0;
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify) = 0;
+    virtual bool processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) = 0;
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify) = 0;
 
     SVGPropertyRole m_role;
     bool m_ownsValues;
diff --git a/Source/WebCore/svg/properties/SVGListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGListPropertyTearOff.h
index c3bba4be1..a9dbec52b 100644
--- a/Source/WebCore/svg/properties/SVGListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGListPropertyTearOff.h
@@ -47,30 +47,35 @@ public:
         return adoptRef(new Self(animatedProperty, role, values, wrappers));
     }
 
-    int removeItemFromList(ListItemTearOff* removeItem, bool shouldSynchronizeWrappers)
+    int findItem(ListItemTearOff* item) const
     {
         ASSERT(m_values);
         ASSERT(m_wrappers);
 
-        // Lookup item in cache and remove its corresponding wrapper.
         unsigned size = m_wrappers->size();
         ASSERT(size == m_values->size());
-        for (unsigned i = 0; i < size; ++i) {
-            RefPtr<ListItemTearOff>& item = m_wrappers->at(i);
-            if (item != removeItem)
-                continue;
+        for (size_t i = 0; i < size; ++i) {
+            if (item == m_wrappers->at(i))
+                return i;
+        }
 
-            item->detachWrapper();
-            m_wrappers->remove(i);
-            m_values->remove(i);
+        return -1;
+    }
 
-            if (shouldSynchronizeWrappers)
-                commitChange();
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        ASSERT(m_values);
+        ASSERT(m_wrappers);
+        ASSERT(m_values->size() == m_wrappers->size());
+        ASSERT(itemIndex < m_wrappers->size());
 
-            return i;
-        }
+        RefPtr<ListItemTearOff>& item = m_wrappers->at(itemIndex);
+        item->detachWrapper();
+        m_wrappers->remove(itemIndex);
+        m_values->remove(itemIndex);
 
-        return -1;
+        if (shouldSynchronizeWrappers)
+            commitChange();
     }
 
     // SVGList API
@@ -144,19 +149,20 @@ protected:
         m_animatedProperty->commitChange();
     }
 
-    virtual void processIncomingListItemValue(const ListItemType&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify)
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify)
     {
         SVGAnimatedProperty* animatedPropertyOfItem = newItem->animatedProperty();
 
         // newItem has been created manually, it doesn't belong to any SVGElement.
         // (for example: "textElement.x.baseVal.appendItem(svgsvgElement.createSVGLength())")
         if (!animatedPropertyOfItem)
-            return;
+            return true;
 
         // newItem belongs to a SVGElement, but its associated SVGAnimatedProperty is not an animated list tear off.
         // (for example: "textElement.x.baseVal.appendItem(rectElement.width.baseVal)")
@@ -167,25 +173,34 @@ protected:
             // that's inserted into SVGTextElements SVGAnimatedLengthList 'x'. textElement.x.baseVal.getItem(0).value += 150 would
             // mutate the rectElement width _and_ the textElement x list. That's obviously wrong, take care of that.
             newItem = ListItemTearOff::create(newItem->propertyReference());
-            return;
+            return true;
         }
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // 'newItem' is already living in another list. If it's not our list, synchronize the other lists wrappers after the removal.
         bool livesInOtherList = animatedPropertyOfItem != m_animatedProperty;
-        int removedIndex = static_cast<AnimatedListPropertyTearOff*>(animatedPropertyOfItem)->removeItemFromList(newItem.get(), livesInOtherList);
-        ASSERT(removedIndex != -1);
+        AnimatedListPropertyTearOff* propertyTearOff = static_cast<AnimatedListPropertyTearOff*>(animatedPropertyOfItem);
+        int indexToRemove = propertyTearOff->findItem(newItem.get());
+        ASSERT(indexToRemove != -1);
+
+        // Do not remove newItem if already in this list at the target index.
+        if (!livesInOtherList && indexToModify && static_cast<unsigned>(indexToRemove) == *indexToModify)
+            return false;
+
+        propertyTearOff->removeItemFromList(indexToRemove, livesInOtherList);
 
         if (!indexToModify)
-            return;
+            return true;
 
         // If the item lived in our list, adjust the insertion index.
         if (!livesInOtherList) {
             unsigned& index = *indexToModify;
             // Spec: If the item is already in this list, note that the index of the item to (replace|insert before) is before the removal of the item.
-            if (static_cast<unsigned>(removedIndex) < index)
+            if (static_cast<unsigned>(indexToRemove) < index)
                 --index;
         }
+
+        return true;
     }
 
     // Back pointer to the animated property that created us
diff --git a/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp b/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp
index 2021301d6..ccef71306 100644
--- a/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp
+++ b/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp
@@ -70,7 +70,7 @@ SVGPathElement* SVGPathSegListPropertyTearOff::contextElement() const
     return static_cast<SVGPathElement*>(contextElement);
 }
 
-void SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify)
+bool SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify)
 {
     SVGPathSegWithContext* newItemWithContext = static_cast<SVGPathSegWithContext*>(newItem.get());
     SVGAnimatedProperty* animatedPropertyOfItem = newItemWithContext->animatedProperty();
@@ -79,29 +79,38 @@ void SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemT
     newItemWithContext->setContextAndRole(contextElement(), m_pathSegRole);
 
     if (!animatedPropertyOfItem)
-        return;
+        return true;
 
     // newItem belongs to a SVGPathElement, but its associated SVGAnimatedProperty is not an animated list tear off.
     // (for example: "pathElement.pathSegList.appendItem(pathElement.createSVGPathSegClosepath())")
     if (!animatedPropertyOfItem->isAnimatedListTearOff())
-        return;
+        return true;
 
     // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
     // 'newItem' is already living in another list. If it's not our list, synchronize the other lists wrappers after the removal.
     bool livesInOtherList = animatedPropertyOfItem != m_animatedProperty;
-    int removedIndex = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(animatedPropertyOfItem)->removeItemFromList(newItem.get(), livesInOtherList);
-    ASSERT(removedIndex != -1);
+    SVGAnimatedPathSegListPropertyTearOff* propertyTearOff = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(animatedPropertyOfItem);
+    int indexToRemove = propertyTearOff->findItem(newItem.get());
+    ASSERT(indexToRemove != -1);
+
+    // Do not remove newItem if already in this list at the target index.
+    if (!livesInOtherList && indexToModify && static_cast<unsigned>(indexToRemove) == *indexToModify)
+        return false;
+
+    propertyTearOff->removeItemFromList(indexToRemove, livesInOtherList);
 
     if (!indexToModify)
-        return;
+        return true;
 
     // If the item lived in our list, adjust the insertion index.
     if (!livesInOtherList) {
         unsigned& index = *indexToModify;
         // Spec: If the item is already in this list, note that the index of the item to (replace|insert before) is before the removal of the item.
-        if (static_cast<unsigned>(removedIndex) < index)
+        if (static_cast<unsigned>(indexToRemove) < index)
             --index;
     }
+
+    return true;
 }
 
 }
diff --git a/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.h
index bac9ee76d..6c22a9022 100644
--- a/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.h
@@ -41,24 +41,28 @@ public:
         return adoptRef(new SVGPathSegListPropertyTearOff(animatedProperty, role, pathSegRole, values, wrappers));
     }
 
-    int removeItemFromList(const ListItemType& removeItem, bool shouldSynchronizeWrappers)
+    int findItem(const ListItemType& item) const
     {
         ASSERT(m_values);
+
         unsigned size = m_values->size();
-        for (unsigned i = 0; i < size; ++i) {
-            ListItemType& item = m_values->at(i);
-            if (item != removeItem)
-                continue;
+        for (size_t i = 0; i < size; ++i) {
+            if (item == m_values->at(i))
+                return i;
+        }
 
-            m_values->remove(i);
+        return -1;
+    }
 
-            if (shouldSynchronizeWrappers)
-                commitChange();
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        ASSERT(m_values);
+        ASSERT(itemIndex < m_values->size());
 
-            return i;
-        }
+        m_values->remove(itemIndex);
 
-        return -1;
+        if (shouldSynchronizeWrappers)
+            commitChange();
     }
 
     // SVGList API
@@ -149,10 +153,11 @@ private:
         m_values->commitChange(m_animatedProperty->contextElement(), listModification);
     }
 
-    virtual void processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify);
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) OVERRIDE;
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
 private:
diff --git a/Source/WebCore/svg/properties/SVGStaticListPropertyTearOff.h b/Source/WebCore/svg/properties/SVGStaticListPropertyTearOff.h
index b39aaf3c1..4e6f5fd9f 100644
--- a/Source/WebCore/svg/properties/SVGStaticListPropertyTearOff.h
+++ b/Source/WebCore/svg/properties/SVGStaticListPropertyTearOff.h
@@ -96,14 +96,16 @@ private:
         m_values->commitChange(m_contextElement.get());
     }
 
-    virtual void processIncomingListItemValue(const ListItemType&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType&, unsigned*)
     {
         // no-op for static lists
+        return true;
     }
 
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
 private: