Skip to content

Commit

Permalink
Fix use-after-free issue when destroying a OGRSpatialReference object…
Browse files Browse the repository at this point in the history 
… in a thread...

when another thread has created it but has been destroy in-between

The root cause is that a PJ* object captures the PJ_CONTEXT that created it,
so when that PJ_CONTEXT is a TLS object, and that the thread disappears, the
PJ* object references a dead object. So before destroying it, make sure to
reassign the context of the current tread.

Was seen while investigating QGIS issue qgis/QGIS#30569
No guarantee that this fix fixes the issue folks hits in practice since
I can't reproduce it, but it's a fix anyway.

We got the following stack trace while opening a project where we were
asked to choose a datum transform and we let the QGIS OGR connection pool
time out.

```
==27326== Invalid read of size 4
==27326==    at 0xEA7996D: internal_pj_ctx_get_errno (ctx.cpp:195)
==27326==    by 0xEA916EC: internal_proj_errno (4D_api.cpp:1262)
==27326==    by 0xEA762D0: internal_pj_free (malloc.cpp:190)
==27326==    by 0xEA916C5: internal_proj_destroy (4D_api.cpp:1253)
==27326==    by 0xCA8B547: OGRSpatialReference::Private::~Private() (ogrspatialreference.cpp:163)
==27326==    by 0xCAA34A3: std::default_delete<OGRSpatialReference::Private>::operator()(OGRSpatialReference::Private*) const (unique_ptr.h:76)
==27326==    by 0xCAA334C: std::unique_ptr<OGRSpatialReference::Private, std::default_delete<OGRSpatialReference::Private> >::~unique_ptr() (unique_ptr.h:239)
==27326==    by 0xCA8CFAB: OGRSpatialReference::~OGRSpatialReference() (ogrspatialreference.cpp:721)
==27326==    by 0xCA8CFDB: OGRSpatialReference::~OGRSpatialReference() (ogrspatialreference.cpp:724)
==27326==    by 0xCA8D37E: OGRSpatialReference::Release() (ogrspatialreference.cpp:924)
==27326==    by 0xC5C089D: GDALGeoPackageDataset::~GDALGeoPackageDataset() (ogrgeopackagedatasource.cpp:842)
==27326==    by 0xC5C0997: GDALGeoPackageDataset::~GDALGeoPackageDataset() (ogrgeopackagedatasource.cpp:844)
==27326==    by 0xBFE637A: GDALClose (gdaldataset.cpp:3628)
==27326==    by 0x7CF94EA: QgsOgrProviderUtils::GDALCloseWrapper(void*) (qgsogrprovider.cpp:4171)
==27326==    by 0x78749AD: qgsConnectionPool_ConnectionDestroy(QgsOgrConn*) (qgsogrconnpool.h:50)
==27326==    by 0x78793EE: QgsConnectionPoolGroup<QgsOgrConn*>::onConnectionExpired() (qgsconnectionpool.h:227)
==27326==    by 0x78749EB: QgsOgrConnPoolGroup::handleConnectionExpired() (qgsogrconnpool.h:89)
==27326==    by 0x784F64E: QgsOgrConnPoolGroup::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_qgsogrconnpool.cpp:78)
==27326==    by 0x9E8E588: QMetaObject::activate(QObject*, int, int, void**) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9E9AE16: QTimer::timeout(QTimer::QPrivateSignal) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9E9B117: QTimer::timerEvent(QTimerEvent*) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9E8F582: QObject::event(QEvent*) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x8F7A3FB: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /opt/qt59/lib/libQt5Widgets.so.5.9.1)
==27326==    by 0x8F81E06: QApplication::notify(QObject*, QEvent*) (in /opt/qt59/lib/libQt5Widgets.so.5.9.1)
==27326==    by 0x7D4ED9A: QgsApplication::notify(QObject*, QEvent*) (qgsapplication.cpp:416)
==27326==    by 0x9E62107: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9EB666D: QTimerInfoList::activateTimers() (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9EB6F28: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x125CE196: g_main_context_dispatch (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==27326==    by 0x125CE3EF: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==27326==    by 0x125CE49B: g_main_context_iteration (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
==27326==    by 0x9EB729E: QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9E60139: QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x91A9AA6: QDialog::exec() (in /opt/qt59/lib/libQt5Widgets.so.5.9.1)
==27326==    by 0x6A40D77: QgsDatumTransformDialog::run(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QWidget*, QgsMapCanvas*, QString const&) (qgsdatumtransformdialog.cpp:53)
==27326==    by 0x51E3C10: QgisApp::askUserForDatumTransform(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QgsMapLayer const*) (qgisapp.cpp:14254)
==27326==    by 0x51C8DB3: QgisApp::projectCrsChanged() (qgisapp.cpp:10343)
==27326==    by 0x526E713: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (QgisApp::*)()>::call(void (QgisApp::*)(), QgisApp*, void**) (qobjectdefs_impl.h:136)
==27326==    by 0x5269A53: void QtPrivate::FunctionPointer<void (QgisApp::*)()>::call<QtPrivate::List<>, void>(void (QgisApp::*)(), QgisApp*, void**) (qobjectdefs_impl.h:169)
==27326==    by 0x525F44C: QtPrivate::QSlotObject<void (QgisApp::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobject_impl.h:120)
==27326==  Address 0x5c0c38b0 is 0 bytes inside a block of size 112 free'd
==27326==    at 0x4C2F440: operator delete(void*) (vg_replace_malloc.c:586)
==27326==    by 0xEA79944: internal_pj_ctx_free (ctx.cpp:183)
==27326==    by 0xEA9182F: internal_proj_context_destroy (4D_api.cpp:1359)
==27326==    by 0xCB12CD9: OSRPJContextHolder::deinit() (ogr_proj_p.cpp:96)
==27326==    by 0xCB12C99: OSRPJContextHolder::~OSRPJContextHolder() (ogr_proj_p.cpp:88)
==27326==    by 0xA75B5FE: __call_tls_dtors (cxa_thread_atexit_impl.c:155)
==27326==    by 0x10F956C7: start_thread (pthread_create.c:343)
==27326==    by 0xA82841C: clone (clone.S:109)
==27326==  Block was alloc'd at
==27326==    at 0x4C2E709: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:387)
==27326==    by 0xEA798DC: internal_pj_ctx_alloc (ctx.cpp:173)
==27326==    by 0xEA917F0: internal_proj_context_create (4D_api.cpp:1347)
==27326==    by 0xCB12C57: OSRPJContextHolder::init() (ogr_proj_p.cpp:80)
==27326==    by 0xCB12D20: OSRGetProjTLSContext() (ogr_proj_p.cpp:143)
==27326==    by 0xCAA3116: OGRSpatialReference::Private::getPROJContext() (ogrspatialreference.cpp:149)
==27326==    by 0xCAA0B79: OGRSpatialReference::importFromEPSGA(int) (ogrspatialreference.cpp:10260)
==27326==    by 0xCAA0DFB: OGRSpatialReference::importFromEPSG(int) (ogrspatialreference.cpp:10353)
==27326==    by 0xC5BEB2D: GDALGPKGImportFromEPSG(OGRSpatialReference*, int) (ogrgeopackagedatasource.cpp:255)
==27326==    by 0xC5BEE8E: GDALGeoPackageDataset::GetSpatialRef(int) (ogrgeopackagedatasource.cpp:312)
==27326==    by 0xC5DFB11: OGRGeoPackageTableLayer::ReadTableDefinition() (ogrgeopackagetablelayer.cpp:898)
==27326==    by 0xC5DE918: OGRGeoPackageTableLayer::GetLayerDefn() (ogrgeopackagetablelayer.cpp:562)
==27326==    by 0xC5E57E3: OGRGeoPackageTableLayer::TestCapability(char const*) (ogrgeopackagetablelayer.cpp:2614)
==27326==    by 0xC6C8A46: OGR_L_TestCapability (ogrlayer.cpp:1078)
==27326==    by 0x7CDD664: QgsOgrProviderUtils::setRelevantFields(void*, int, bool, QList<int> const&, bool, QString const&) (qgsogrprovider.cpp:1244)
==27326==    by 0x7D20456: QgsOgrFeatureIterator::QgsOgrFeatureIterator(QgsOgrFeatureSource*, bool, QgsFeatureRequest const&) (qgsogrfeatureiterator.cpp:162)
==27326==    by 0x7D22932: QgsOgrFeatureSource::getFeatures(QgsFeatureRequest const&) (qgsogrfeatureiterator.cpp:573)
==27326==    by 0x81E2BCF: QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator(QgsVectorLayerFeatureSource*, bool, QgsFeatureRequest const&) (qgsvectorlayerfeatureiterator.cpp:274)
==27326==    by 0x81E1C2C: QgsVectorLayerFeatureSource::getFeatures(QgsFeatureRequest const&) (qgsvectorlayerfeatureiterator.cpp:99)
==27326==    by 0x82056D9: QgsVectorLayerRenderer::render() (qgsvectorlayerrenderer.cpp:272)
==27326==    by 0x7F55B08: QgsMapRendererCustomPainterJob::doRender() (qgsmaprenderercustompainterjob.cpp:314)
==27326==    by 0x7F55601: QgsMapRendererCustomPainterJob::staticRender(QgsMapRendererCustomPainterJob*) (qgsmaprenderercustompainterjob.cpp:264)
==27326==    by 0x7F58D42: QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor() (qtconcurrentstoredfunctioncall.h:432)
==27326==    by 0x7F56690: QtConcurrent::RunFunctionTask<void>::run() (qtconcurrentrunbase.h:136)
==27326==    by 0x9C7E942: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x9C82658: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==27326==    by 0x10F956B9: start_thread (pthread_create.c:333)
==27326==    by 0xA82841C: clone (clone.S:109)
==27326==
```
  • Loading branch information
