Skip to content

Commit 2e3a717

Browse files
nyalldawsonnyalldawson
committedJan 28, 2019
Make SSL error handling thread safe, avoid races and crashes when
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.
1 parent c44f8f0 commit 2e3a717

File tree

8 files changed

+366
-91
lines changed

8 files changed

+366
-91
lines changed
 

‎python/core/auto_generated/qgsnetworkaccessmanager.sip.in

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,9 @@ QgsNetworkRequestParameters.AttributeInitiatorRequestId attribute set.
9696

9797
};
9898

99+
100+
101+
99102
class QgsNetworkAccessManager : QNetworkAccessManager
100103
{
101104
%Docstring
@@ -141,6 +144,7 @@ need to be handled on the main thread. See in-depth discussion below.
141144

142145
QgsNetworkAccessManager( QObject *parent = 0 );
143146

147+
144148
void insertProxyFactory( QNetworkProxyFactory *factory /Transfer/ );
145149
%Docstring
146150
insert a factory into the proxy factories list
@@ -267,6 +271,22 @@ created in any thread.
267271
.. versionadded:: 3.6
268272
%End
269273

274+
275+
void requestEncounteredSslErrors( int requestId, const QList<QSslError> &errors );
276+
%Docstring
277+
Emitted when a network request encounters SSL ``errors``.
278+
279+
The ``requestId`` argument reflects the unique ID identifying the original request which the progress report relates to.
280+
281+
This signal is propagated to the main thread QgsNetworkAccessManager instance, so it is necessary
282+
only to connect to the main thread's signal in order to receive notifications about SSL errors
283+
from any thread.
284+
285+
.. versionadded:: 3.6
286+
%End
287+
288+
289+
270290
void requestCreated( QNetworkReply * ) /Deprecated/;
271291
%Docstring
272292

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

276296
void requestTimedOut( QNetworkReply * );
277297

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

‎src/app/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ SET(QGIS_APP_SRCS
1616
qgsappscreenshots.cpp
1717
qgsjoindialog.cpp
1818
qgsannotationwidget.cpp
19+
qgsappsslerrorhandler.cpp
1920
qgsattributeactiondialog.cpp
2021
qgsattributeactionpropertiesdialog.cpp
2122
qgsattributetypedialog.cpp
@@ -251,6 +252,7 @@ SET (QGIS_APP_MOC_HDRS
251252
qgsalignrasterdialog.h
252253
qgsappbrowserproviders.h
253254
qgsappscreenshots.h
255+
qgsappsslerrorhandler.h
254256
qgsjoindialog.h
255257
qgsaddtaborgroup.h
256258
qgsannotationwidget.h

‎src/app/qgisapp.cpp

Lines changed: 2 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ Q_GUI_EXPORT extern int qt_defaultDpiX();
145145
#include "qgsappbrowserproviders.h"
146146
#include "qgsapplayertreeviewmenuprovider.h"
147147
#include "qgsapplication.h"
148+
#include "qgsappsslerrorhandler.h"
148149
#include "qgsactionmanager.h"
149150
#include "qgsannotationmanager.h"
150151
#include "qgsannotationregistry.h"
@@ -13826,8 +13827,7 @@ void QgisApp::namSetup()
1382613827
this, &QgisApp::namRequestTimedOut );
1382713828

1382813829
#ifndef QT_NO_SSL
13829-
connect( nam, &QNetworkAccessManager::sslErrors,
13830-
this, &QgisApp::namSslErrors );
13830+
nam->setSslErrorHandler( qgis::make_unique<QgsAppSslErrorHandler>() );
1383113831
#endif
1383213832
}
1383313833

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

13950-
#ifndef QT_NO_SSL
13951-
void QgisApp::namSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
13952-
{
13953-
// stop the timeout timer, or app crashes if the user (or slot) takes longer than
13954-
// singleshot timeout and tries to update the closed QNetworkReply
13955-
QTimer *timer = reply->findChild<QTimer *>( QStringLiteral( "timeoutTimer" ) );
13956-
if ( timer )
13957-
{
13958-
QgsDebugMsg( QStringLiteral( "Stopping network reply timeout" ) );
13959-
timer->stop();
13960-
}
13961-
13962-
QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );
13963-
13964-
QString hostport( QStringLiteral( "%1:%2" )
13965-
.arg( reply->url().host() )
13966-
.arg( reply->url().port() != -1 ? reply->url().port() : 443 )
13967-
.trimmed() );
13968-
QString digest( QgsAuthCertUtils::shaHexForCert( reply->sslConfiguration().peerCertificate() ) );
13969-
QString dgsthostport( QStringLiteral( "%1:%2" ).arg( digest, hostport ) );
13970-
13971-
const QHash<QString, QSet<QSslError::SslError> > &errscache( QgsApplication::authManager()->ignoredSslErrorCache() );
13972-
13973-
if ( errscache.contains( dgsthostport ) )
13974-
{
13975-
QgsDebugMsg( QStringLiteral( "Ignored SSL errors cahced item found, ignoring errors if they match for %1" ).arg( hostport ) );
13976-
const QSet<QSslError::SslError> &errenums( errscache.value( dgsthostport ) );
13977-
bool ignore = !errenums.isEmpty();
13978-
int errmatched = 0;
13979-
if ( ignore )
13980-
{
13981-
Q_FOREACH ( const QSslError &error, errors )
13982-
{
13983-
if ( error.error() == QSslError::NoError )
13984-
continue;
13985-
13986-
bool errmatch = errenums.contains( error.error() );
13987-
ignore = ignore && errmatch;
13988-
errmatched += errmatch ? 1 : 0;
13989-
}
13990-
}
13991-
13992-
if ( ignore && errenums.size() == errmatched )
13993-
{
13994-
QgsDebugMsg( QStringLiteral( "Errors matched cached item's, ignoring all for %1" ).arg( hostport ) );
13995-
reply->ignoreSslErrors();
13996-
return;
13997-
}
13998-
13999-
QgsDebugMsg( QStringLiteral( "Errors %1 for cached item for %2" )
14000-
.arg( errenums.isEmpty() ? QStringLiteral( "not found" ) : QStringLiteral( "did not match" ),
14001-
hostport ) );
14002-
}
14003-
14004-
14005-
QgsAuthSslErrorsDialog *dlg = new QgsAuthSslErrorsDialog( reply, errors, this, digest, hostport );
14006-
dlg->setWindowModality( Qt::ApplicationModal );
14007-
dlg->resize( 580, 512 );
14008-
if ( dlg->exec() )
14009-
{
14010-
if ( reply )
14011-
{
14012-
QgsDebugMsg( QStringLiteral( "All SSL errors ignored for %1" ).arg( hostport ) );
14013-
reply->ignoreSslErrors();
14014-
}
14015-
}
14016-
dlg->deleteLater();
14017-
14018-
// restart network request timeout timer
14019-
if ( reply )
14020-
{
14021-
QgsSettings s;
14022-
QTimer *timer = reply->findChild<QTimer *>( QStringLiteral( "timeoutTimer" ) );
14023-
if ( timer )
14024-
{
14025-
QgsDebugMsg( QStringLiteral( "Restarting network reply timeout" ) );
14026-
timer->setSingleShot( true );
14027-
timer->start( s.value( QStringLiteral( "/qgis/networkAndProxy/networkTimeout" ), "60000" ).toInt() );
14028-
}
14029-
}
14030-
}
14031-
#endif
14032-
1403313950
void QgisApp::namRequestTimedOut( const QgsNetworkRequestParameters &request )
1403413951
{
1403513952
QLabel *msgLabel = new QLabel( tr( "Network request to %1 timed out, any data received is likely incomplete." ).arg( request.request().url().toString() ) +

‎src/app/qgisapp.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ class QgsUserProfileManager;
107107
class QgsUserProfileManagerWidgetFactory;
108108
class Qgs3DMapCanvasDockWidget;
109109
class QgsHandleBadLayersHandler;
110+
class QgsNetworkAccessManager;
110111

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

888886
//! Schedule and erase of the authentication database upon confirmation

‎src/app/qgsappsslerrorhandler.cpp

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/***************************************************************************
2+
qgsappsslerrorhandler.cpp
3+
---------------------------
4+
begin : December 2018
5+
copyright : (C) 2018 by Nyall Dawson
6+
email : nyall dot dawson at gmail dot com
7+
***************************************************************************
8+
* *
9+
* This program is free software; you can redistribute it and/or modify *
10+
* it under the terms of the GNU General Public License as published by *
11+
* the Free Software Foundation; either version 2 of the License, or *
12+
* (at your option) any later version. *
13+
* *
14+
***************************************************************************/
15+
16+
#include "qgsappsslerrorhandler.h"
17+
#include "qgslogger.h"
18+
#include "qgsauthcertutils.h"
19+
#include "qgsapplication.h"
20+
#include "qgsauthmanager.h"
21+
#include "qgsauthsslerrorsdialog.h"
22+
#include "qgisapp.h"
23+
24+
void QgsAppSslErrorHandler::handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
25+
{
26+
Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );
27+
28+
QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );
29+
30+
QString hostport( QStringLiteral( "%1:%2" )
31+
.arg( reply->url().host() )
32+
.arg( reply->url().port() != -1 ? reply->url().port() : 443 )
33+
.trimmed() );
34+
QString digest( QgsAuthCertUtils::shaHexForCert( reply->sslConfiguration().peerCertificate() ) );
35+
QString dgsthostport( QStringLiteral( "%1:%2" ).arg( digest, hostport ) );
36+
37+
const QHash<QString, QSet<QSslError::SslError> > &errscache( QgsApplication::authManager()->ignoredSslErrorCache() );
38+
39+
if ( errscache.contains( dgsthostport ) )
40+
{
41+
QgsDebugMsg( QStringLiteral( "Ignored SSL errors cached item found, ignoring errors if they match for %1" ).arg( hostport ) );
42+
const QSet<QSslError::SslError> &errenums( errscache.value( dgsthostport ) );
43+
bool ignore = !errenums.isEmpty();
44+
int errmatched = 0;
45+
if ( ignore )
46+
{
47+
for ( const QSslError &error : errors )
48+
{
49+
if ( error.error() == QSslError::NoError )
50+
continue;
51+
52+
bool errmatch = errenums.contains( error.error() );
53+
ignore = ignore && errmatch;
54+
errmatched += errmatch ? 1 : 0;
55+
}
56+
}
57+
58+
if ( ignore && errenums.size() == errmatched )
59+
{
60+
QgsDebugMsg( QStringLiteral( "Errors matched cached item's, ignoring all for %1" ).arg( hostport ) );
61+
reply->ignoreSslErrors();
62+
return;
63+
}
64+
65+
QgsDebugMsg( QStringLiteral( "Errors %1 for cached item for %2" )
66+
.arg( errenums.isEmpty() ? QStringLiteral( "not found" ) : QStringLiteral( "did not match" ),
67+
hostport ) );
68+
}
69+
70+
QgsAuthSslErrorsDialog *dlg = new QgsAuthSslErrorsDialog( reply, errors, QgisApp::instance(), digest, hostport );
71+
dlg->setWindowModality( Qt::ApplicationModal );
72+
dlg->resize( 580, 512 );
73+
if ( dlg->exec() )
74+
{
75+
if ( reply )
76+
{
77+
QgsDebugMsg( QStringLiteral( "All SSL errors ignored for %1" ).arg( hostport ) );
78+
reply->ignoreSslErrors();
79+
}
80+
}
81+
dlg->deleteLater();
82+
}

‎src/app/qgsappsslerrorhandler.h

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/***************************************************************************
2+
qgsappsslerrorhandler.h
3+
-------------------------
4+
begin : December 2018
5+
copyright : (C) 2018 by Nyall Dawson
6+
email : nyall dot dawson at gmail dot com
7+
***************************************************************************
8+
* *
9+
* This program is free software; you can redistribute it and/or modify *
10+
* it under the terms of the GNU General Public License as published by *
11+
* the Free Software Foundation; either version 2 of the License, or *
12+
* (at your option) any later version. *
13+
* *
14+
***************************************************************************/
15+
#ifndef QGSAPPSSLERRORHANDLER_H
16+
#define QGSAPPSSLERRORHANDLER_H
17+
18+
#include "qgsnetworkaccessmanager.h"
19+
20+
class CORE_EXPORT QgsAppSslErrorHandler : public QgsSslErrorHandler
21+
{
22+
Q_OBJECT
23+
24+
public slots:
25+
26+
void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override;
27+
28+
};
29+
30+
31+
#endif // QGSAPPSSLERRORHANDLER_H

0 commit comments

Comments
 (0)