Skip to content

Commit

Permalink
Make SSL error handling thread safe, avoid races and crashes when
Browse files Browse the repository at this point in the history
SSL error occurs in non-main thread

This commit reworks how SSL errors are handled in QGIS. Previously,
ssl errors emitted by non-main thread QgsNetworkAccessManager instances
were handled by the main thread in QgisApp on next event loop. But
app (in the main thread) tried to pause the timeout timer for the
QNetworkReply containing the error, which doesn't work, because you
can't stop a timer in the main thread for a timer object created
in another thread. This meant that the reply could get deleted in
the background thread while the main thread was still using it
and waiting for users to respond to the SSL error. Now the timer
handling is done in the background thread's network access manager
instance, so in the thread matching the reply's timeout timer. We
then have to do some fancy thread locking using QWaitCondition,
because we need to "pause" that thread until the SSL error
is handled in the main thread -- if we don't pause the background
thread, Qt immediately resumes the QNetworkReply without ever
giving us the change to ignore ssl errors in the reply. Phew!

Additionally, the previous approach had a shortcoming in that
there was no way to notify background threads that ssl errors
had actually be handled. To do this we need a new signal which
can be fired after app has shown the ssl dialog and given users
a chance to respond. BUT... we can't safely do this -- if we
add a method to notify background threads when ssl errors have
been handled, then it CANNOT safely reside in app -- doing so
would break for QGIS server and QField etc, where theres no
app instance around to provide this notification. As a result
I've abstracted out the ssl error handling. By default there's
a simple error handler which just logs errors and returns
without ignoring them (i.e. default Qt behavior, an ssl error
cancels the request). App has a specific subclass of the ssl
error handler which presents the nice dialog and asks users to
choose what to do. Potentially server could decide to make
its own subclass too, which could e.g. ignore SSL warnings
if an environment variable is present (but at the moment
the behavior is effectively unchanged for server).

The end result is that SSL error handling should now be
totally thread safe, and we shouldn't hit any more deadlocks
and crashes when ssl errors occur in background threads.

I've also taken the opportunity to add a new signal which
is always emitted by the main thread QgsNetworkAccessManager
instance, which is emitted whenever ANY request on any
thread encounters an SSL error, and contains the requestId
so that the ssl error can be linked back to the originating
request. This is for debugging, and for use by the
network monitoring plugin to show ssl errors encountered
by requests.
  • Loading branch information
nyalldawson committed Jan 28, 2019
1 parent c44f8f0 commit 2e3a717
Show file tree
Hide file tree
Showing 8 changed files with 366 additions and 91 deletions.
21 changes: 21 additions & 0 deletions python/core/auto_generated/qgsnetworkaccessmanager.sip.in
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ QgsNetworkRequestParameters.AttributeInitiatorRequestId attribute set.

};




class QgsNetworkAccessManager : QNetworkAccessManager
{
%Docstring
Expand Down Expand Up @@ -141,6 +144,7 @@ need to be handled on the main thread. See in-depth discussion below.

QgsNetworkAccessManager( QObject *parent = 0 );


void insertProxyFactory( QNetworkProxyFactory *factory /Transfer/ );
%Docstring
insert a factory into the proxy factories list
Expand Down Expand Up @@ -267,6 +271,22 @@ created in any thread.
.. versionadded:: 3.6
%End


void requestEncounteredSslErrors( int requestId, const QList<QSslError> &errors );
%Docstring
Emitted when a network request encounters SSL ``errors``.

The ``requestId`` argument reflects the unique ID identifying the original request which the progress report relates to.

This signal is propagated to the main thread QgsNetworkAccessManager instance, so it is necessary
only to connect to the main thread's signal in order to receive notifications about SSL errors
from any thread.

.. versionadded:: 3.6
%End



void requestCreated( QNetworkReply * ) /Deprecated/;
%Docstring

Expand All @@ -275,6 +295,7 @@ created in any thread.

void requestTimedOut( QNetworkReply * );


protected:
virtual QNetworkReply *createRequest( QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *outgoingData = 0 );

Expand Down
2 changes: 2 additions & 0 deletions src/app/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ SET(QGIS_APP_SRCS
qgsappscreenshots.cpp
qgsjoindialog.cpp
qgsannotationwidget.cpp
qgsappsslerrorhandler.cpp
qgsattributeactiondialog.cpp
qgsattributeactionpropertiesdialog.cpp
qgsattributetypedialog.cpp
Expand Down Expand Up @@ -251,6 +252,7 @@ SET (QGIS_APP_MOC_HDRS
qgsalignrasterdialog.h
qgsappbrowserproviders.h
qgsappscreenshots.h
qgsappsslerrorhandler.h
qgsjoindialog.h
qgsaddtaborgroup.h
qgsannotationwidget.h
Expand Down
87 changes: 2 additions & 85 deletions src/app/qgisapp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ Q_GUI_EXPORT extern int qt_defaultDpiX();
#include "qgsappbrowserproviders.h"
#include "qgsapplayertreeviewmenuprovider.h"
#include "qgsapplication.h"
#include "qgsappsslerrorhandler.h"
#include "qgsactionmanager.h"
#include "qgsannotationmanager.h"
#include "qgsannotationregistry.h"
Expand Down Expand Up @@ -13826,8 +13827,7 @@ void QgisApp::namSetup()
this, &QgisApp::namRequestTimedOut );

#ifndef QT_NO_SSL
connect( nam, &QNetworkAccessManager::sslErrors,
this, &QgisApp::namSslErrors );
nam->setSslErrorHandler( qgis::make_unique<QgsAppSslErrorHandler>() );
#endif
}

Expand Down Expand Up @@ -13947,89 +13947,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
auth->setPassword( password );
}

