Skip to content

Commit

Permalink
Fix #10655 (race condition in QgsCredentials)
Browse files Browse the repository at this point in the history
Example of race condition during rendering:
Threads 1 and 2 call get(), it checks that there are cached credentials.
Thread 1 takes the cached credentials, thread 2 will get no data -> will request credentials in dialog
  • Loading branch information
wonder-sk committed Jun 20, 2014
1 parent 61e934b commit 2a4684a
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 0 deletions.
18 changes: 18 additions & 0 deletions python/core/qgscredentials.sip
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,30 @@ class QgsCredentials
//! retrieves instance
static QgsCredentials *instance();

/**
* 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.
* @note added in 2.4
*/
void lock();
/**
* Unlock the instance after being locked.
* @note added in 2.4
*/
void unlock();

protected:
QgsCredentials();

//! request a password
virtual bool request( QString realm, QString &username /In,Out/, QString &password /In,Out/, QString message = QString::null ) = 0;

//! register instance
void setInstance( QgsCredentials *theInstance );

private:
QgsCredentials( const QgsCredentials& );
};


Expand Down
16 changes: 16 additions & 0 deletions src/core/qgscredentials.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ QgsCredentials *QgsCredentials::instance()
return new QgsCredentialsConsole();
}

QgsCredentials::QgsCredentials()
{
}

QgsCredentials::~QgsCredentials()
{
}
Expand Down Expand Up @@ -70,6 +74,18 @@ void QgsCredentials::put( QString realm, QString username, QString password )
mCredentialCache.insert( realm, QPair<QString, QString>( username, password ) );
}


void QgsCredentials::lock()
{
mMutex.lock();
}

void QgsCredentials::unlock()
{
mMutex.unlock();
}


////////////////////////////////
// QgsCredentialsConsole

Expand Down
20 changes: 20 additions & 0 deletions src/core/qgscredentials.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <QObject>
#include <QPair>
#include <QMap>
#include <QMutex>

/** \ingroup core
* Interface for requesting credentials in QGIS in GUI independent way.
Expand All @@ -45,19 +46,38 @@ class CORE_EXPORT QgsCredentials
//! retrieves instance
static QgsCredentials *instance();

/**
* 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.
* @note added in 2.4
*/
void lock();
/**
* Unlock the instance after being locked.
* @note added in 2.4
*/
void unlock();

protected:
QgsCredentials();

//! request a password
virtual bool request( QString realm, QString &username, QString &password, QString message = QString::null ) = 0;

//! register instance
void setInstance( QgsCredentials *theInstance );

private:
Q_DISABLE_COPY( QgsCredentials )

//! cache for already requested credentials in this session
QMap< QString, QPair<QString, QString> > mCredentialCache;

//! Pointer to the credential instance
static QgsCredentials *smInstance;

QMutex mMutex;
};


Expand Down
4 changes: 4 additions & 0 deletions src/providers/oracle/qgsoracleconn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceURI uri )
if ( !username.isEmpty() )
realm.prepend( username + "@" );

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

while ( !mDatabase.open() )
{
bool ok = QgsCredentials::instance()->get( realm, username, password, mDatabase.lastError().text() );
Expand All @@ -102,6 +104,8 @@ QgsOracleConn::QgsOracleConn( QgsDataSourceURI uri )

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

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

if ( !mDatabase.isOpen() )
Expand Down
4 changes: 4 additions & 0 deletions src/providers/postgres/qgspostgresconn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ QgsPostgresConn::QgsPostgresConn( QString conninfo, bool readOnly, bool shared )
QString username = uri.username();
QString password = uri.password();

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

while ( PQstatus() != CONNECTION_OK )
{
bool ok = QgsCredentials::instance()->get( conninfo, username, password, PQerrorMessage() );
Expand All @@ -201,6 +203,8 @@ QgsPostgresConn::QgsPostgresConn( QString conninfo, bool readOnly, bool shared )

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

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

if ( PQstatus() != CONNECTION_OK )
Expand Down

1 comment on commit 2a4684a

@3nids
Copy link
Member

@3nids 3nids commented on 2a4684a Jun 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will test this on monday!

Please sign in to comment.