Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix hang when WMS credentials requested
Remove responsibility for credentials mutex locking from external
callers and handle appropriate locks internally. This allows the
mutex lock to be much "tighter" and avoids deadlocks when
credentials are requested while an existing credentials dialog
is being shown.

(No mutex is required protecting the credentials dialog itself
as this is ALWAYS shown in the main thread)

Fixes #20826
  • Loading branch information
nyalldawson committed Feb 3, 2019
1 parent 59aea69 commit c9e7616
Show file tree
Hide file tree
Showing 9 changed files with 31 additions and 54 deletions.
12 changes: 6 additions & 6 deletions python/core/auto_generated/qgscredentials.sip.in
Expand Up @@ -41,27 +41,27 @@ signal destroyed() to be notified of the deletion
retrieves instance
%End

void lock();
void lock() /Deprecated/;
%Docstring
Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
it will just prevent other threads to lock the instance and continue the execution. When the class is used
from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.

.. versionadded:: 2.4
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
%End

void unlock();
void unlock() /Deprecated/;
%Docstring
Unlock the instance after being locked.

.. versionadded:: 2.4
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
%End

QMutex *mutex();
QMutex *mutex() /Deprecated/;
%Docstring
Returns pointer to mutex

.. versionadded:: 2.4
.. deprecated:: since QGIS 3.4 - mutex locking is automatically handled
%End

protected:
Expand Down
7 changes: 0 additions & 7 deletions src/app/qgisapp.cpp
Expand Up @@ -1589,10 +1589,6 @@ QgisApp::~QgisApp()
qDeleteAll( mCustomDropHandlers );
qDeleteAll( mCustomLayoutDropHandlers );

// replace gui based network access managers with failing only ones
QgsNetworkAccessManager::instance()->setSslErrorHandler( qgis::make_unique< QgsSslErrorHandler >() );
QgsNetworkAccessManager::instance()->setAuthHandler( qgis::make_unique< QgsNetworkAuthenticationHandler >() );

