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 #36125, other crashes seen in the wild
  • Loading branch information
nyalldawson committed Oct 15, 2024
1 parent 1d791b8 commit 244da7a
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 20 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 @@ -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;

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

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

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

if ( !transform )
{
// ouch!
Expand Down
22 changes: 17 additions & 5 deletions src/core/proj/qgsprojutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -397,19 +397,16 @@ 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,
nullptr,
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;
}
Expand Down Expand Up @@ -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 );
}
40 changes: 40 additions & 0 deletions src/core/proj/qgsprojutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
12 changes: 3 additions & 9 deletions src/gui/proj/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 244da7a

Please sign in to comment.