@rouault
rouault committed Nov 22, 2019
1 parent 8eb34ce commit bec5fbd
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 4 deletions.
15 changes: 15 additions & 0 deletions autotest/osr/osr_basic.py
View file
Expand Up @@ -1569,3 +1569,18 @@ def threaded_function(arg):

sr = osr.SpatialReference()
assert sr.ImportFromEPSG(32631) == 0


def test_osr_create_in_one_thread_destroy_in_other():
def threaded_function(arg):
sr = osr.SpatialReference()
sr.ImportFromEPSG(32631)
arg[0] = sr

arg = [ None ]

thread = Thread(target = threaded_function, args = (arg, ))
thread.start()
thread.join()
assert arg[0]
del arg[0]
33 changes: 29 additions & 4 deletions gdal/ogr/ogrspatialreference.cpp
View file
Expand Up @@ -159,18 +159,36 @@ OGRSpatialReference::Private::Private():

OGRSpatialReference::Private::~Private()
{
// In case we destroy the object not in the thread that created it,
// we need to reassign the PROJ context. Having the context bundled inside
// PJ* deeply sucks...
auto ctxt = getPROJContext();

proj_assign_context( m_pj_crs, ctxt );
proj_destroy(m_pj_crs);

proj_assign_context( m_pj_geod_base_crs_temp, ctxt );
proj_destroy(m_pj_geod_base_crs_temp);

proj_assign_context( m_pj_proj_crs_cs_temp, ctxt );
proj_destroy(m_pj_proj_crs_cs_temp);

proj_assign_context( m_pj_bound_crs_target, ctxt );
proj_destroy(m_pj_bound_crs_target);

proj_assign_context( m_pj_bound_crs_co, ctxt );
proj_destroy(m_pj_bound_crs_co);

proj_assign_context( m_pj_crs_backup, ctxt );
proj_destroy(m_pj_crs_backup);

delete m_poRootBackup;
delete m_poRoot;
}

