fix ASAN in SharedStringPool

regression from
commit 3581f1d71ae0d431ba28c0f3b7b263ff6212ce7b
optimize SharedStringPool::purge() and fix tests

which results in us potentially de-referencing an already de-allocated
OUString object in the first loop in purge().

So switch to a different strategy, which only needs one data structure,
instead of two.

Change-Id: Iaac6beda48459643afdb7b14ce7d39d68a93339c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/95226
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 58 insertions, 41 deletions

diff --git a/svl/qa/unit/svl.cxx b/svl/qa/unit/svl.cxx
index 194dc550c278..c32017f7aea6 100644
--- a/svl/qa/unit/svl.cxx
+++ b/svl/qa/unit/svl.cxx
@@ -60,6 +60,7 @@ public:
     void testSharedString();
     void testSharedStringPool();
     void testSharedStringPoolPurge();
+    void testSharedStringPoolPurgeBug1();
     void testFdo60915();
     void testI116701();
     void testTdf103060();
@@ -77,6 +78,7 @@ public:
     CPPUNIT_TEST(testSharedString);
     CPPUNIT_TEST(testSharedStringPool);
     CPPUNIT_TEST(testSharedStringPoolPurge);
+    CPPUNIT_TEST(testSharedStringPoolPurgeBug1);
     CPPUNIT_TEST(testFdo60915);
     CPPUNIT_TEST(testI116701);
     CPPUNIT_TEST(testTdf103060);
@@ -376,30 +378,30 @@ void Test::testSharedStringPoolPurge()
     std::optional<svl::SharedString> pStr3 = aPool.intern("ANDY");
     std::optional<svl::SharedString> pStr4 = aPool.intern("Bruce");
 
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(5), aPool.getCount());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase());
 
     // This shouldn't purge anything.
     aPool.purge();
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(5), aPool.getCount());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase());
 
     // Delete one heap string object, and purge. That should purge one string.
     pStr1.reset();
     aPool.purge();
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(3), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase());
 
     // Nothing changes, because the upper-string is still in the map
     pStr3.reset();
     aPool.purge();
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(3), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), aPool.getCount());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCountIgnoreCase());
 
     // Again.
     pStr2.reset();
     aPool.purge();
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), aPool.getCount());
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), aPool.getCountIgnoreCase());
 
     // Delete 'Bruce' and purge.
@@ -409,6 +411,19 @@ void Test::testSharedStringPoolPurge()
     CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCountIgnoreCase());
 }
 
