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.

Sign up for GitHub

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

Jump to bottom

Fix QgsNetworkAccessManager ssl error handler race condition issue #50443

Merged
merged 1 commit into from Oct 6, 2022
Merged

Fix QgsNetworkAccessManager ssl error handler race condition issue #50443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 9 additions & 16 deletions src/core/network/qgsnetworkaccessmanager.cpp
View file
Open in desktop
Expand Up @@ -207,6 +207,7 @@ QgsNetworkAccessManager *QgsNetworkAccessManager::instance( Qt::ConnectionType c

QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent )
: QNetworkAccessManager( parent )
, mSslErrorHandlerSemaphore( 1 )
, mAuthRequestHandlerSemaphore( 1 )
{
setProxyFactory( new QgsNetworkProxyFactory() );
Expand Down Expand Up @@ -385,14 +386,6 @@ QNetworkReply *QgsNetworkAccessManager::createRequest( QNetworkAccessManager::Op
return reply;
}

#ifndef QT_NO_SSL
void QgsNetworkAccessManager::unlockAfterSslErrorHandled()
{
Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );
mSslErrorWaitCondition.wakeOne();
}
#endif

void QgsNetworkAccessManager::abortRequest()
{
QTimer *timer = qobject_cast<QTimer *>( sender() );
Expand Down Expand Up @@ -434,16 +427,18 @@ void QgsNetworkAccessManager::onReplySslErrors( const QList<QSslError> &errors )

emit requestEncounteredSslErrors( getRequestId( reply ), errors );

// acquire semaphore a first time, so we block next acquire until release is called
mSslErrorHandlerSemaphore.acquire();

// in main thread this will trigger SSL error handler immediately and return once the errors are handled,
// while in worker thread the signal will be queued (and return immediately) -- hence the need to lock the thread in the next block
emit sslErrorsOccurred( reply, errors );
if ( this != sMainNAM )
{
// lock thread and wait till error is handled. If we return from this slot now, then the reply will resume
// without actually giving the main thread the chance to act on the ssl error and possibly ignore it.
mSslErrorHandlerMutex.lock();
mSslErrorWaitCondition.wait( &mSslErrorHandlerMutex );
mSslErrorHandlerMutex.unlock();
mSslErrorHandlerSemaphore.acquire();
troopa81 marked this conversation as resolved.
Show resolved Hide resolved
mSslErrorHandlerSemaphore.release();
afterSslErrorHandled( reply );
}
}
Expand All @@ -455,11 +450,6 @@ void QgsNetworkAccessManager::afterSslErrorHandled( QNetworkReply *reply )
restartTimeout( reply );
emit sslErrorsHandled( reply );
}
else if ( this == sMainNAM )
{
// notify other threads to allow them to handle the reply
qobject_cast< QgsNetworkAccessManager *>( reply->manager() )->unlockAfterSslErrorHandled(); // safe to call directly - the other thread will be stuck waiting for us
}
}

void QgsNetworkAccessManager::afterAuthRequestHandled( QNetworkReply *reply )
Expand Down Expand Up @@ -505,6 +495,7 @@ void QgsNetworkAccessManager::handleSslErrors( QNetworkReply *reply, const QList
{
mSslErrorHandler->handleSslErrors( reply, errors );
afterSslErrorHandled( reply );
qobject_cast<QgsNetworkAccessManager *>( reply->manager() )->mSslErrorHandlerSemaphore.release();
}

#endif
Expand All @@ -519,7 +510,9 @@ void QgsNetworkAccessManager::onAuthRequired( QNetworkReply *reply, QAuthenticat

emit requestRequiresAuth( getRequestId( reply ), auth->realm() );

// acquire semaphore a first time, so we block next acquire until release is called
mAuthRequestHandlerSemaphore.acquire();

// in main thread this will trigger auth handler immediately and return once the request is satisfied,
// while in worker thread the signal will be queued (and return immediately) -- hence the need to lock the thread in the next block
emit authRequestOccurred( reply, auth );
Expand Down
7 changes: 2 additions & 5 deletions src/core/network/qgsnetworkaccessmanager.h
View file
Open in desktop
Expand Up @@ -852,7 +852,6 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager

private:
#ifndef QT_NO_SSL
void unlockAfterSslErrorHandled();
void afterSslErrorHandled( QNetworkReply *reply );
#endif

Expand All @@ -872,10 +871,8 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
static QgsNetworkAccessManager *sMainNAM;
// ssl error handler, will be set for main thread ONLY
std::unique_ptr< QgsSslErrorHandler > mSslErrorHandler;
// only in use by worker threads, unused in main thread
QMutex mSslErrorHandlerMutex;
// only in use by worker threads, unused in main thread
QWaitCondition mSslErrorWaitCondition;
// Used by worker threads to wait for ssl error handler run in main thread
QSemaphore mSslErrorHandlerSemaphore;

// auth request handler, will be set for main thread ONLY
std::unique_ptr< QgsNetworkAuthenticationHandler > mAuthHandler;
Expand Down