const QList<QgsMapCanvas *> canvases = mapCanvases();
for ( QgsMapCanvas *canvas : canvases )
{
Expand Down Expand Up @@ -13978,7 +13974,6 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe
bool ok;

{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
ok = QgsCredentials::instance()->get(
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
username, password,
Expand All @@ -13992,15 +13987,13 @@ void QgisApp::namProxyAuthenticationRequired( const QNetworkProxy &proxy, QAuthe

// credentials didn't change - stored ones probably wrong? clear password and retry
{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
QgsCredentials::instance()->put(
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
username, QString() );
}
}

{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
QgsCredentials::instance()->put(
QStringLiteral( "proxy %1:%2 [%3]" ).arg( proxy.hostName() ).arg( proxy.port() ).arg( auth->realm() ),
username, password
Expand Down
33 changes: 11 additions & 22 deletions src/app/qgsappauthrequesthandler.cpp
Expand Up @@ -44,39 +44,28 @@ void QgsAppAuthRequestHandler::handleAuthRequest( QNetworkReply *reply, QAuthent

for ( ;; )
{
bool ok;

{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
ok = QgsCredentials::instance()->get(
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
username, password,
QObject::tr( "Authentication required" ) );
}
bool ok = QgsCredentials::instance()->get(
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
username, password,
QObject::tr( "Authentication required" ) );
if ( !ok )
return;

if ( auth->user() != username || ( password != auth->password() && !password.isNull() ) )
break;

// credentials didn't change - stored ones probably wrong? clear password and retry
{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
QgsCredentials::instance()->put(
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
username, QString() );
}
}

// save credentials
{
QMutexLocker lock( QgsCredentials::instance()->mutex() );
QgsCredentials::instance()->put(
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
username, password
);
username, QString() );
}

// save credentials
QgsCredentials::instance()->put(
QStringLiteral( "%1 at %2" ).arg( auth->realm(), reply->url().host() ),
username, password
);

auth->setUser( username );
auth->setPassword( password );
}
2 changes: 0 additions & 2 deletions src/core/auth/qgsauthmanager.cpp
Expand Up @@ -3193,10 +3193,8 @@ bool QgsAuthManager::masterPasswordInput()
if ( ! ok )
{
QgsCredentials *creds = QgsCredentials::instance();
creds->lock();
pass.clear();
ok = creds->getMasterPassword( pass, masterPasswordHashInDatabase() );
creds->unlock();
}

if ( ok && !pass.isEmpty() && mMasterPass != pass )
Expand Down
4 changes: 4 additions & 0 deletions src/core/qgscredentials.cpp
Expand Up @@ -40,16 +40,19 @@ QgsCredentials *QgsCredentials::instance()

bool QgsCredentials::get( const QString &realm, QString &username, QString &password, const QString &message )
{
QMutexLocker locker( &mMutex );
if ( mCredentialCache.contains( realm ) )
{
QPair<QString, QString> credentials = mCredentialCache.take( realm );
locker.unlock();
username = credentials.first;
password = credentials.second;
QgsDebugMsg( QStringLiteral( "retrieved realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );

if ( !password.isNull() )
return true;
}
locker.unlock();

if ( request( realm, username, password, message ) )
{
Expand All @@ -66,6 +69,7 @@ bool QgsCredentials::get( const QString &realm, QString &username, QString &pass
void QgsCredentials::put( const QString &realm, const QString &username, const QString &password )
{
QgsDebugMsg( QStringLiteral( "inserting realm:%1 username:%2 password:%3" ).arg( realm, username, password ) );
QMutexLocker locker( &mMutex );
mCredentialCache.insert( realm, QPair<QString, QString>( username, password ) );
}

Expand Down
16 changes: 10 additions & 6 deletions src/core/qgscredentials.h
Expand Up @@ -59,21 +59,25 @@ class CORE_EXPORT QgsCredentials
* Lock the instance against access from multiple threads. This does not really lock access to get/put methds,
* it will just prevent other threads to lock the instance and continue the execution. When the class is used
* from non-GUI threads, they should call lock() before the get/put calls to avoid race conditions.
* \since QGIS 2.4
*
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
*/
void lock();
Q_DECL_DEPRECATED void lock() SIP_DEPRECATED;

/**
* Unlock the instance after being locked.
* \since QGIS 2.4
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
*/
void unlock();
Q_DECL_DEPRECATED void unlock() SIP_DEPRECATED;

/**
* Returns pointer to mutex
* \since QGIS 2.4
* \deprecated since QGIS 3.4 - mutex locking is automatically handled
*/
QMutex *mutex() { return &mMutex; }
Q_DECL_DEPRECATED QMutex *mutex() SIP_DEPRECATED
{
return &mMutex;
}

protected:

Expand Down
3 changes: 0 additions & 3 deletions src/providers/db2/qgsdb2provider.cpp
Expand Up @@ -235,7 +235,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
db.setPort( port.toInt() );
bool connected = false;
int i = 0;
QgsCredentials::instance()->lock();
while ( !connected && i < 3 )
{
i++;
Expand All @@ -249,7 +248,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
{
errMsg = QStringLiteral( "Cancel clicked" );
QgsDebugMsg( errMsg );
QgsCredentials::instance()->unlock();
break;
}
}
Expand Down Expand Up @@ -291,7 +289,6 @@ QSqlDatabase QgsDb2Provider::getDatabase( const QString &connInfo, QString &errM
{
QgsCredentials::instance()->put( databaseName, userName, password );
}
QgsCredentials::instance()->unlock();

return db;
}
Expand Down
4 changes: 0 additions & 4 deletions src/providers/oracle/qgsoracleconn.cpp
Expand Up @@ -96,8 +96,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )
QgsDebugMsg( QStringLiteral( "Connecting with options: " ) + options );
if ( !mDatabase.open() )
{
QgsCredentials::instance()->lock();

while ( !mDatabase.open() )
{
bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() );
Expand Down Expand Up @@ -127,8 +125,6 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceUri uri )

if ( mDatabase.isOpen() )
QgsCredentials::instance()->put( realm, username, password );

QgsCredentials::instance()->unlock();
}

if ( !mDatabase.isOpen() )
Expand Down
4 changes: 0 additions & 4 deletions src/providers/postgres/qgspostgresconn.cpp
Expand Up @@ -272,8 +272,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
QString username = uri.username();
QString password = uri.password();

QgsCredentials::instance()->lock();

int i = 0;
while ( PQstatus() != CONNECTION_OK && i < 5 )
{
Expand All @@ -298,8 +296,6 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s

if ( PQstatus() == CONNECTION_OK )
QgsCredentials::instance()->put( conninfo, username, password );

QgsCredentials::instance()->unlock();
}

if ( PQstatus() != CONNECTION_OK )
Expand Down

0 comments on commit c9e7616

Please sign in to comment.