+void Test::testSharedStringPoolPurgeBug1()
+{
+    // We had a bug where, if we had two strings that mapped to the same uppercase string,
+    // purge() would de-reference a dangling pointer and consequently cause an ASAN failure.
+    SvtSysLocale aSysLocale;
+    svl::SharedStringPool aPool(*aSysLocale.GetCharClassPtr());
+    aPool.intern("Andy");
+    aPool.intern("andy");
+    aPool.purge();
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCount());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), aPool.getCountIgnoreCase());
+}
+
 void Test::checkPreviewString(SvNumberFormatter& aFormatter,
                               const OUString& sCode,
                               double fPreviewNumber,
diff --git a/svl/source/misc/sharedstringpool.cxx b/svl/source/misc/sharedstringpool.cxx
index d2d890004fbd..e4bc873e5f69 100644
--- a/svl/source/misc/sharedstringpool.cxx
+++ b/svl/source/misc/sharedstringpool.cxx
@@ -29,10 +29,10 @@ sal_Int32 getRefCount( const rtl_uString* p )
 struct SharedStringPool::Impl
 {
     mutable osl::Mutex maMutex;
-    // set of upper-case, so we can share these as the value in the maStrMap
-    std::unordered_set<OUString> maStrPoolUpper;
-    // map with rtl_uString* as value so we can avoid some ref-counting
-    std::unordered_map<OUString,rtl_uString*> maStrMap;
+    // We use this map for two purposes - to store lower->upper case mappings
+    // and to retrieve a shared uppercase object, so the management logic
+    // is quite complex.
+    std::unordered_map<OUString,OUString> maStrMap;
     const CharClass& mrCharClass;
 
     explicit Impl( const CharClass& rCharClass ) : mrCharClass(rCharClass) {}
@@ -49,31 +49,34 @@ SharedString SharedStringPool::intern( const OUString& rStr )
 {
     osl::MutexGuard aGuard(&mpImpl->maMutex);
 
-    auto [mapIt,bInserted] = mpImpl->maStrMap.emplace(rStr, rStr.pData);
-    if (bInserted)
+    auto [mapIt,bInserted] = mpImpl->maStrMap.emplace(rStr, rStr);
+    if (!bInserted)
+        // there is already a mapping
+        return SharedString(mapIt->first.pData, mapIt->second.pData);
+
+    // This is a new string insertion. Establish mapping to upper-case variant.
+    OUString aUpper = mpImpl->mrCharClass.uppercase(rStr);
+    if (aUpper == rStr)
+        // no need to do anything more, because we inserted an upper->upper mapping
+        return SharedString(mapIt->first.pData, mapIt->second.pData);
+
+    // We need to insert a lower->upper mapping, so also insert
+    // an upper->upper mapping, which we can use both for when an upper string
+    // is interned, and to look up a shared upper string.
+    auto mapIt2 = mpImpl->maStrMap.find(aUpper);
+    if (mapIt2 != mpImpl->maStrMap.end())
     {
-        // This is a new string insertion. Establish mapping to upper-case variant.
-        OUString aUpper = mpImpl->mrCharClass.uppercase(rStr);
-        if (aUpper == rStr)
-        {
-            auto insertResult = mpImpl->maStrPoolUpper.insert(rStr);
-            // need to use the same underlying rtl_uString object so the
-            // upper->upper detection in purge() works
-            auto pData = insertResult.first->pData;
-            // This is dodgy, but necessary. I don't want to do a delete/insert because
-            // this class is very performance sensitive. This does not violate the internals
-            // the map because the new key points to something with the same hash and equality
-            // as the old key.
-            const_cast<OUString&>(mapIt->first) = *insertResult.first;
-            mapIt->second = pData;
-        }
-        else
-        {
-            auto insertResult = mpImpl->maStrPoolUpper.insert(aUpper);
-            mapIt->second = insertResult.first->pData;
-        }
+        // there is an already existing upper string
+        mapIt->second = mapIt2->first;
+        return SharedString(mapIt->first.pData, mapIt->second.pData);
     }
-    return SharedString(mapIt->first.pData, mapIt->second);
+
+    // There is no already existing upper string.
+    // First, update using the iterator, can't do this later because
+    // the iterator will be invalid.
+    mapIt->second = aUpper;
+    mpImpl->maStrMap.emplace_hint(mapIt2, aUpper, aUpper);
+    return SharedString(rStr.pData, aUpper.pData);
 }
 
 void SharedStringPool::purge()
@@ -91,7 +94,7 @@ void SharedStringPool::purge()
     while (it != itEnd)
     {
         rtl_uString* p1 = it->first.pData;
-        rtl_uString* p2 = it->second;
+        rtl_uString* p2 = it->second.pData;
         if (p1 != p2)
         {
             // normal case - lowercase mapped to uppercase, which
@@ -100,10 +103,6 @@ void SharedStringPool::purge()
             if (getRefCount(p1) == 1)
             {
                 it = mpImpl->maStrMap.erase(it);
-                // except that the uppercase entry may be mapped to
-                // by other lower-case entries
-                if (getRefCount(p2) == 1)
-                    mpImpl->maStrPoolUpper.erase(OUString::unacquired(&p2));
                 continue;
             }
         }
@@ -115,16 +114,15 @@ void SharedStringPool::purge()
     while (it != itEnd)
     {
         rtl_uString* p1 = it->first.pData;
-        rtl_uString* p2 = it->second;
+        rtl_uString* p2 = it->second.pData;
         if (p1 == p2)
         {
             // uppercase which is mapped to itself, which means
             // one ref-counted entry as the key in the map, and
-            // one ref-counted entry in the set
+            // one ref-counted entry in the value in the map
             if (getRefCount(p1) == 2)
             {
                 it = mpImpl->maStrMap.erase(it);
-                mpImpl->maStrPoolUpper.erase(OUString::unacquired(&p1));
                 continue;
             }
         }
@@ -141,7 +139,11 @@ size_t SharedStringPool::getCount() const
 size_t SharedStringPool::getCountIgnoreCase() const
 {
     osl::MutexGuard aGuard(&mpImpl->maMutex);
-    return mpImpl->maStrPoolUpper.size();
+    // this is only called from unit tests, so no need to be efficient
+    std::unordered_set<OUString> aUpperSet;
+    for (auto const & pair : mpImpl->maStrMap)
+        aUpperSet.insert(pair.second);
+    return aUpperSet.size();
 }
 
 }