#ifndef QT_NO_SSL
void QgisApp::namSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
{
// stop the timeout timer, or app crashes if the user (or slot) takes longer than
// singleshot timeout and tries to update the closed QNetworkReply
QTimer *timer = reply->findChild<QTimer *>( QStringLiteral( "timeoutTimer" ) );
if ( timer )
{
QgsDebugMsg( QStringLiteral( "Stopping network reply timeout" ) );
timer->stop();
}

QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );

QString hostport( QStringLiteral( "%1:%2" )
.arg( reply->url().host() )
.arg( reply->url().port() != -1 ? reply->url().port() : 443 )
.trimmed() );
QString digest( QgsAuthCertUtils::shaHexForCert( reply->sslConfiguration().peerCertificate() ) );
QString dgsthostport( QStringLiteral( "%1:%2" ).arg( digest, hostport ) );

const QHash<QString, QSet<QSslError::SslError> > &errscache( QgsApplication::authManager()->ignoredSslErrorCache() );

if ( errscache.contains( dgsthostport ) )
{
QgsDebugMsg( QStringLiteral( "Ignored SSL errors cahced item found, ignoring errors if they match for %1" ).arg( hostport ) );
const QSet<QSslError::SslError> &errenums( errscache.value( dgsthostport ) );
bool ignore = !errenums.isEmpty();
int errmatched = 0;
if ( ignore )
{
Q_FOREACH ( const QSslError &error, errors )
{
if ( error.error() == QSslError::NoError )
continue;

bool errmatch = errenums.contains( error.error() );
ignore = ignore && errmatch;
errmatched += errmatch ? 1 : 0;
}
}

if ( ignore && errenums.size() == errmatched )
{
QgsDebugMsg( QStringLiteral( "Errors matched cached item's, ignoring all for %1" ).arg( hostport ) );
reply->ignoreSslErrors();
return;
}

QgsDebugMsg( QStringLiteral( "Errors %1 for cached item for %2" )
.arg( errenums.isEmpty() ? QStringLiteral( "not found" ) : QStringLiteral( "did not match" ),
hostport ) );
}


QgsAuthSslErrorsDialog *dlg = new QgsAuthSslErrorsDialog( reply, errors, this, digest, hostport );
dlg->setWindowModality( Qt::ApplicationModal );
dlg->resize( 580, 512 );
if ( dlg->exec() )
{
if ( reply )
{
QgsDebugMsg( QStringLiteral( "All SSL errors ignored for %1" ).arg( hostport ) );
reply->ignoreSslErrors();
}
}
dlg->deleteLater();

// restart network request timeout timer
if ( reply )
{
QgsSettings s;
QTimer *timer = reply->findChild<QTimer *>( QStringLiteral( "timeoutTimer" ) );
if ( timer )
{
QgsDebugMsg( QStringLiteral( "Restarting network reply timeout" ) );
timer->setSingleShot( true );
timer->start( s.value( QStringLiteral( "/qgis/networkAndProxy/networkTimeout" ), "60000" ).toInt() );
}
}
}
#endif

