From 6ccbfd6ca498da04e4ef02102c4ded9768225b5a Mon Sep 17 00:00:00 2001 From: Aron Rosenberg Date: Wed, 25 Apr 2012 09:57:43 -0700 Subject: Fix various NTLM/Digest multi-threading and usage issues - Fix multi-threading bug where NTLM/Digest auth would fail when concurrent requests were on the wire. The fix for this is too not internally share QAuthenticationPrivate pointers, since the challange values would get overridden in different threads. This was failing because the internal QAuthenticationPrivate members would have been set with the status/values of the current request which would mess up the state of the new request. As currently implemented, the helper functions inside QAuthenticationPrivate can't call detach to implement proper copy on write symantics. - Fix issue where if user was set via constructor, the NTLM domain parsing would not occur. Parsing of DOMAIN\user is now redone if proxy type is determined to be NTLM. Task-number: QTBUG-15472 Task-number: QTBUG-17322 Task-number: QTBUG-18794 Task-number: QTBUG-13063 Task-number: QTBUG-16585 Change-Id: I8a898c51fb04fab6fb08d96d88dd73be0c87af5d Reviewed-by: Aron Rosenberg Reviewed-by: Shane Kearns --- src/network/kernel/qauthenticator.cpp | 87 +++++++++++++++++++++-------------- src/network/kernel/qauthenticator_p.h | 4 +- 2 files changed, 54 insertions(+), 37 deletions(-) diff --git a/src/network/kernel/qauthenticator.cpp b/src/network/kernel/qauthenticator.cpp index 43b3618ea2..6808ae51ca 100644 --- a/src/network/kernel/qauthenticator.cpp +++ b/src/network/kernel/qauthenticator.cpp @@ -140,7 +140,7 @@ QAuthenticator::QAuthenticator() */ QAuthenticator::~QAuthenticator() { - if (d && !d->ref.deref()) + if (d) delete d; } @@ -148,10 +148,10 @@ QAuthenticator::~QAuthenticator() Constructs a copy of \a other. */ QAuthenticator::QAuthenticator(const QAuthenticator &other) - : d(other.d) + : d(0) { - if (d) - d->ref.ref(); + if (other.d) + *this = other; } /*! @@ -162,12 +162,23 @@ QAuthenticator &QAuthenticator::operator=(const QAuthenticator &other) if (d == other.d) return *this; - if (d && !d->ref.deref()) + // Do not share the d since challange reponse/based changes + // could corrupt the internal store and different network requests + // can utilize different types of proxies. + detach(); + if (other.d) { + d->user = other.d->user; + d->userDomain = other.d->userDomain; + d->workstation = other.d->workstation; + d->extractedUser = other.d->extractedUser; + d->password = other.d->password; + d->realm = other.d->realm; + d->method = other.d->method; + d->options = other.d->options; + } else { delete d; - - d = other.d; - if (d) - d->ref.ref(); + d = 0; + } return *this; } @@ -209,28 +220,8 @@ QString QAuthenticator::user() const void QAuthenticator::setUser(const QString &user) { detach(); - int separatorPosn = 0; - - switch(d->method) { - case QAuthenticatorPrivate::Ntlm: - if((separatorPosn = user.indexOf(QLatin1String("\\"))) != -1) { - //domain name is present - d->realm.clear(); - d->userDomain = user.left(separatorPosn); - d->extractedUser = user.mid(separatorPosn + 1); - d->user = user; - } else { - d->extractedUser = user; - d->user = user; - d->realm.clear(); - d->userDomain.clear(); - } - break; - default: - d->user = user; - d->userDomain.clear(); - break; - } + d->user = user; + d->updateCredentials(); } /*! @@ -259,11 +250,9 @@ void QAuthenticator::detach() { if (!d) { d = new QAuthenticatorPrivate; - d->ref.store(1); return; } - qAtomicDetach(d); d->phase = QAuthenticatorPrivate::Start; } @@ -325,8 +314,7 @@ bool QAuthenticator::isNull() const } QAuthenticatorPrivate::QAuthenticatorPrivate() - : ref(0) - , method(None) + : method(None) , hasFailed(false) , phase(Start) , nonceCount(0) @@ -336,6 +324,33 @@ QAuthenticatorPrivate::QAuthenticatorPrivate() nonceCount = 0; } +QAuthenticatorPrivate::~QAuthenticatorPrivate() +{ +} + +void QAuthenticatorPrivate::updateCredentials() +{ + int separatorPosn = 0; + + switch (method) { + case QAuthenticatorPrivate::Ntlm: + if ((separatorPosn = user.indexOf(QLatin1String("\\"))) != -1) { + //domain name is present + realm.clear(); + userDomain = user.left(separatorPosn); + extractedUser = user.mid(separatorPosn + 1); + } else { + extractedUser = user; + realm.clear(); + userDomain.clear(); + } + break; + default: + userDomain.clear(); + break; + } +} + void QAuthenticatorPrivate::parseHttpResponse(const QList > &values, bool isProxy) { const char *search = isProxy ? "proxy-authenticate" : "www-authenticate"; @@ -369,6 +384,8 @@ void QAuthenticatorPrivate::parseHttpResponse(const QList options = parseDigestAuthenticationChallenge(challenge); diff --git a/src/network/kernel/qauthenticator_p.h b/src/network/kernel/qauthenticator_p.h index a41b31d333..b96c8c13e1 100644 --- a/src/network/kernel/qauthenticator_p.h +++ b/src/network/kernel/qauthenticator_p.h @@ -68,8 +68,8 @@ class Q_AUTOTEST_EXPORT QAuthenticatorPrivate public: enum Method { None, Basic, Plain, Login, Ntlm, CramMd5, DigestMd5 }; QAuthenticatorPrivate(); + ~QAuthenticatorPrivate(); - QAtomicInt ref; QString user; QString extractedUser; QString password; @@ -104,7 +104,7 @@ public: static QHash parseDigestAuthenticationChallenge(const QByteArray &challenge); void parseHttpResponse(const QList >&, bool isProxy); - + void updateCredentials(); }; -- cgit v1.2.3