Remove ScLookupCache from ScLookupCacheMap it had been added to

...instead of removing an arbitrary ScLookupCache with a matching ScRange from
the first ScLookupCacheMap that happens to contain one.

79449d73900d7a9bf061244d76f5f8eecc441198 "make VLOOKUP in Calc thread-safe"
introduced per-thread ScLookupCacheMaps, so that multiple ScLookupCacheMaps can
contain ScLookupCaches with identical ScRanges.  For example, UITest_calc_tests6
adds ScLookupCaches for ScRange 1!R2C18:R97C18 to different threads'
ScLookupCacheMaps.  That causes confusion so that calling
ScDocument::RemoveLookupCacheHelper to remove an ScLookupCache from a
mismatching ScLookupCacheMap accesses a different

ScLookupCache* pCache = (*it).second.release();

that may already have been destroyed; see failing ASan/UBSan builds like
<https://ci.libreoffice.org//job/lo_ubsan/1067/>.

Change-Id: I70c33b236dc502b8a98e0e313d422424eec5dbca
Reviewed-on: https://gerrit.libreoffice.org/62194
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

4 files changed, 15 insertions, 24 deletions

diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index bf8790c6537d..6fe442954901 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -2495,8 +2495,6 @@ private:
 
     void EndListeningGroups( const std::vector<ScAddress>& rPosArray );
     void SetNeedsListeningGroups( const std::vector<ScAddress>& rPosArray );
-
-    bool RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache );
 };
 
 typedef std::unique_ptr<ScDocument, o3tl::default_delete<ScDocument>> ScDocumentUniquePtr;
diff --git a/sc/inc/lookupcache.hxx b/sc/inc/lookupcache.hxx
index cca7a52ad38c..3a2be452a2ee 100644
--- a/sc/inc/lookupcache.hxx
+++ b/sc/inc/lookupcache.hxx
@@ -27,6 +27,7 @@
 #include <unordered_map>
 
 class ScDocument;
+struct ScLookupCacheMap;
 struct ScQueryEntry;
 
 /** Lookup cache for one range used with interpreter functions such as VLOOKUP
@@ -106,7 +107,7 @@ public:
     };
 
     /// MUST be new'd because Notify() deletes.
-                            ScLookupCache( ScDocument * pDoc, const ScRange & rRange );
+                            ScLookupCache( ScDocument * pDoc, const ScRange & rRange, ScLookupCacheMap & cacheMap );
     virtual                 ~ScLookupCache() override;
     /// Remove from document structure and delete (!) cache on modify hint.
     virtual void Notify( const SfxHint& rHint ) override;
@@ -129,6 +130,8 @@ public:
 
     const ScRange&  getRange() const { return maRange; }
 
+    ScLookupCacheMap & getCacheMap() const { return mCacheMap; }
+
     struct Hash
     {
         size_t operator()( const ScRange & rRange ) const
@@ -184,6 +187,7 @@ private:
     std::unordered_map< QueryKey, QueryCriteriaAndResult, QueryKey::Hash > maQueryMap;
     ScRange const   maRange;
     ScDocument *    mpDoc;
+    ScLookupCacheMap & mCacheMap;
 
     ScLookupCache( const ScLookupCache & ) = delete;
     ScLookupCache & operator=( const ScLookupCache & ) = delete;
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index f2c317026f8e..32b3f15b6a35 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -1151,7 +1151,7 @@ ScLookupCache & ScDocument::GetLookupCache( const ScRange & rRange, ScInterprete
     if (findIt == rpCacheMap->aCacheMap.end())
     {
         auto insertIt = rpCacheMap->aCacheMap.emplace_hint(findIt,
-                    rRange, o3tl::make_unique<ScLookupCache>(this, rRange) );
+                    rRange, o3tl::make_unique<ScLookupCache>(this, rRange, *rpCacheMap) );
         pCache = insertIt->second.get();
         // The StartListeningArea() call is not thread-safe, as all threads
         // would access the same SvtBroadcaster.
@@ -1170,29 +1170,17 @@ void ScDocument::RemoveLookupCache( ScLookupCache & rCache )
     // a result of user input or recalc). If it turns out this can be the case, locking is needed
     // here and also in ScLookupCache::Notify().
     assert(!IsThreadedGroupCalcInProgress());
-    if( RemoveLookupCacheHelper( GetNonThreadedContext().mScLookupCache, rCache ))
-        return;
-    // The cache may be possibly in the caches stored for other threads.
-    for( ScLookupCacheMap* cacheMap : mThreadStoredScLookupCaches )
-        if( RemoveLookupCacheHelper( cacheMap, rCache ))
-            return;
-    OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
-}
-
-bool ScDocument::RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache )
-{
-    if( cacheMap == nullptr )
-        return false;
-    auto it(cacheMap->aCacheMap.find(rCache.getRange()));
-    if (it != cacheMap->aCacheMap.end())
+    auto & cacheMap = rCache.getCacheMap();
+    auto it(cacheMap.aCacheMap.find(rCache.getRange()));
+    if (it != cacheMap.aCacheMap.end())
     {
         ScLookupCache* pCache = (*it).second.release();
-        cacheMap->aCacheMap.erase(it);
+        cacheMap.aCacheMap.erase(it);
         assert(!IsThreadedGroupCalcInProgress()); // EndListeningArea() is not thread-safe
         EndListeningArea(pCache->getRange(), false, &rCache);
-        return true;
+        return;
     }
-    return false;
+    OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
 }
 
 void ScDocument::ClearLookupCaches()
diff --git a/sc/source/core/tool/lookupcache.cxx b/sc/source/core/tool/lookupcache.cxx
index fa6fbc0a1693..9522b46678fe 100644
--- a/sc/source/core/tool/lookupcache.cxx
+++ b/sc/source/core/tool/lookupcache.cxx
@@ -68,9 +68,10 @@ ScLookupCache::QueryCriteria::~QueryCriteria()
     deleteString();
 }
 
-ScLookupCache::ScLookupCache( ScDocument * pDoc, const ScRange & rRange ) :
+ScLookupCache::ScLookupCache( ScDocument * pDoc, const ScRange & rRange, ScLookupCacheMap & cacheMap ) :
     maRange( rRange),
-    mpDoc( pDoc)
+    mpDoc( pDoc),
+    mCacheMap(cacheMap)
 {
 }