From 244da7a40211119811a321a3b98a720cb50be3dd Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 15 Oct 2024 09:59:14 +1000 Subject: [PATCH] Use a scoped proj logger to avoid user data lifetime issues Previously we incorrectly tried to reset the proj logger by calling proj_log_func with a nullptr function in some exit paths. This leads to crashes, as the nullptr arg makes proj_log_func a no-op, and then later proj tries to log using the now destroyed user data string list. Make all this more robust by switching to a scoped QgsScopedProjCollectingLogger class, which automatically correctly restores the default QGIS proj logger on destruction and ensures there's no object lifetime issues for the log function vs the user data object. Fixes #36125, other crashes seen in the wild --- src/core/proj/qgscoordinatetransform_p.cpp | 8 ++--- src/core/proj/qgsprojutils.cpp | 22 +++++++++--- src/core/proj/qgsprojutils.h | 40 ++++++++++++++++++++++ src/gui/proj/qgscrsdefinitionwidget.cpp | 12 ++----- 4 files changed, 62 insertions(+), 20 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform_p.cpp b/src/core/proj/qgscoordinatetransform_p.cpp index e59ef6f27109..a89b31c9757c 100644 --- a/src/core/proj/qgscoordinatetransform_p.cpp +++ b/src/core/proj/qgscoordinatetransform_p.cpp @@ -264,8 +264,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() locker.changeMode( QgsReadWriteLocker::Write ); // use a temporary proj error collector - QStringList projErrors; - proj_log_func( context, &projErrors, QgsProjUtils::proj_collecting_logger ); + QgsScopedProjCollectingLogger errorLogger; mIsReversed = false; @@ -315,7 +314,6 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() { if ( !mSourceCRS.projObject() || ! mDestCRS.projObject() ) { - proj_log_func( context, nullptr, nullptr ); return nullptr; } @@ -467,6 +465,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() if ( !transform && nonAvailableError.isEmpty() ) { const int errNo = proj_context_errno( context ); + const QStringList projErrors = errorLogger.errors(); if ( errNo && errNo != -61 ) { nonAvailableError = QString( proj_errno_string( errNo ) ); @@ -502,9 +501,6 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() } } - // reset logger to terminal output - proj_log_func( context, nullptr, QgsProjUtils::proj_logger ); - if ( !transform ) { // ouch! diff --git a/src/core/proj/qgsprojutils.cpp b/src/core/proj/qgsprojutils.cpp index ee79cb6c1932..453086b50622 100644 --- a/src/core/proj/qgsprojutils.cpp +++ b/src/core/proj/qgsprojutils.cpp @@ -397,8 +397,7 @@ QgsProjUtils::proj_pj_unique_ptr QgsProjUtils::createCompoundCrs( const PJ *hori PJ_CONTEXT *context = QgsProjContext::get(); // collect errors instead of dumping them to terminal - QStringList tempErrors; - proj_log_func( context, &tempErrors, proj_collecting_logger ); + QgsScopedProjCollectingLogger projLogger; // const cast here is for compatibility with proj < 9.5 QgsProjUtils::proj_pj_unique_ptr compoundCrs( proj_create_compound_crs( context, @@ -406,10 +405,8 @@ QgsProjUtils::proj_pj_unique_ptr QgsProjUtils::createCompoundCrs( const PJ *hori const_cast< PJ *>( horizontalCrs ), const_cast< PJ * >( verticalCrs ) ) ); - // reset logging function - proj_log_func( context, nullptr, proj_logger ); if ( errors ) - *errors = tempErrors; + *errors = projLogger.errors(); return compoundCrs; } @@ -611,3 +608,18 @@ QStringList QgsProjUtils::searchPaths() } return res; } + +// +// QgsScopedProjCollectingLogger +// + +QgsScopedProjCollectingLogger::QgsScopedProjCollectingLogger() +{ + proj_log_func( QgsProjContext::get(), &mProjErrors, QgsProjUtils::proj_collecting_logger ); +} + +QgsScopedProjCollectingLogger::~QgsScopedProjCollectingLogger() +{ + // reset logger back to terminal output + proj_log_func( QgsProjContext::get(), nullptr, QgsProjUtils::proj_logger ); +} diff --git a/src/core/proj/qgsprojutils.h b/src/core/proj/qgsprojutils.h index 06ffa012f624..862e52a76b87 100644 --- a/src/core/proj/qgsprojutils.h +++ b/src/core/proj/qgsprojutils.h @@ -311,6 +311,46 @@ class CORE_EXPORT QgsProjContext #endif }; + +/** + * \ingroup core + * + * \brief Scoped object for temporary swapping to an error-collecting PROJ log function. + * + * Temporarily sets the PROJ log function to one which collects errors for the lifetime of the object, + * before returning it to the default QGIS proj logging function on destruction. + * + * \note The collecting logger ONLY applies to the current thread. + * + * \note Not available in Python bindings + * \since QGIS 3.40 + */ +class CORE_EXPORT QgsScopedProjCollectingLogger +{ + public: + + /** + * Constructor for QgsScopedProjCollectingLogger. + * + * PROJ errors will be collected, and can be retrieved by calling errors(). + */ + QgsScopedProjCollectingLogger(); + + /** + * Returns the PROJ logger back to the default QGIS PROJ logger. + */ + ~QgsScopedProjCollectingLogger(); + + /** + * Returns the (possibly empty) list of collected errors. + */ + QStringList errors() const { return mProjErrors; } + + private: + + QStringList mProjErrors; +}; + Q_DECLARE_OPERATORS_FOR_FLAGS( QgsProjUtils::IdentifyFlags ) #endif #endif // QGSPROJUTILS_H diff --git a/src/gui/proj/qgscrsdefinitionwidget.cpp b/src/gui/proj/qgscrsdefinitionwidget.cpp index a87964284663..3ce7880a48b5 100644 --- a/src/gui/proj/qgscrsdefinitionwidget.cpp +++ b/src/gui/proj/qgscrsdefinitionwidget.cpp @@ -114,10 +114,9 @@ void QgsCrsDefinitionWidget::validateCurrent() { const QString projDef = mTextEditParameters->toPlainText(); - PJ_CONTEXT *context = proj_context_create(); + PJ_CONTEXT *context = QgsProjContext::get(); - QStringList projErrors; - proj_log_func( context, &projErrors, QgsProjUtils::proj_collecting_logger ); + QgsScopedProjCollectingLogger projLogger; QgsProjUtils::proj_pj_unique_ptr crs; switch ( static_cast< Qgis::CrsDefinitionFormat >( mFormatComboBox->currentData().toInt() ) ) @@ -161,16 +160,11 @@ void QgsCrsDefinitionWidget::validateCurrent() else { QMessageBox::warning( this, tr( "Custom Coordinate Reference System" ), - tr( "This proj projection definition is not valid:" ) + QStringLiteral( "\n\n" ) + projErrors.join( '\n' ) ); + tr( "This proj projection definition is not valid:" ) + QStringLiteral( "\n\n" ) + projLogger.errors().join( '\n' ) ); } break; } } - - // reset logger to terminal output - proj_log_func( context, nullptr, nullptr ); - proj_context_destroy( context ); - context = nullptr; } void QgsCrsDefinitionWidget::formatChanged()