void QgisApp::namRequestTimedOut( const QgsNetworkRequestParameters &request )
{
QLabel *msgLabel = new QLabel( tr( "Network request to %1 timed out, any data received is likely incomplete." ).arg( request.request().url().toString() ) +
Expand Down
4 changes: 1 addition & 3 deletions src/app/qgisapp.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ class QgsUserProfileManager;
class QgsUserProfileManagerWidgetFactory;
class Qgs3DMapCanvasDockWidget;
class QgsHandleBadLayersHandler;
class QgsNetworkAccessManager;

class QDomDocument;
class QNetworkReply;
Expand Down Expand Up @@ -880,9 +881,6 @@ class APP_EXPORT QgisApp : public QMainWindow, private Ui::MainWindow
//! request credentials for network manager
void namAuthenticationRequired( QNetworkReply *reply, QAuthenticator *auth );
void namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthenticator *auth );
#ifndef QT_NO_SSL
void namSslErrors( QNetworkReply *reply, const QList<QSslError> &errors );
#endif
void namRequestTimedOut( const QgsNetworkRequestParameters &request );

//! Schedule and erase of the authentication database upon confirmation
Expand Down
82 changes: 82 additions & 0 deletions src/app/qgsappsslerrorhandler.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/***************************************************************************
qgsappsslerrorhandler.cpp
---------------------------
begin : December 2018
copyright : (C) 2018 by Nyall Dawson
email : nyall dot dawson at gmail dot com
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/

#include "qgsappsslerrorhandler.h"
#include "qgslogger.h"
#include "qgsauthcertutils.h"
#include "qgsapplication.h"
#include "qgsauthmanager.h"
#include "qgsauthsslerrorsdialog.h"
#include "qgisapp.h"

void QgsAppSslErrorHandler::handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
{
Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );

QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );

QString hostport( QStringLiteral( "%1:%2" )
.arg( reply->url().host() )
.arg( reply->url().port() != -1 ? reply->url().port() : 443 )
.trimmed() );
QString digest( QgsAuthCertUtils::shaHexForCert( reply->sslConfiguration().peerCertificate() ) );
QString dgsthostport( QStringLiteral( "%1:%2" ).arg( digest, hostport ) );

const QHash<QString, QSet<QSslError::SslError> > &errscache( QgsApplication::authManager()->ignoredSslErrorCache() );

if ( errscache.contains( dgsthostport ) )
{
QgsDebugMsg( QStringLiteral( "Ignored SSL errors cached item found, ignoring errors if they match for %1" ).arg( hostport ) );
const QSet<QSslError::SslError> &errenums( errscache.value( dgsthostport ) );
bool ignore = !errenums.isEmpty();
int errmatched = 0;
if ( ignore )
{
for ( const QSslError &error : errors )
{
if ( error.error() == QSslError::NoError )
continue;

bool errmatch = errenums.contains( error.error() );
ignore = ignore && errmatch;
errmatched += errmatch ? 1 : 0;
}
}

if ( ignore && errenums.size() == errmatched )
{
QgsDebugMsg( QStringLiteral( "Errors matched cached item's, ignoring all for %1" ).arg( hostport ) );
reply->ignoreSslErrors();
return;
}

QgsDebugMsg( QStringLiteral( "Errors %1 for cached item for %2" )
.arg( errenums.isEmpty() ? QStringLiteral( "not found" ) : QStringLiteral( "did not match" ),
hostport ) );
}

QgsAuthSslErrorsDialog *dlg = new QgsAuthSslErrorsDialog( reply, errors, QgisApp::instance(), digest, hostport );
dlg->setWindowModality( Qt::ApplicationModal );
dlg->resize( 580, 512 );
if ( dlg->exec() )
{
if ( reply )
{
QgsDebugMsg( QStringLiteral( "All SSL errors ignored for %1" ).arg( hostport ) );
reply->ignoreSslErrors();
}
}
dlg->deleteLater();
}
31 changes: 31 additions & 0 deletions src/app/qgsappsslerrorhandler.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/***************************************************************************
qgsappsslerrorhandler.h
-------------------------
begin : December 2018
copyright : (C) 2018 by Nyall Dawson
email : nyall dot dawson at gmail dot com
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/
#ifndef QGSAPPSSLERRORHANDLER_H
#define QGSAPPSSLERRORHANDLER_H

#include "qgsnetworkaccessmanager.h"

class CORE_EXPORT QgsAppSslErrorHandler : public QgsSslErrorHandler
{
Q_OBJECT

public slots:

void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override;

};


#endif // QGSAPPSSLERRORHANDLER_H
Loading

0 comments on commit 2e3a717

Please sign in to comment.