From 51d63ab2858818be7707c884a284f3fe6210085d Mon Sep 17 00:00:00 2001 From: Urs Fleisch Date: Fri, 1 Dec 2023 12:54:42 +0100 Subject: [PATCH] Detach list when setAutoDelete() is called When calling setAutoDelete() on the implicitly shared copy of a list, also auto-deletion of the original container was modified because it was not detached. On the other hand, detach() was called by some methods getting an Iterator parameter, which can lead to modification of other implicitly shared copies but not the object is was called on. This happens when the method is called on a not-already-detached container, which is normally not the case because the container is detached when the iterator is taken (e.g. calling begin()). In such methods detach() cannot be called, and the client must make sure that the iterator is taken after making an implicit copy. This will NOT work: List l1 = { 1 }; auto it = l1.begin(); List l2 = l1; l1.erase(it); This will modify both l1 and l2. The second and the third lines must be swapped so that l1.begin() will detach l1 from l2. --- taglib/toolkit/tlist.h | 15 ++++++++++++++- taglib/toolkit/tlist.tcc | 8 +++++++- taglib/toolkit/tmap.h | 12 ++++++++---- taglib/toolkit/tmap.tcc | 1 - tests/test_list.cpp | 39 +++++++++++++++++++++++++++++---------- 5 files changed, 58 insertions(+), 17 deletions(-) diff --git a/taglib/toolkit/tlist.h b/taglib/toolkit/tlist.h index 6171ffdb..406204f4 100644 --- a/taglib/toolkit/tlist.h +++ b/taglib/toolkit/tlist.h @@ -121,6 +121,10 @@ namespace TagLib { /*! * Inserts a copy of \a item before \a it. + * + * \note This method cannot detach because \a it is tied to the internal + * list. Do not make an implicitly shared copy of this list between + * getting the iterator and calling this method! */ Iterator insert(Iterator it, const T &item); @@ -199,6 +203,10 @@ namespace TagLib { /*! * Erase the item at \a it from the list. + * + * \note This method cannot detach because \a it is tied to the internal + * list. Do not make an implicitly shared copy of this list between + * getting the iterator and calling this method! */ Iterator erase(Iterator it); @@ -232,6 +240,11 @@ namespace TagLib { */ void setAutoDelete(bool autoDelete); + /*! + * Returns true is auto-deletion is enabled. + */ + bool autoDelete() const; + /*! * Returns a reference to item \a i in the list. * @@ -293,7 +306,7 @@ namespace TagLib { /* * If this List is being shared via implicit sharing, do a deep copy of the * data and separate from the shared members. This should be called by all - * non-const subclass members. + * non-const subclass members without Iterator parameters. */ void detach(); diff --git a/taglib/toolkit/tlist.tcc b/taglib/toolkit/tlist.tcc index 97da8267..535fb2e7 100644 --- a/taglib/toolkit/tlist.tcc +++ b/taglib/toolkit/tlist.tcc @@ -148,7 +148,6 @@ typename List::ConstIterator List::cend() const template typename List::Iterator List::insert(Iterator it, const T &item) { - detach(); return d->list.insert(it, item); } @@ -270,9 +269,16 @@ const T &List::back() const template void List::setAutoDelete(bool autoDelete) { + detach(); d->autoDelete = autoDelete; } +template +bool List::autoDelete() const +{ + return d->autoDelete; +} + template T &List::back() { diff --git a/taglib/toolkit/tmap.h b/taglib/toolkit/tmap.h index eb515d4a..709235eb 100644 --- a/taglib/toolkit/tmap.h +++ b/taglib/toolkit/tmap.h @@ -164,12 +164,16 @@ namespace TagLib { bool contains(const Key &key) const; /*! - * Erase the item at \a it from the list. + * Erase the item at \a it from the map. + * + * \note This method cannot detach because \a it is tied to the internal + * map. Do not make an implicitly shared copy of this map between + * getting the iterator and calling this method! */ Map &erase(Iterator it); /*! - * Erase the item with \a key from the list. + * Erase the item with \a key from the map. */ Map &erase(const Key &key); @@ -225,9 +229,9 @@ namespace TagLib { protected: /* - * If this List is being shared via implicit sharing, do a deep copy of the + * If this Map is being shared via implicit sharing, do a deep copy of the * data and separate from the shared members. This should be called by all - * non-const subclass members. + * non-const subclass members without Iterator parameters. */ void detach(); diff --git a/taglib/toolkit/tmap.tcc b/taglib/toolkit/tmap.tcc index 067491db..a040b6ce 100644 --- a/taglib/toolkit/tmap.tcc +++ b/taglib/toolkit/tmap.tcc @@ -148,7 +148,6 @@ bool Map::contains(const Key &key) const template Map &Map::erase(Iterator it) { - detach(); d->map.erase(it); return *this; } diff --git a/tests/test_list.cpp b/tests/test_list.cpp index 6d954e97..776e4bf7 100644 --- a/tests/test_list.cpp +++ b/tests/test_list.cpp @@ -60,17 +60,36 @@ public: void testDetach() { - List l1; - l1.append(1); - l1.append(2); - l1.append(3); - l1.append(4); + { + List l1; + l1.append(1); + l1.append(2); + l1.append(3); + l1.append(4); - List l2 = l1; - auto it = l2.find(3); - *it = 33; - CPPUNIT_ASSERT_EQUAL(3, l1[2]); - CPPUNIT_ASSERT_EQUAL(33, l2[2]); + List l2 = l1; + auto it = l2.find(3); + *it = 33; + CPPUNIT_ASSERT_EQUAL(3, l1[2]); + CPPUNIT_ASSERT_EQUAL(33, l2[2]); + } + { + List l1; + List l2 = l1; + CPPUNIT_ASSERT(!l1.autoDelete()); + CPPUNIT_ASSERT(!l2.autoDelete()); + l2.setAutoDelete(true); + CPPUNIT_ASSERT(!l1.autoDelete()); + CPPUNIT_ASSERT(l2.autoDelete()); + } + { + List l1; + List l2 = l1; + l1.insert(l1.begin(), 1); + CPPUNIT_ASSERT(!l1.isEmpty()); + CPPUNIT_ASSERT_EQUAL(1, l1.front()); + CPPUNIT_ASSERT(l2.isEmpty()); + } } void bracedInit()