Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash in QgsCoordinateTransformPrivate at application exit (fixes #31762) #31848

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Sep 17, 2019

This fixes the following Valgrind reported error:

==1703== Invalid read of size 4
==1703==    at 0xE614195: internal_pj_ctx_get_errno (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE6116AE: internal_pj_free (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE624E78: internal_proj_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7D3EFE7: QgsCoordinateTransformPrivate::freeProj() (qgscoordinatetransform_p.cpp:659)
==1703==    by 0x7D3CD23: QgsCoordinateTransformPrivate::~QgsCoordinateTransformPrivate() (qgscoordinatetransform_p.cpp:120)
==1703==    by 0x7D3AC46: QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate>::~QExplicitlySharedDataPointer() (qshareddata.h:165)
==1703==    by 0x7D36D63: QgsCoordinateTransform::~QgsCoordinateTransform() (qgscoordinatetransform.cpp:148)
==1703==    by 0x7D3C425: QHashNode<QPair<QString, QString>, QgsCoordinateTransform>::~QHashNode() (qhash.h:149)
==1703==    by 0x7D3C459: QHash<QPair<QString, QString>, QgsCoordinateTransform>::deleteNode2(QHashData::Node*) (qhash.h:536)
==1703==    by 0x9BEEB78: QHashData::free_helper(void (*)(QHashData::Node*)) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x7D3B81C: QHash<QPair<QString, QString>, QgsCoordinateTransform>::freeData(QHashData*) (qhash.h:576)
==1703==    by 0x7D3B2B3: QHash<QPair<QString, QString>, QgsCoordinateTransform>::~QHash() (qhash.h:254)
==1703==    by 0x7D3B7DA: QHash<QPair<QString, QString>, QgsCoordinateTransform>::operator=(QHash<QPair<QString, QString>, QgsCoordinateTransform>&&) (qhash.h:260)
==1703==    by 0x7D3B239: QHash<QPair<QString, QString>, QgsCoordinateTransform>::clear() (qhash.h:582)
==1703==    by 0x7D3A685: QgsCoordinateTransform::invalidateCache(bool) (qgscoordinatetransform.cpp:958)
==1703==    by 0x7C73601: QgsApplication::invalidateCaches() (qgsapplication.cpp:365)
==1703==    by 0x7C7A490: QgsApplication::exitQgis() (qgsapplication.cpp:1275)
==1703==    by 0x511BCC5: QgisApp::~QgisApp() (qgisapp.cpp:1662)
==1703==    by 0x511BE73: QgisApp::~QgisApp() (qgisapp.cpp:1664)
==1703==    by 0x418CB4: main (main.cpp:1579)
==1703==  Address 0x48fc4710 is 0 bytes inside a block of size 136 free'd
==1703==    at 0x4C2F440: operator delete(void*) (vg_replace_malloc.c:586)
==1703==    by 0xE625A3A: internal_proj_context_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B78A: QgsProjContext::~QgsProjContext() (qgsprojutils.cpp:48)
==1703==    by 0xA6195FE: __call_tls_dtors (cxa_thread_atexit_impl.c:155)
==1703==    by 0x10A936C7: start_thread (pthread_create.c:343)
==1703==    by 0xA6E641C: clone (clone.S:109)
==1703==  Block was alloc'd at
==1703==    at 0x4C2E709: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:387)
==1703==    by 0xE61445E: internal_pj_ctx_alloc (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B761: QgsProjContext::QgsProjContext() (qgsprojutils.cpp:39)
==1703==    by 0x7F9C6B9: __tls_init (qgsprojutils.cpp:31)
==1703==    by 0x7F9C703: TLS wrapper function for QgsProjContext::sProjContext (in /home/even/qgis/QGIS/build/output/lib/libqgis_core.so.3.9.0)
==1703==    by 0x7F9B796: QgsProjContext::get() (qgsprojutils.cpp:57)
==1703==    by 0x7D3D5AA: QgsCoordinateTransformPrivate::threadLocalProjData() (qgscoordinatetransform_p.cpp:299)
==1703==    by 0x7D3D11A: QgsCoordinateTransformPrivate::initialize() (qgscoordinatetransform_p.cpp:199)
==1703==    by 0x7D368E4: QgsCoordinateTransform::QgsCoordinateTransform(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QgsCoordinateTransformContext const&) (qgscoordinatetransform.cpp:75)
==1703==    by 0x8317CBE: QgsRasterProjector::block(int, QgsRectangle const&, int, int, QgsRasterBlockFeedback*) (qgsrasterprojector.cpp:779)
==1703==    by 0x82EBE87: QgsRasterIterator::readNextRasterPartInternal(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >*, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:130)
==1703==    by 0x82EB791: QgsRasterIterator::readNextRasterPart(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >&, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:80)
==1703==    by 0x8334E8D: QgsRasterDrawer::draw(QPainter*, QgsRasterViewPort*, QgsMapToPixel const*, QgsRasterBlockFeedback*) (qgsrasterdrawer.cpp:60)
==1703==    by 0x830AEE3: QgsRasterLayerRenderer::render() (qgsrasterlayerrenderer.cpp:269)
==1703==    by 0x7E764FB: QgsMapRendererCustomPainterJob::doRender() (qgsmaprenderercustompainterjob.cpp:310)
==1703==    by 0x7E76027: QgsMapRendererCustomPainterJob::staticRender(QgsMapRendererCustomPainterJob*) (qgsmaprenderercustompainterjob.cpp:261)
==1703==    by 0x7E788D6: QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor() (qtconcurrentstoredfunctioncall.h:432)
==1703==    by 0x7E76E42: QtConcurrent::RunFunctionTask<void>::run() (qtconcurrentrunbase.h:136)
==1703==    by 0x9B93942: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x9B97658: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x10A936B9: start_thread (pthread_create.c:333)
==1703==    by 0xA6E641C: clone (clone.S:109)

This issue is also found on QGIS 3.4 / PROJ 5 with a slighly comparable stack
trace.

The issue here is that the static QgsCoordinateTransform::sTransforms cache map
contains QgsCoordinateTransformPrivate::mProjProjections objects, which themselves
are PJ*/projPJ PROJ objects. Those objects may have been created by a thread,
using a PROJ context, which is a TLS object, and thus has been deleted when
the thread is itself deleted. However sTransforms is cleaned afterwards, and
when destroying a PJ*/projPJ object, the context it is attached to must still
be alive.

This fix of this commit consists in creating a temporary PROJ context and assigning
it to the PJ* object before its destruction.

A proper fix would be to remove from sTransforms the PROJ objects that belong to
a given thread when that thread is deleted (or more exactly QgsProjContext is destroyed),
but that's more involved. Another reason for such a proper fix is to avoid a
kind of memory leak, since currently sTransforms might grow without control when
threads are created and destroyed.

@rouault rouault added Needs Backporting Bug Either a bug report, or a bug fix. Let's hope for the latter! labels Sep 17, 2019
@nirvn
Copy link
Contributor

nirvn commented Sep 17, 2019

You are a hero, you deserve an award... oh wait, that just happened ;)

Fantastic work, you've just resolved a long, long standing issue.

…gis#31762)

This fixes the following Valgrind reported error:
```
==1703== Invalid read of size 4
==1703==    at 0xE614195: internal_pj_ctx_get_errno (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE6116AE: internal_pj_free (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE624E78: internal_proj_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7D3EFE7: QgsCoordinateTransformPrivate::freeProj() (qgscoordinatetransform_p.cpp:659)
==1703==    by 0x7D3CD23: QgsCoordinateTransformPrivate::~QgsCoordinateTransformPrivate() (qgscoordinatetransform_p.cpp:120)
==1703==    by 0x7D3AC46: QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate>::~QExplicitlySharedDataPointer() (qshareddata.h:165)
==1703==    by 0x7D36D63: QgsCoordinateTransform::~QgsCoordinateTransform() (qgscoordinatetransform.cpp:148)
==1703==    by 0x7D3C425: QHashNode<QPair<QString, QString>, QgsCoordinateTransform>::~QHashNode() (qhash.h:149)
==1703==    by 0x7D3C459: QHash<QPair<QString, QString>, QgsCoordinateTransform>::deleteNode2(QHashData::Node*) (qhash.h:536)
==1703==    by 0x9BEEB78: QHashData::free_helper(void (*)(QHashData::Node*)) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x7D3B81C: QHash<QPair<QString, QString>, QgsCoordinateTransform>::freeData(QHashData*) (qhash.h:576)
==1703==    by 0x7D3B2B3: QHash<QPair<QString, QString>, QgsCoordinateTransform>::~QHash() (qhash.h:254)
==1703==    by 0x7D3B7DA: QHash<QPair<QString, QString>, QgsCoordinateTransform>::operator=(QHash<QPair<QString, QString>, QgsCoordinateTransform>&&) (qhash.h:260)
==1703==    by 0x7D3B239: QHash<QPair<QString, QString>, QgsCoordinateTransform>::clear() (qhash.h:582)
==1703==    by 0x7D3A685: QgsCoordinateTransform::invalidateCache(bool) (qgscoordinatetransform.cpp:958)
==1703==    by 0x7C73601: QgsApplication::invalidateCaches() (qgsapplication.cpp:365)
==1703==    by 0x7C7A490: QgsApplication::exitQgis() (qgsapplication.cpp:1275)
==1703==    by 0x511BCC5: QgisApp::~QgisApp() (qgisapp.cpp:1662)
==1703==    by 0x511BE73: QgisApp::~QgisApp() (qgisapp.cpp:1664)
==1703==    by 0x418CB4: main (main.cpp:1579)
==1703==  Address 0x48fc4710 is 0 bytes inside a block of size 136 free'd
==1703==    at 0x4C2F440: operator delete(void*) (vg_replace_malloc.c:586)
==1703==    by 0xE625A3A: internal_proj_context_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B78A: QgsProjContext::~QgsProjContext() (qgsprojutils.cpp:48)
==1703==    by 0xA6195FE: __call_tls_dtors (cxa_thread_atexit_impl.c:155)
==1703==    by 0x10A936C7: start_thread (pthread_create.c:343)
==1703==    by 0xA6E641C: clone (clone.S:109)
==1703==  Block was alloc'd at
==1703==    at 0x4C2E709: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:387)
==1703==    by 0xE61445E: internal_pj_ctx_alloc (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B761: QgsProjContext::QgsProjContext() (qgsprojutils.cpp:39)
==1703==    by 0x7F9C6B9: __tls_init (qgsprojutils.cpp:31)
==1703==    by 0x7F9C703: TLS wrapper function for QgsProjContext::sProjContext (in /home/even/qgis/QGIS/build/output/lib/libqgis_core.so.3.9.0)
==1703==    by 0x7F9B796: QgsProjContext::get() (qgsprojutils.cpp:57)
==1703==    by 0x7D3D5AA: QgsCoordinateTransformPrivate::threadLocalProjData() (qgscoordinatetransform_p.cpp:299)
==1703==    by 0x7D3D11A: QgsCoordinateTransformPrivate::initialize() (qgscoordinatetransform_p.cpp:199)
==1703==    by 0x7D368E4: QgsCoordinateTransform::QgsCoordinateTransform(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QgsCoordinateTransformContext const&) (qgscoordinatetransform.cpp:75)
==1703==    by 0x8317CBE: QgsRasterProjector::block(int, QgsRectangle const&, int, int, QgsRasterBlockFeedback*) (qgsrasterprojector.cpp:779)
==1703==    by 0x82EBE87: QgsRasterIterator::readNextRasterPartInternal(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >*, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:130)
==1703==    by 0x82EB791: QgsRasterIterator::readNextRasterPart(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >&, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:80)
==1703==    by 0x8334E8D: QgsRasterDrawer::draw(QPainter*, QgsRasterViewPort*, QgsMapToPixel const*, QgsRasterBlockFeedback*) (qgsrasterdrawer.cpp:60)
==1703==    by 0x830AEE3: QgsRasterLayerRenderer::render() (qgsrasterlayerrenderer.cpp:269)
==1703==    by 0x7E764FB: QgsMapRendererCustomPainterJob::doRender() (qgsmaprenderercustompainterjob.cpp:310)
==1703==    by 0x7E76027: QgsMapRendererCustomPainterJob::staticRender(QgsMapRendererCustomPainterJob*) (qgsmaprenderercustompainterjob.cpp:261)
==1703==    by 0x7E788D6: QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor() (qtconcurrentstoredfunctioncall.h:432)
==1703==    by 0x7E76E42: QtConcurrent::RunFunctionTask<void>::run() (qtconcurrentrunbase.h:136)
==1703==    by 0x9B93942: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x9B97658: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x10A936B9: start_thread (pthread_create.c:333)
==1703==    by 0xA6E641C: clone (clone.S:109)
```

This issue is also found on QGIS 3.4 / PROJ 5 with a slighly comparable stack
trace.

The issue here is that the static QgsCoordinateTransform::sTransforms cache map
contains QgsCoordinateTransformPrivate::mProjProjections objects, which themselves
are PJ*/projPJ PROJ objects. Those objects may have been created by a thread,
using a PROJ context, which is a TLS object, and thus has been deleted when
the thread is itself deleted. However sTransforms is cleaned afterwards, and
when destroying a PJ*/projPJ object, the context it is attached to must still
be alive.

This fix of this commit consists in creating a temporary PROJ context and assigning
it to the PJ* object before its destruction.

A proper fix would be to remove from sTransforms the PROJ objects that belong to
a given thread when that thread is deleted (or more exactly QgsProjContext is destroyed),
but that's more involved. Another reason for such a proper fix is to avoid a
kind of memory leak, since currently sTransforms might grow without control when
threads are created and destroyed.
rouault added a commit to rouault/QGIS that referenced this pull request Sep 17, 2019
…gis#31762)

Backport of qgis#31848

This fixes the following Valgrind reported error:
```
==1703== Invalid read of size 4
==1703==    at 0xE614195: internal_pj_ctx_get_errno (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE6116AE: internal_pj_free (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE624E78: internal_proj_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7D3EFE7: QgsCoordinateTransformPrivate::freeProj() (qgscoordinatetransform_p.cpp:659)
==1703==    by 0x7D3CD23: QgsCoordinateTransformPrivate::~QgsCoordinateTransformPrivate() (qgscoordinatetransform_p.cpp:120)
==1703==    by 0x7D3AC46: QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate>::~QExplicitlySharedDataPointer() (qshareddata.h:165)
==1703==    by 0x7D36D63: QgsCoordinateTransform::~QgsCoordinateTransform() (qgscoordinatetransform.cpp:148)
==1703==    by 0x7D3C425: QHashNode<QPair<QString, QString>, QgsCoordinateTransform>::~QHashNode() (qhash.h:149)
==1703==    by 0x7D3C459: QHash<QPair<QString, QString>, QgsCoordinateTransform>::deleteNode2(QHashData::Node*) (qhash.h:536)
==1703==    by 0x9BEEB78: QHashData::free_helper(void (*)(QHashData::Node*)) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x7D3B81C: QHash<QPair<QString, QString>, QgsCoordinateTransform>::freeData(QHashData*) (qhash.h:576)
==1703==    by 0x7D3B2B3: QHash<QPair<QString, QString>, QgsCoordinateTransform>::~QHash() (qhash.h:254)
==1703==    by 0x7D3B7DA: QHash<QPair<QString, QString>, QgsCoordinateTransform>::operator=(QHash<QPair<QString, QString>, QgsCoordinateTransform>&&) (qhash.h:260)
==1703==    by 0x7D3B239: QHash<QPair<QString, QString>, QgsCoordinateTransform>::clear() (qhash.h:582)
==1703==    by 0x7D3A685: QgsCoordinateTransform::invalidateCache(bool) (qgscoordinatetransform.cpp:958)
==1703==    by 0x7C73601: QgsApplication::invalidateCaches() (qgsapplication.cpp:365)
==1703==    by 0x7C7A490: QgsApplication::exitQgis() (qgsapplication.cpp:1275)
==1703==    by 0x511BCC5: QgisApp::~QgisApp() (qgisapp.cpp:1662)
==1703==    by 0x511BE73: QgisApp::~QgisApp() (qgisapp.cpp:1664)
==1703==    by 0x418CB4: main (main.cpp:1579)
==1703==  Address 0x48fc4710 is 0 bytes inside a block of size 136 free'd
==1703==    at 0x4C2F440: operator delete(void*) (vg_replace_malloc.c:586)
==1703==    by 0xE625A3A: internal_proj_context_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B78A: QgsProjContext::~QgsProjContext() (qgsprojutils.cpp:48)
==1703==    by 0xA6195FE: __call_tls_dtors (cxa_thread_atexit_impl.c:155)
==1703==    by 0x10A936C7: start_thread (pthread_create.c:343)
==1703==    by 0xA6E641C: clone (clone.S:109)
==1703==  Block was alloc'd at
==1703==    at 0x4C2E709: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:387)
==1703==    by 0xE61445E: internal_pj_ctx_alloc (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B761: QgsProjContext::QgsProjContext() (qgsprojutils.cpp:39)
==1703==    by 0x7F9C6B9: __tls_init (qgsprojutils.cpp:31)
==1703==    by 0x7F9C703: TLS wrapper function for QgsProjContext::sProjContext (in /home/even/qgis/QGIS/build/output/lib/libqgis_core.so.3.9.0)
==1703==    by 0x7F9B796: QgsProjContext::get() (qgsprojutils.cpp:57)
==1703==    by 0x7D3D5AA: QgsCoordinateTransformPrivate::threadLocalProjData() (qgscoordinatetransform_p.cpp:299)
==1703==    by 0x7D3D11A: QgsCoordinateTransformPrivate::initialize() (qgscoordinatetransform_p.cpp:199)
==1703==    by 0x7D368E4: QgsCoordinateTransform::QgsCoordinateTransform(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QgsCoordinateTransformContext const&) (qgscoordinatetransform.cpp:75)
==1703==    by 0x8317CBE: QgsRasterProjector::block(int, QgsRectangle const&, int, int, QgsRasterBlockFeedback*) (qgsrasterprojector.cpp:779)
==1703==    by 0x82EBE87: QgsRasterIterator::readNextRasterPartInternal(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >*, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:130)
==1703==    by 0x82EB791: QgsRasterIterator::readNextRasterPart(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >&, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:80)
==1703==    by 0x8334E8D: QgsRasterDrawer::draw(QPainter*, QgsRasterViewPort*, QgsMapToPixel const*, QgsRasterBlockFeedback*) (qgsrasterdrawer.cpp:60)
==1703==    by 0x830AEE3: QgsRasterLayerRenderer::render() (qgsrasterlayerrenderer.cpp:269)
==1703==    by 0x7E764FB: QgsMapRendererCustomPainterJob::doRender() (qgsmaprenderercustompainterjob.cpp:310)
==1703==    by 0x7E76027: QgsMapRendererCustomPainterJob::staticRender(QgsMapRendererCustomPainterJob*) (qgsmaprenderercustompainterjob.cpp:261)
==1703==    by 0x7E788D6: QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor() (qtconcurrentstoredfunctioncall.h:432)
==1703==    by 0x7E76E42: QtConcurrent::RunFunctionTask<void>::run() (qtconcurrentrunbase.h:136)
==1703==    by 0x9B93942: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x9B97658: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x10A936B9: start_thread (pthread_create.c:333)
==1703==    by 0xA6E641C: clone (clone.S:109)
```

This issue is also found on QGIS 3.4 / PROJ 5 with a slighly comparable stack
trace.

The issue here is that the static QgsCoordinateTransform::sTransforms cache map
contains QgsCoordinateTransformPrivate::mProjProjections objects, which themselves
are PJ*/projPJ PROJ objects. Those objects may have been created by a thread,
using a PROJ context, which is a TLS object, and thus has been deleted when
the thread is itself deleted. However sTransforms is cleaned afterwards, and
when destroying a PJ*/projPJ object, the context it is attached to must still
be alive.

This fix of this commit consists in creating a temporary PROJ context and assigning
it to the PJ* object before its destruction.

A proper fix would be to remove from sTransforms the PROJ objects that belong to
a given thread when that thread is deleted (or more exactly QgsProjContext is destroyed),
but that's more involved. Another reason for such a proper fix is to avoid a
kind of memory leak, since currently sTransforms might grow without control when
threads are created and destroyed.
@rouault
Copy link
Contributor Author

rouault commented Sep 17, 2019

you've just resolved a long, long standing issue.

nothing would have been possible without your reproducer (and Valgrind !) :-)

rouault added a commit to rouault/QGIS that referenced this pull request Sep 17, 2019
… (relates to qgis#31762)

Follow-up to qgis#31848

When a thread exits, make sure that we iterate over QgsCoordinateTransform::sTransforms
to remove from QgsCoordinateTransformPrivate::mProjProjections objects that
relates to the current thread, in order to free memory.

This is only implemented in the PROJ >= 6 case. In theory, we could probably
now revert //github.com/qgis/pull/31848 for its PROJ 6 code paths, but
keeping it doesn't hurt...

A similar fix could potentially be done in PROJ < 6 case, but it would require
probably the starting call to be in QgsProjContextStore::~QgsProjContextStore()
instead of QgsProjContext::~QgsProjContext()
nyalldawson pushed a commit that referenced this pull request Sep 17, 2019
…31762)

Backport of #31848

This fixes the following Valgrind reported error:
```
==1703== Invalid read of size 4
==1703==    at 0xE614195: internal_pj_ctx_get_errno (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE6116AE: internal_pj_free (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0xE624E78: internal_proj_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7D3EFE7: QgsCoordinateTransformPrivate::freeProj() (qgscoordinatetransform_p.cpp:659)
==1703==    by 0x7D3CD23: QgsCoordinateTransformPrivate::~QgsCoordinateTransformPrivate() (qgscoordinatetransform_p.cpp:120)
==1703==    by 0x7D3AC46: QExplicitlySharedDataPointer<QgsCoordinateTransformPrivate>::~QExplicitlySharedDataPointer() (qshareddata.h:165)
==1703==    by 0x7D36D63: QgsCoordinateTransform::~QgsCoordinateTransform() (qgscoordinatetransform.cpp:148)
==1703==    by 0x7D3C425: QHashNode<QPair<QString, QString>, QgsCoordinateTransform>::~QHashNode() (qhash.h:149)
==1703==    by 0x7D3C459: QHash<QPair<QString, QString>, QgsCoordinateTransform>::deleteNode2(QHashData::Node*) (qhash.h:536)
==1703==    by 0x9BEEB78: QHashData::free_helper(void (*)(QHashData::Node*)) (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x7D3B81C: QHash<QPair<QString, QString>, QgsCoordinateTransform>::freeData(QHashData*) (qhash.h:576)
==1703==    by 0x7D3B2B3: QHash<QPair<QString, QString>, QgsCoordinateTransform>::~QHash() (qhash.h:254)
==1703==    by 0x7D3B7DA: QHash<QPair<QString, QString>, QgsCoordinateTransform>::operator=(QHash<QPair<QString, QString>, QgsCoordinateTransform>&&) (qhash.h:260)
==1703==    by 0x7D3B239: QHash<QPair<QString, QString>, QgsCoordinateTransform>::clear() (qhash.h:582)
==1703==    by 0x7D3A685: QgsCoordinateTransform::invalidateCache(bool) (qgscoordinatetransform.cpp:958)
==1703==    by 0x7C73601: QgsApplication::invalidateCaches() (qgsapplication.cpp:365)
==1703==    by 0x7C7A490: QgsApplication::exitQgis() (qgsapplication.cpp:1275)
==1703==    by 0x511BCC5: QgisApp::~QgisApp() (qgisapp.cpp:1662)
==1703==    by 0x511BE73: QgisApp::~QgisApp() (qgisapp.cpp:1664)
==1703==    by 0x418CB4: main (main.cpp:1579)
==1703==  Address 0x48fc4710 is 0 bytes inside a block of size 136 free'd
==1703==    at 0x4C2F440: operator delete(void*) (vg_replace_malloc.c:586)
==1703==    by 0xE625A3A: internal_proj_context_destroy (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B78A: QgsProjContext::~QgsProjContext() (qgsprojutils.cpp:48)
==1703==    by 0xA6195FE: __call_tls_dtors (cxa_thread_atexit_impl.c:155)
==1703==    by 0x10A936C7: start_thread (pthread_create.c:343)
==1703==    by 0xA6E641C: clone (clone.S:109)
==1703==  Block was alloc'd at
==1703==    at 0x4C2E709: operator new(unsigned long, std::nothrow_t const&) (vg_replace_malloc.c:387)
==1703==    by 0xE61445E: internal_pj_ctx_alloc (in /home/even/proj/install-proj-master/lib/libproj.so.15.2.0)
==1703==    by 0x7F9B761: QgsProjContext::QgsProjContext() (qgsprojutils.cpp:39)
==1703==    by 0x7F9C6B9: __tls_init (qgsprojutils.cpp:31)
==1703==    by 0x7F9C703: TLS wrapper function for QgsProjContext::sProjContext (in /home/even/qgis/QGIS/build/output/lib/libqgis_core.so.3.9.0)
==1703==    by 0x7F9B796: QgsProjContext::get() (qgsprojutils.cpp:57)
==1703==    by 0x7D3D5AA: QgsCoordinateTransformPrivate::threadLocalProjData() (qgscoordinatetransform_p.cpp:299)
==1703==    by 0x7D3D11A: QgsCoordinateTransformPrivate::initialize() (qgscoordinatetransform_p.cpp:199)
==1703==    by 0x7D368E4: QgsCoordinateTransform::QgsCoordinateTransform(QgsCoordinateReferenceSystem const&, QgsCoordinateReferenceSystem const&, QgsCoordinateTransformContext const&) (qgscoordinatetransform.cpp:75)
==1703==    by 0x8317CBE: QgsRasterProjector::block(int, QgsRectangle const&, int, int, QgsRasterBlockFeedback*) (qgsrasterprojector.cpp:779)
==1703==    by 0x82EBE87: QgsRasterIterator::readNextRasterPartInternal(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >*, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:130)
==1703==    by 0x82EB791: QgsRasterIterator::readNextRasterPart(int, int&, int&, std::unique_ptr<QgsRasterBlock, std::default_delete<QgsRasterBlock> >&, int&, int&, QgsRectangle*) (qgsrasteriterator.cpp:80)
==1703==    by 0x8334E8D: QgsRasterDrawer::draw(QPainter*, QgsRasterViewPort*, QgsMapToPixel const*, QgsRasterBlockFeedback*) (qgsrasterdrawer.cpp:60)
==1703==    by 0x830AEE3: QgsRasterLayerRenderer::render() (qgsrasterlayerrenderer.cpp:269)
==1703==    by 0x7E764FB: QgsMapRendererCustomPainterJob::doRender() (qgsmaprenderercustompainterjob.cpp:310)
==1703==    by 0x7E76027: QgsMapRendererCustomPainterJob::staticRender(QgsMapRendererCustomPainterJob*) (qgsmaprenderercustompainterjob.cpp:261)
==1703==    by 0x7E788D6: QtConcurrent::StoredFunctorCall1<void, void (*)(QgsMapRendererCustomPainterJob*), QgsMapRendererCustomPainterJob*>::runFunctor() (qtconcurrentstoredfunctioncall.h:432)
==1703==    by 0x7E76E42: QtConcurrent::RunFunctionTask<void>::run() (qtconcurrentrunbase.h:136)
==1703==    by 0x9B93942: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x9B97658: ??? (in /opt/qt59/lib/libQt5Core.so.5.9.1)
==1703==    by 0x10A936B9: start_thread (pthread_create.c:333)
==1703==    by 0xA6E641C: clone (clone.S:109)
```

This issue is also found on QGIS 3.4 / PROJ 5 with a slighly comparable stack
trace.

The issue here is that the static QgsCoordinateTransform::sTransforms cache map
contains QgsCoordinateTransformPrivate::mProjProjections objects, which themselves
are PJ*/projPJ PROJ objects. Those objects may have been created by a thread,
using a PROJ context, which is a TLS object, and thus has been deleted when
the thread is itself deleted. However sTransforms is cleaned afterwards, and
when destroying a PJ*/projPJ object, the context it is attached to must still
be alive.

This fix of this commit consists in creating a temporary PROJ context and assigning
it to the PJ* object before its destruction.

A proper fix would be to remove from sTransforms the PROJ objects that belong to
a given thread when that thread is deleted (or more exactly QgsProjContext is destroyed),
but that's more involved. Another reason for such a proper fix is to avoid a
kind of memory leak, since currently sTransforms might grow without control when
threads are created and destroyed.
@nirvn
Copy link
Contributor

nirvn commented Sep 18, 2019

Should we merge this now? The 3.4 backport was already merged, I assume it's been deemed safe :)

@nyalldawson
Copy link
Collaborator

@nirvn wasn't this replaced by #31857?

@nirvn
Copy link
Contributor

nirvn commented Sep 18, 2019

@nyalldawson, I think the other PR carries on this commit as it's needed for the second commit relevant to that other PR.

@rouault
Copy link
Contributor Author

rouault commented Sep 18, 2019

You can merge this one directly.
#31857 can be merged afterwards, and contains more a nice-to-have memory leak fix, but not critical for application termination

@nirvn nirvn merged commit b5be645 into qgis:master Sep 18, 2019
nirvn pushed a commit that referenced this pull request Sep 18, 2019
… (relates to #31762)

Follow-up to #31848

When a thread exits, make sure that we iterate over QgsCoordinateTransform::sTransforms
to remove from QgsCoordinateTransformPrivate::mProjProjections objects that
relates to the current thread, in order to free memory.

This is only implemented in the PROJ >= 6 case. In theory, we could probably
now revert //github.com//pull/31848 for its PROJ 6 code paths, but
keeping it doesn't hurt...

A similar fix could potentially be done in PROJ < 6 case, but it would require
probably the starting call to be in QgsProjContextStore::~QgsProjContextStore()
instead of QgsProjContext::~QgsProjContext()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Needs Backporting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants