Skip to content

Commit

Permalink
Use a scoped proj logger to avoid user data lifetime issues
Browse files Browse the repository at this point in the history
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 qgis#36125, other crashes seen in the wild

(cherry picked from commit 244da7a)
  • Loading branch information
nyalldawson committed Oct 15, 2024
1 parent e635dc7 commit bbf2cd0
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 15 deletions.
8 changes: 2 additions & 6 deletions src/core/proj/qgscoordinatetransform_p.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -312,7 +311,6 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData()
{
if ( !mSourceCRS.projObject() || ! mDestCRS.projObject() )
{
proj_log_func( context, nullptr, nullptr );
return nullptr;
}

Expand Down Expand Up @@ -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 ) );
Expand Down Expand Up @@ -499,9 +498,6 @@ ProjData QgsCoordinateTransformPrivate::threadLocalProjData()
}
}

// reset logger to terminal output
proj_log_func( context, nullptr, QgsProjUtils::proj_logger );

if ( !transform )
{
// ouch!
Expand Down
15 changes: 15 additions & 0 deletions src/core/proj/qgsprojutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}
40 changes: 40 additions & 0 deletions src/core/proj/qgsprojutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 3 additions & 9 deletions src/gui/qgscrsdefinitionwidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() ) )
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit bbf2cd0

Please sign in to comment.