From fbf047ba07d45ab46191973d6afc9475c111ff90 Mon Sep 17 00:00:00 2001
From: scott snyder <snyder@bnl.gov>
Date: Mon, 19 Mar 2018 15:59:22 +0100
Subject: [PATCH] SGTools: Fix potential dangling proxy pointer.
If a proxy was registered using only addToStore such that the first call
was for a CLID different than proxy->clID(), then the primary_sgkey
recorded is based on the CLID used for this first record, not
proxy->clID(). [This ordering used to happen when ProxyProviderSvc
initialized a proxy.] This in turn would confuse removeProxy when a proxy
was to be actually deleted, causing a dangling reference to the deleted
proxy to remain in m_keyMap. This could then lead to a crash in overwrite().
This was observed in a derivation workflow. Fix this.
Former-commit-id: b7782968d9509c22152345c0b8357b642507fa4d
---
Control/SGTools/src/DataStore.cxx | 5 +++--
Control/SGTools/test/DataStore_test.cxx | 17 +++++++++++++++++
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Control/SGTools/src/DataStore.cxx b/Control/SGTools/src/DataStore.cxx
index 1201c297c34..d7e596184fb 100755
--- a/Control/SGTools/src/DataStore.cxx
+++ b/Control/SGTools/src/DataStore.cxx
@@ -218,8 +218,9 @@ DataStore::removeProxy(DataProxy* proxy, bool forceRemove, bool hard)
// Remove all symlinks too.
for (CLID symclid : clids)
{
- if (symclid == clid) continue;
- m_keyMap.erase (m_pool.stringToKey (name, symclid));
+ sgkey_t sgkey = m_pool.stringToKey (name, symclid);
+ m_keyMap.erase (sgkey);
+ if (clid == symclid) continue;
storeIter = m_storeMap.find(symclid);
if (storeIter != m_storeMap.end()) {
SG::ProxyIterator it = storeIter->second.find (name);
diff --git a/Control/SGTools/test/DataStore_test.cxx b/Control/SGTools/test/DataStore_test.cxx
index 16bca70d27d..2c5b4ef122c 100644
--- a/Control/SGTools/test/DataStore_test.cxx
+++ b/Control/SGTools/test/DataStore_test.cxx
@@ -386,7 +386,24 @@ void test_removeProxy ATLAS_NOT_THREAD_SAFE ()
assert (store.proxy (123, "dp1") == 0);
assert (store.proxy (124, "dp1") == 0);
assert (store.proxy (123, "dp1x") == 0);
+ dp1->release();
+ //==============================================
+
+ // Test recording with a secondary CLID first.
+ SG::DataProxy* dp2 = make_proxy (223, "dp2");
+ dp2->resetOnly (false);
+ dp2->addRef();
+ dp2->setTransientID (223);
+ dp2->setTransientID (224);
+ assert (store.addToStore (224, dp2).isSuccess());
+ assert (store.addToStore (223, dp2).isSuccess());
+ assert (dp2->refCount() == 3);
+ assert (store.removeProxy (dp2, false, false).isSuccess());
+ assert (dp2->refCount() == 1);
+ assert (store.proxy_exact (pool.stringToKey ("dp2", 223)) == 0);
+ assert (store.proxy_exact (pool.stringToKey ("dp2", 224)) == 0);
+ dp2->release();
}
--
GitLab