void OGRSpatialReference::Private::clear()
{
proj_assign_context( m_pj_crs, getPROJContext() );
proj_destroy(m_pj_crs);
m_pj_crs = nullptr;

Expand Down Expand Up @@ -210,6 +228,7 @@ void OGRSpatialReference::Private::setRoot(OGR_SRSNode* poRoot)
void OGRSpatialReference::Private::setPjCRS(PJ* pj_crsIn,
bool doRefreshAxisMapping)
{
proj_assign_context( m_pj_crs, getPROJContext() );
proj_destroy(m_pj_crs);
m_pj_crs = pj_crsIn;
if( m_pj_crs )
Expand Down Expand Up @@ -516,18 +535,21 @@ PJ *OGRSpatialReference::Private::getGeodBaseCRS()
return m_pj_crs;
}

auto ctxt = getPROJContext();
if( m_pjType == PJ_TYPE_PROJECTED_CRS ) {
proj_assign_context(m_pj_geod_base_crs_temp, ctxt);
proj_destroy(m_pj_geod_base_crs_temp);
m_pj_geod_base_crs_temp = proj_crs_get_geodetic_crs(
getPROJContext(), m_pj_crs);
ctxt, m_pj_crs);
return m_pj_geod_base_crs_temp;
}

proj_assign_context(m_pj_geod_base_crs_temp, ctxt);
proj_destroy(m_pj_geod_base_crs_temp);
auto cs = proj_create_ellipsoidal_2D_cs(
getPROJContext(), PJ_ELLPS2D_LATITUDE_LONGITUDE, nullptr, 0);
ctxt, PJ_ELLPS2D_LATITUDE_LONGITUDE, nullptr, 0);
m_pj_geod_base_crs_temp = proj_create_geographic_crs(
getPROJContext(),
ctxt,
"WGS 84", "World Geodetic System 1984", "WGS 84", SRS_WGS84_SEMIMAJOR,
SRS_WGS84_INVFLATTENING, SRS_PM_GREENWICH, 0.0,
SRS_UA_DEGREE, CPLAtof(SRS_UA_DEGREE_CONV), cs);
Expand All @@ -538,16 +560,19 @@ PJ *OGRSpatialReference::Private::getGeodBaseCRS()

PJ *OGRSpatialReference::Private::getProjCRSCoordSys()
{
auto ctxt = getPROJContext();
if( m_pjType == PJ_TYPE_PROJECTED_CRS ) {
proj_assign_context(m_pj_proj_crs_cs_temp, ctxt);
proj_destroy(m_pj_proj_crs_cs_temp);
m_pj_proj_crs_cs_temp = proj_crs_get_coordinate_system(
getPROJContext(), m_pj_crs);
return m_pj_proj_crs_cs_temp;
}

proj_assign_context(m_pj_proj_crs_cs_temp, ctxt);
proj_destroy(m_pj_proj_crs_cs_temp);
m_pj_proj_crs_cs_temp = proj_create_cartesian_2D_cs(
getPROJContext(), PJ_CART2D_EASTING_NORTHING, nullptr, 0);
ctxt, PJ_CART2D_EASTING_NORTHING, nullptr, 0);
return m_pj_proj_crs_cs_temp;
}

Expand Down

0 comments on commit bec5fbd

Please sign in to comment.