From e635dc72614f64a0ee0d9209069fc75520bb51cf Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 15 Oct 2024 09:45:10 +1000 Subject: [PATCH 1/3] De-duplicate proj logging functions (cherry picked from commit 97c5f95a7473c5432dea6556b5c73097abadc870) --- src/core/proj/qgscoordinatetransform_p.cpp | 34 ++-------------------- src/core/proj/qgsprojutils.cpp | 34 ++++++++++++++++++++++ src/core/proj/qgsprojutils.h | 16 ++++++++++ src/gui/qgscrsdefinitionwidget.cpp | 10 +------ 4 files changed, 53 insertions(+), 41 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform_p.cpp b/src/core/proj/qgscoordinatetransform_p.cpp index 63e438a90f9e..571a9fcb7cce 100644 --- a/src/core/proj/qgscoordinatetransform_p.cpp +++ b/src/core/proj/qgscoordinatetransform_p.cpp @@ -244,36 +244,6 @@ void QgsCoordinateTransformPrivate::calculateTransforms( const QgsCoordinateTran } } -static void proj_collecting_logger( void *user_data, int /*level*/, const char *message ) -{ - QStringList *dest = reinterpret_cast< QStringList * >( user_data ); - dest->append( QString( message ) ); -} - -static void proj_logger( void *, int level, const char *message ) -{ -#ifndef QGISDEBUG - Q_UNUSED( message ) -#endif - if ( level == PJ_LOG_ERROR ) - { - const QString messageString( message ); - if ( messageString == QLatin1String( "push: Invalid latitude" ) ) - { - // these messages tend to spam the console as they can be repeated 1000s of times - QgsDebugMsgLevel( messageString, 3 ); - } - else - { - QgsDebugError( messageString ); - } - } - else if ( level == PJ_LOG_DEBUG ) - { - QgsDebugMsgLevel( QString( message ), 3 ); - } -} - ProjData QgsCoordinateTransformPrivate::threadLocalProjData() { QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Read ); @@ -292,7 +262,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() // use a temporary proj error collector QStringList projErrors; - proj_log_func( context, &projErrors, proj_collecting_logger ); + proj_log_func( context, &projErrors, QgsProjUtils::proj_collecting_logger ); mIsReversed = false; @@ -530,7 +500,7 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() } // reset logger to terminal output - proj_log_func( context, nullptr, proj_logger ); + proj_log_func( context, nullptr, QgsProjUtils::proj_logger ); if ( !transform ) { diff --git a/src/core/proj/qgsprojutils.cpp b/src/core/proj/qgsprojutils.cpp index 437d8bada109..be9497da4403 100644 --- a/src/core/proj/qgsprojutils.cpp +++ b/src/core/proj/qgsprojutils.cpp @@ -259,6 +259,40 @@ QgsProjUtils::proj_pj_unique_ptr QgsProjUtils::crsToDatumEnsemble( const PJ *crs #endif } +void QgsProjUtils::proj_collecting_logger( void *user_data, int /*level*/, const char *message ) +{ + QStringList *dest = reinterpret_cast< QStringList * >( user_data ); + QString messageString( message ); + messageString.replace( QLatin1String( "internal_proj_create: " ), QString() ); + dest->append( messageString ); +} + +void QgsProjUtils::proj_logger( void *, int level, const char *message ) +{ +#ifdef QGISDEBUG + if ( level == PJ_LOG_ERROR ) + { + const QString messageString( message ); + if ( messageString == QLatin1String( "push: Invalid latitude" ) ) + { + // these messages tend to spam the console as they can be repeated 1000s of times + QgsDebugMsgLevel( messageString, 3 ); + } + else + { + QgsDebugError( messageString ); + } + } + else if ( level == PJ_LOG_DEBUG ) + { + QgsDebugMsgLevel( QString( message ), 3 ); + } +#else + ( void )level; + ( void )message; +#endif +} + bool QgsProjUtils::identifyCrs( const PJ *crs, QString &authName, QString &authCode, IdentifyFlags flags ) { authName.clear(); diff --git a/src/core/proj/qgsprojutils.h b/src/core/proj/qgsprojutils.h index 3efe159469d0..6b04e2b2f045 100644 --- a/src/core/proj/qgsprojutils.h +++ b/src/core/proj/qgsprojutils.h @@ -211,6 +211,22 @@ class CORE_EXPORT QgsProjUtils */ static QList< QgsDatumTransform::GridDetails > gridsUsed( const QString &proj ); + /** + * Default QGIS proj log function. + * + * Uses QgsDebugError or QgsDebugMsgLevel to report errors in debug builds only. + */ + static void proj_logger( void *user_data, int level, const char *message ); + + /** + * QGIS proj log function which collects errors to a QStringList. + * + * \warning The user_data argument passed to proj_log_func MUST be a QStringList object, + * and must exist for the duration where proj_collecting_logger is used. You MUST reset + * the proj_log_func to proj_logger before the user data QStringList is destroyed. + */ + static void proj_collecting_logger( void *user_data, int level, const char *message ); + #if 0 // not possible in current Proj 6 API /** diff --git a/src/gui/qgscrsdefinitionwidget.cpp b/src/gui/qgscrsdefinitionwidget.cpp index 6f9c5ed44806..f8faf0561249 100644 --- a/src/gui/qgscrsdefinitionwidget.cpp +++ b/src/gui/qgscrsdefinitionwidget.cpp @@ -110,14 +110,6 @@ void QgsCrsDefinitionWidget::pbnCopyCRS_clicked() } } -static void proj_collecting_logger( void *user_data, int /*level*/, const char *message ) -{ - QStringList *dest = reinterpret_cast< QStringList * >( user_data ); - QString messageString( message ); - messageString.replace( QLatin1String( "internal_proj_create: " ), QString() ); - dest->append( messageString ); -} - void QgsCrsDefinitionWidget::validateCurrent() { const QString projDef = mTextEditParameters->toPlainText(); @@ -125,7 +117,7 @@ void QgsCrsDefinitionWidget::validateCurrent() PJ_CONTEXT *context = proj_context_create(); QStringList projErrors; - proj_log_func( context, &projErrors, proj_collecting_logger ); + proj_log_func( context, &projErrors, QgsProjUtils::proj_collecting_logger ); QgsProjUtils::proj_pj_unique_ptr crs; switch ( static_cast< Qgis::CrsDefinitionFormat >( mFormatComboBox->currentData().toInt() ) ) From bbf2cd00de19085a857530ec502eae34047f787a Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 15 Oct 2024 09:59:14 +1000 Subject: [PATCH 2/3] 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 (cherry picked from commit 244da7a40211119811a321a3b98a720cb50be3dd) --- src/core/proj/qgscoordinatetransform_p.cpp | 8 ++--- src/core/proj/qgsprojutils.cpp | 15 ++++++++ src/core/proj/qgsprojutils.h | 40 ++++++++++++++++++++++ src/gui/qgscrsdefinitionwidget.cpp | 12 ++----- 4 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/core/proj/qgscoordinatetransform_p.cpp b/src/core/proj/qgscoordinatetransform_p.cpp index 571a9fcb7cce..789d71789ed8 100644 --- a/src/core/proj/qgscoordinatetransform_p.cpp +++ b/src/core/proj/qgscoordinatetransform_p.cpp @@ -261,8 +261,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; @@ -312,7 +311,6 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData() { if ( !mSourceCRS.projObject() || ! mDestCRS.projObject() ) { - proj_log_func( context, nullptr, nullptr ); return nullptr; } @@ -464,6 +462,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 ) ); @@ -499,9 +498,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 be9497da4403..d89840b042c5 100644 --- a/src/core/proj/qgsprojutils.cpp +++ b/src/core/proj/qgsprojutils.cpp @@ -490,3 +490,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 6b04e2b2f045..bea22384befd 100644 --- a/src/core/proj/qgsprojutils.h +++ b/src/core/proj/qgsprojutils.h @@ -281,6 +281,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/qgscrsdefinitionwidget.cpp b/src/gui/qgscrsdefinitionwidget.cpp index f8faf0561249..5fb9a315386a 100644 --- a/src/gui/qgscrsdefinitionwidget.cpp +++ b/src/gui/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() From 756e2442bfb1f38ce68672bac4a4060a827f8325 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 15 Oct 2024 14:11:12 +1000 Subject: [PATCH 3/3] Fix build --- src/core/proj/qgsprojutils.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/proj/qgsprojutils.cpp b/src/core/proj/qgsprojutils.cpp index d89840b042c5..c9eb605a68cc 100644 --- a/src/core/proj/qgsprojutils.cpp +++ b/src/core/proj/qgsprojutils.cpp @@ -18,6 +18,7 @@ #include "qgis.h" #include "qgscoordinatetransform.h" #include "qgsexception.h" +#include "qgslogger.h" #include #include #include