From 03a271901c39d60e4519e67e258d565ad5e1e085 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Tue, 23 Feb 2016 16:39:48 +0100 Subject: [PATCH] Guard against globally shared UNO ref accessed from wrong UNO env connectivity/source/drivers/jdbc/jdbc.component has environment="@CPPU_ENV@:affine" to place the com.sun.star.comp.sdbc.JDBCDriver implementation into an affine UNOenvironment. The com.sun.star.sdbcx.comp.hsqldb.Driver implementation (in the normal C++ UNO environment), in ODriverDelegator::connect (connectivity/source/drivers/hsqldb/HDriver.cxx), calls StorageContainer::registerStorage to store an XStorage in a global map, then calls the JDBCDriver (i.e., thread enters the affine environment), which calls via (non-UNO) JNI into hsqldb.jar code, which in turn calls via (non-UNO) JNI into Java_com_sun_star_sdbcx_comp_hsqldb_StorageFileAccess_isStreamElement (connectivity/source/drivers/hsqldb/StorageFileAccess.cxx), which uses StorageContainer::getRegisteredStorage to obtain the XStorage and use it. But that XStorage is the original C++ object, not a proxy that witnesses the mapping between the normal C++ and the affine UNO environment. (And the thread is still in the affine environment, after having passed through the Java stack frames via non-UNO JNI.) That does not necessarily cause any problems immediately (so apparently went unnoticed for quite a while), but when the XStorage-implementation in turn wants to obtain the SimpleLogRing singleton, it would now trigger the std::abort();//TODO in cppuhelper/source/servicemanager.cxx (where the invocation-by-constructor case hasn't yet been implemented for differing environments), when that singleton is changed to use the constructor feature in "tdf#74608: Constructor function for SimpleLogRing singleton." So just do any necessary mapping every time an XStorage stored in the static StorageContainer is accessed. Change-Id: I91a62fd7e1cec29026f70a2c3acdfe051885c0fa --- .../source/drivers/hsqldb/HStorageMap.cxx | 44 ++++++++++++++----- .../drivers/hsqldb/StorageFileAccess.cxx | 19 ++++---- .../source/inc/hsqldb/HStorageMap.hxx | 6 +++ 3 files changed, 51 insertions(+), 18 deletions(-) diff --git a/connectivity/source/drivers/hsqldb/HStorageMap.cxx b/connectivity/source/drivers/hsqldb/HStorageMap.cxx index afd97ed77eae..334ff2b23970 100644 --- a/connectivity/source/drivers/hsqldb/HStorageMap.cxx +++ b/connectivity/source/drivers/hsqldb/HStorageMap.cxx @@ -25,6 +25,7 @@ #include #include #include +#include #include namespace connectivity @@ -99,6 +100,28 @@ namespace connectivity return m_xSeek; } + css::uno::Reference StorageData::mapStorage() + const + { + css::uno::Environment env(css::uno::Environment::getCurrent()); + if (!(env.is() && storageEnvironment.is())) { + throw css::uno::RuntimeException("cannot get environments"); + } + if (env.get() == storageEnvironment.get()) { + return storage; + } else { + css::uno::Mapping map(storageEnvironment, env); + if (!map.is()) { + throw css::uno::RuntimeException("cannot get mapping"); + } + css::uno::Reference mapped; + map.mapInterface( + reinterpret_cast(&mapped), storage.get(), + cppu::UnoType::get()); + return mapped; + } + } + TStorages& lcl_getStorageMap() { static TStorages s_aMap; @@ -170,12 +193,12 @@ namespace connectivity // check if the storage is already in our map TStorages::iterator aFind = ::std::find_if(rMap.begin(),rMap.end(), [&_xStorage] (const TStorages::value_type& storage) { - return storage.second.storage == _xStorage; + return storage.second.mapStorage() == _xStorage; }); if ( aFind == rMap.end() ) { - aFind = rMap.insert(TStorages::value_type(lcl_getNextCount(), {_xStorage, _sURL, TStreamMap()})).first; + aFind = rMap.insert(TStorages::value_type(lcl_getNextCount(), {_xStorage, css::uno::Environment::getCurrent(), _sURL, TStreamMap()})).first; } return aFind->first; @@ -201,7 +224,7 @@ namespace connectivity // check if the storage is already in our map TStorages::iterator aFind = ::std::find_if(rMap.begin(),rMap.end(), [&_xStorage] (const TStorages::value_type& storage) { - return storage.second.storage == _xStorage; + return storage.second.mapStorage() == _xStorage; }); if ( aFind != rMap.end() ) @@ -219,10 +242,10 @@ namespace connectivity { if ( _xListener.is() ) { - Reference xBroad(aFind->second.storage,UNO_QUERY); + Reference xBroad(aFind->second.mapStorage(),UNO_QUERY); if ( xBroad.is() ) xBroad->removeTransactionListener(_xListener); - Reference xTrans(aFind->second.storage,UNO_QUERY); + Reference xTrans(aFind->second.mapStorage(),UNO_QUERY); if ( xTrans.is() ) xTrans->commit(); } @@ -244,8 +267,9 @@ namespace connectivity if ( aFind != rMap.end() ) { TStorages::mapped_type aStoragePair = StorageContainer::getRegisteredStorage(sKey); - OSL_ENSURE(aStoragePair.storage.is(),"No Storage available!"); - if ( aStoragePair.storage.is() ) + auto storage = aStoragePair.mapStorage(); + OSL_ENSURE(storage.is(),"No Storage available!"); + if ( storage.is() ) { OUString sOrgName = StorageContainer::jstring2ustring(env,name); OUString sName = removeURLPrefix(sOrgName,aStoragePair.url); @@ -261,7 +285,7 @@ namespace connectivity { try { - pHelper.reset(new StreamHelper(aStoragePair.storage->openStreamElement(sName,_nMode))); + pHelper.reset(new StreamHelper(storage->openStreamElement(sName,_nMode))); } catch(const Exception&) { @@ -272,7 +296,7 @@ namespace connectivity bool bIsStream = true; try { - bIsStream = aStoragePair.storage->isStreamElement(sStrippedName); + bIsStream = storage->isStreamElement(sStrippedName); } catch(const Exception&) { @@ -281,7 +305,7 @@ namespace connectivity if ( !bIsStream ) return pHelper; // readonly file without data stream } - pHelper.reset( new StreamHelper(aStoragePair.storage->openStreamElement( sStrippedName, _nMode ) ) ); + pHelper.reset( new StreamHelper(storage->openStreamElement( sStrippedName, _nMode ) ) ); } aFind->second.streams.insert(TStreamMap::value_type(sName,pHelper)); } diff --git a/connectivity/source/drivers/hsqldb/StorageFileAccess.cxx b/connectivity/source/drivers/hsqldb/StorageFileAccess.cxx index 8937898a71bb..69b676e95b05 100644 --- a/connectivity/source/drivers/hsqldb/StorageFileAccess.cxx +++ b/connectivity/source/drivers/hsqldb/StorageFileAccess.cxx @@ -43,7 +43,8 @@ extern "C" SAL_JNI_EXPORT jboolean JNICALL Java_com_sun_star_sdbcx_comp_hsqldb_S (JNIEnv * env, jobject /*obj_this*/,jstring key, jstring name) { TStorages::mapped_type aStoragePair = StorageContainer::getRegisteredStorage(StorageContainer::jstring2ustring(env,key)); - if ( aStoragePair.storage.is() ) + auto storage = aStoragePair.mapStorage(); + if ( storage.is() ) { try { @@ -51,11 +52,11 @@ extern "C" SAL_JNI_EXPORT jboolean JNICALL Java_com_sun_star_sdbcx_comp_hsqldb_S try { OUString sOldName = StorageContainer::removeOldURLPrefix(sName); - if ( aStoragePair.storage->isStreamElement(sOldName) ) + if ( storage->isStreamElement(sOldName) ) { try { - aStoragePair.storage->renameElement(sOldName,StorageContainer::removeURLPrefix(sName,aStoragePair.url)); + storage->renameElement(sOldName,StorageContainer::removeURLPrefix(sName,aStoragePair.url)); } catch(const Exception&) { @@ -68,7 +69,7 @@ extern "C" SAL_JNI_EXPORT jboolean JNICALL Java_com_sun_star_sdbcx_comp_hsqldb_S catch(const IllegalArgumentException&) { } - return aStoragePair.storage->isStreamElement(StorageContainer::removeURLPrefix(sName,aStoragePair.url)); + return storage->isStreamElement(StorageContainer::removeURLPrefix(sName,aStoragePair.url)); } catch(const NoSuchElementException&) { @@ -101,11 +102,12 @@ extern "C" SAL_JNI_EXPORT void JNICALL Java_com_sun_star_sdbcx_comp_hsqldb_Stora } #endif TStorages::mapped_type aStoragePair = StorageContainer::getRegisteredStorage(StorageContainer::jstring2ustring(env,key)); - if ( aStoragePair.storage.is() ) + auto storage = aStoragePair.mapStorage(); + if ( storage.is() ) { try { - aStoragePair.storage->removeElement(StorageContainer::removeURLPrefix(StorageContainer::jstring2ustring(env,name),aStoragePair.url)); + storage->removeElement(StorageContainer::removeURLPrefix(StorageContainer::jstring2ustring(env,name),aStoragePair.url)); } catch(const NoSuchElementException&) { @@ -137,11 +139,12 @@ extern "C" SAL_JNI_EXPORT void JNICALL Java_com_sun_star_sdbcx_comp_hsqldb_Stora } #endif TStorages::mapped_type aStoragePair = StorageContainer::getRegisteredStorage(StorageContainer::jstring2ustring(env,key)); - if ( aStoragePair.storage.is() ) + auto storage = aStoragePair.mapStorage(); + if ( storage.is() ) { try { - aStoragePair.storage->renameElement( + storage->renameElement( StorageContainer::removeURLPrefix(StorageContainer::jstring2ustring(env,oldname),aStoragePair.url), StorageContainer::removeURLPrefix(StorageContainer::jstring2ustring(env,newname),aStoragePair.url) ); diff --git a/connectivity/source/inc/hsqldb/HStorageMap.hxx b/connectivity/source/inc/hsqldb/HStorageMap.hxx index 1150be49a580..d3bba8eda7b3 100644 --- a/connectivity/source/inc/hsqldb/HStorageMap.hxx +++ b/connectivity/source/inc/hsqldb/HStorageMap.hxx @@ -31,6 +31,7 @@ #include #include #include +#include namespace connectivity { @@ -54,11 +55,16 @@ namespace connectivity typedef std::map< OUString, std::shared_ptr > TStreamMap; + struct StorageData { css::uno::Reference storage; + css::uno::Environment storageEnvironment; OUString url; TStreamMap streams; + + css::uno::Reference mapStorage() const; }; + typedef std::map TStorages; /** contains all storages so far accessed. */