Proxy: Improved mutex usage in LLProxy. Introduced an LLAtomic member to track the status of the http proxy that can be checked without locking a mutex.

3 files changed, 119 insertions, 93 deletions

diff --git a/indra/llmessage/llproxy.cpp b/indra/llmessage/llproxy.cpp
index 381308fb2a..b224757673 100644
--- a/indra/llmessage/llproxy.cpp
+++ b/indra/llmessage/llproxy.cpp
@@ -1,6 +1,6 @@
 /**
- * @file llsocks5.cpp
- * @brief SOCKS 5 implementation
+ * @file llproxy.cpp
+ * @brief UDP and HTTP proxy communications
  *
  * $LicenseInfo:firstyear=2011&license=viewerlgpl$
  * Second Life Viewer Source Code
@@ -28,7 +28,6 @@
 
 #include "llproxy.h"
 
-#include <algorithm>
 #include <string>
 #include <curl/curl.h>
 
@@ -44,7 +43,6 @@
 // incoming packet just to do a simple bool test. The getter for this
 // member is also static
 bool LLProxy::sUDPProxyEnabled = false;
-bool LLProxy::sHTTPProxyEnabled = false;
 
 // Some helpful TCP functions
 static LLSocket::ptr_t tcp_open_channel(apr_pool_t* pool, LLHost host); // Open a TCP channel to a given host
@@ -52,17 +50,16 @@ static void tcp_close_channel(LLSocket::ptr_t* handle_ptr); // Close an open TCP
 static S32 tcp_handshake(LLSocket::ptr_t handle, char * dataout, apr_size_t outlen, char * datain, apr_size_t maxinlen); // Do a TCP data handshake
 
 LLProxy::LLProxy():
-		mProxyType(LLPROXY_SOCKS),
+		mHTTPProxyEnabled(false),
+		mProxyMutex(0),
 		mUDPProxy(),
 		mTCPProxy(),
+		mPool(gAPRPoolp),
 		mHTTPProxy(),
+		mProxyType(LLPROXY_SOCKS),
 		mAuthMethodSelected(METHOD_NOAUTH),
 		mSocksUsername(),
-		mSocksPassword(),
-		mPool(gAPRPoolp),
-		mSOCKSAuthStrings(),
-		mHTTPProxyAddrStrings(),
-		mProxyMutex(0)
+		mSocksPassword()
 {
 }
 
@@ -70,11 +67,7 @@ LLProxy::~LLProxy()
 {
 	stopSOCKSProxy();
 	sUDPProxyEnabled  = false;
-	sHTTPProxyEnabled = false;
-
-	// Delete c_str versions of the addresses and credentials.
-	for_each(mSOCKSAuthStrings.begin(), mSOCKSAuthStrings.end(), DeletePointerArray());
-	for_each(mHTTPProxyAddrStrings.begin(), mHTTPProxyAddrStrings.end(), DeletePointerArray());
+	mHTTPProxyEnabled = false;
 }
 
 // Perform a SOCKS 5 authentication and UDP association to the proxy
@@ -89,7 +82,7 @@ S32 LLProxy::proxyHandshake(LLHost proxy, U32 message_port)
 
 	socks_auth_request.version     = SOCKS_VERSION;       // SOCKS version 5
 	socks_auth_request.num_methods = 1;                   // Sending 1 method.
-	socks_auth_request.methods     = mAuthMethodSelected; // Send only the selected method.
+	socks_auth_request.methods     = getSelectedAuthMethod(); // Send only the selected method.
 
 	result = tcp_handshake(mProxyControlChannel, (char*)&socks_auth_request, sizeof(socks_auth_request), (char*)&socks_auth_response, sizeof(socks_auth_response));
 	if (result != 0)
@@ -110,13 +103,15 @@ S32 LLProxy::proxyHandshake(LLHost proxy, U32 message_port)
 	if (socks_auth_response.method == METHOD_PASSWORD)
 	{
 		// The server has requested a username/password combination
-		U32 request_size = mSocksUsername.size() + mSocksPassword.size() + 3;
+		std::string socks_username(getSocksUser());
+		std::string socks_password(getSocksPwd());
+		U32 request_size = socks_username.size() + socks_password.size() + 3;
 		char * password_auth = new char[request_size];
 		password_auth[0] = 0x01;
-		password_auth[1] = mSocksUsername.size();
-		memcpy(&password_auth[2], mSocksUsername.c_str(), mSocksUsername.size());
-		password_auth[mSocksUsername.size() + 2] = mSocksPassword.size();
-		memcpy(&password_auth[mSocksUsername.size()+3], mSocksPassword.c_str(), mSocksPassword.size());
+		password_auth[1] = socks_username.size();
+		memcpy(&password_auth[2], socks_username.c_str(), socks_username.size());
+		password_auth[socks_username.size() + 2] = socks_password.size();
+		memcpy(&password_auth[socks_username.size()+3], socks_password.c_str(), socks_password.size());
 
 		authmethod_password_reply_t password_reply;
 
@@ -224,7 +219,7 @@ void LLProxy::stopSOCKSProxy()
 	// then we must shut down any HTTP proxy operations. But it is allowable if web
 	// proxy is being used to continue proxying HTTP.
 
-	if (LLPROXY_SOCKS == mProxyType)
+	if (LLPROXY_SOCKS == getHTTPProxyType())
 	{
 		void disableHTTPProxy();
 	}
@@ -237,6 +232,8 @@ void LLProxy::stopSOCKSProxy()
 
 void LLProxy::setAuthNone()
 {
+	LLMutexLock lock(&mProxyMutex);
+
 	mAuthMethodSelected = METHOD_NOAUTH;
 }
 
@@ -249,23 +246,18 @@ bool LLProxy::setAuthPassword(const std::string &username, const std::string &pa
 		return false;
 	}
 
+	LLMutexLock lock(&mProxyMutex);
+
 	mAuthMethodSelected = METHOD_PASSWORD;
 	mSocksUsername      = username;
 	mSocksPassword      = password;
 
-	U32 size = username.length() + password.length() + 2;
-	char* curl_auth_string  = new char[size];
-	snprintf(curl_auth_string, size, "%s:%s", username.c_str(), password.c_str());
-
-	LLMutexLock lock(&mProxyMutex);
-	mSOCKSAuthStrings.push_back(curl_auth_string);
-
 	return true;
 }
 
 bool LLProxy::enableHTTPProxy(LLHost httpHost, LLHttpProxyType type)
 {
-	if (httpHost.isOk())
+	if (!httpHost.isOk())
 	{
 		LL_WARNS("Proxy") << "Invalid SOCKS 5 Server" << LL_ENDL;
 		return false;
@@ -273,14 +265,10 @@ bool LLProxy::enableHTTPProxy(LLHost httpHost, LLHttpProxyType type)
 
 	LLMutexLock lock(&mProxyMutex);
 
-	sHTTPProxyEnabled = true;
 	mHTTPProxy        = httpHost;
 	mProxyType        = type;
 
-	U32 size = httpHost.getIPString().length() + 1;
-	char* http_addr_string = new char[size];
-	strncpy(http_addr_string, httpHost.getIPString().c_str(), size);
-	mHTTPProxyAddrStrings.push_back(http_addr_string);
+	mHTTPProxyEnabled = true;
 
 	return true;
 }
@@ -289,7 +277,7 @@ bool LLProxy::enableHTTPProxy()
 {
 	LLMutexLock lock(&mProxyMutex);
 
-	sHTTPProxyEnabled = true;
+	mHTTPProxyEnabled = true;
 
 	return true;
 }
@@ -298,7 +286,40 @@ void LLProxy::disableHTTPProxy()
 {
 	LLMutexLock lock(&mProxyMutex);
 
-	sHTTPProxyEnabled = false;
+	mHTTPProxyEnabled = false;
+}
+
+// Get the HTTP proxy address and port
+LLHost LLProxy::getHTTPProxy() const
+{
+	LLMutexLock lock(&mProxyMutex);
+	return mHTTPProxy;
+}
+
+// Get the currently selected HTTP proxy type
+LLHttpProxyType LLProxy::getHTTPProxyType() const
+{
+	LLMutexLock lock(&mProxyMutex);
+	return mProxyType;
+}
+
+std::string LLProxy::getSocksPwd() const
+{
+	LLMutexLock lock(&mProxyMutex);
+	return mSocksPassword;
+}
+
+std::string LLProxy::getSocksUser() const
+{
+	LLMutexLock lock(&mProxyMutex);
+	return mSocksUsername;
+}
+
+// get the currently selected auth method
+LLSocks5AuthType LLProxy::getSelectedAuthMethod() const
+{
+	LLMutexLock lock(&mProxyMutex);
+	return mAuthMethodSelected;
 }
 
 //static
@@ -322,24 +343,30 @@ void LLProxy::applyProxySettings(LLCurl::Easy* handle)
 
 void LLProxy::applyProxySettings(CURL* handle)
 {
-	LLMutexLock lock(&mProxyMutex);
-	if (sHTTPProxyEnabled)
+	// Do a faster unlocked check to see if we are supposed to proxy.
+	if (mHTTPProxyEnabled)
 	{
-		check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXY, mHTTPProxyAddrStrings.back()));
-		U16 port = mHTTPProxy.getPort();
-		check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, port));
-
-		if (mProxyType == LLPROXY_SOCKS)
+		// We think we should proxy, lock the proxy mutex.
+		LLMutexLock lock(&mProxyMutex);
+		// Now test again to verify that the proxy wasn't disabled between the first check and the lock.
+		if (mHTTPProxyEnabled)
 		{
-			check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5));
-			if (mAuthMethodSelected == METHOD_PASSWORD)
+			check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXY, mHTTPProxy.getIPString().c_str()));
+			check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, mHTTPProxy.getPort()));
+
+			if (mProxyType == LLPROXY_SOCKS)
 			{
-				check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYUSERPWD, mSOCKSAuthStrings.back()));
+				check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5));
+				if (mAuthMethodSelected == METHOD_PASSWORD)
+				{
+					std::string auth_string = mSocksUsername + ":" + mSocksPassword;
+					check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYUSERPWD, auth_string.c_str()));
+				}
+			}
+			else
+			{
+				check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP));
 			}
-		}
-		else
-		{
-			check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP));
 		}
 	}
 }
diff --git a/indra/llmessage/llproxy.h b/indra/llmessage/llproxy.h
index ce5bdec5ad..22954f2733 100644
--- a/indra/llmessage/llproxy.h
+++ b/indra/llmessage/llproxy.h
@@ -1,6 +1,6 @@
 /**
- * @file llsocks5.h
- * @brief Socks 5 implementation
+ * @file llproxy.h
+ * @brief UDP and HTTP proxy communications
  *
  * $LicenseInfo:firstyear=2011&license=viewerlgpl$
  * Second Life Viewer Source Code
@@ -188,13 +188,14 @@ public:
 	void setAuthNone();
 
 	// get the currently selected auth method
-	LLSocks5AuthType getSelectedAuthMethod() const { return mAuthMethodSelected; }
+	LLSocks5AuthType getSelectedAuthMethod() const;
 
 	// static check for enabled status for UDP packets
 	static bool isEnabled() { return sUDPProxyEnabled; }
 
-	// static check for enabled status for http packets
-	static bool isHTTPProxyEnabled() { return sHTTPProxyEnabled; }
+	// check for enabled status for HTTP packets
+	// mHTTPProxyEnabled is atomic, so no locking is required for thread safety.
+	bool isHTTPProxyEnabled() const { return mHTTPProxyEnabled; }
 
 	// Proxy HTTP packets via httpHost, which can be a SOCKS 5 or a HTTP proxy
 	// as specified in type
@@ -204,6 +205,11 @@ public:
 	// Stop proxying HTTP packets
 	void disableHTTPProxy();
 
+	// Apply the current proxy settings to a curl request. Doesn't do anything if mHTTPProxyEnabled is false.
+	void applyProxySettings(CURL* handle);
+	void applyProxySettings(LLCurl::Easy* handle);
+	void applyProxySettings(LLCurlEasyRequest* handle);
+
 	// Get the UDP proxy address and port
 	LLHost getUDPProxy() const { return mUDPProxy; }
 
@@ -211,46 +217,51 @@ public:
 	LLHost getTCPProxy() const { return mTCPProxy; }
 
 	// Get the HTTP proxy address and port
-	LLHost getHTTPProxy() const { return mHTTPProxy; }
+	LLHost getHTTPProxy() const;
 
 	// Get the currently selected HTTP proxy type
-	LLHttpProxyType getHTTPProxyType() const { return mProxyType; }
+	LLHttpProxyType getHTTPProxyType() const;
 
-	// Get the username password in a curl compatible format
-	std::string getProxyUserPwdCURL() const { return (mSocksUsername + ":" + mSocksPassword); }
-
-	std::string getSocksPwd() const { return mSocksPassword; }
-	std::string getSocksUser() const { return mSocksUsername; }
-
-	// Apply the current proxy settings to a curl request. Doesn't do anything if sHTTPProxyEnabled is false.
-	void applyProxySettings(CURL* handle);
-	void applyProxySettings(LLCurl::Easy* handle);
-	void applyProxySettings(LLCurlEasyRequest* handle);
+	std::string getSocksPwd() const;
+	std::string getSocksUser() const;
 
 private:
 	// Open a communication channel to the SOCKS 5 proxy proxy, at port messagePort
 	S32 proxyHandshake(LLHost proxy, U32 messagePort);
 
 private:
-	// socket handle to proxy TCP control channel
-	LLSocket::ptr_t mProxyControlChannel;
+	// Is the HTTP proxy enabled?
+	// Safe to read in any thread, do not write directly,
+	// use enableHTTPProxy() and disableHTTPProxy() instead.
+	mutable LLAtomic32<bool> mHTTPProxyEnabled;
+
+	// Mutex to protect shared members in non-main thread calls to applyProxySettings()
+	mutable LLMutex mProxyMutex;
+
+	// MEMBERS READ AND WRITTEN ONLY IN THE MAIN THREAD. DO NOT SHARE!
 
 	// Is the UDP proxy enabled?
 	static bool sUDPProxyEnabled;
-	// Is the HTTP proxy enabled? 
-	// Do not toggle directly, use enableHTTPProxy() and disableHTTPProxy()
-	static bool sHTTPProxyEnabled;
-
-	// currently selected http proxy type
-	LLHttpProxyType mProxyType;
 
 	// UDP proxy address and port
 	LLHost mUDPProxy;
 	// TCP proxy control channel address and port
 	LLHost mTCPProxy;
+
+	// socket handle to proxy TCP control channel
+	LLSocket::ptr_t mProxyControlChannel;
+
+	// APR pool for the socket
+	apr_pool_t* mPool;
+
+	// MEMBERS WRITTEN IN MAIN THREAD AND READ IN ANY THREAD. ONLY READ OR WRITE AFTER LOCKING mProxyMutex!
+
 	// HTTP proxy address and port
 	LLHost mHTTPProxy;
 
+	// Currently selected HTTP proxy type. Can be web or socks.
+	LLHttpProxyType mProxyType;
+
 	// SOCKS 5 auth method selected
 	LLSocks5AuthType mAuthMethodSelected;
 
@@ -258,17 +269,6 @@ private:
 	std::string mSocksUsername;
 	// SOCKS 5 password
 	std::string mSocksPassword;
-
-	// Vectors to store valid pointers to string options that might have been set on CURL requests.
-	// This results in a behavior similar to LLCurl::Easy::setoptstring()
-	std::vector<char*> mSOCKSAuthStrings;
-	std::vector<char*> mHTTPProxyAddrStrings;
-
-	// Mutex to protect members in cross-thread calls to applyProxySettings()
-	LLMutex mProxyMutex;
-
-	// APR pool for the socket
-	apr_pool_t* mPool;
 };
 
 #endif
diff --git a/indra/newview/llstartup.cpp b/indra/newview/llstartup.cpp
index c934ff9f1b..3ec5842e3c 100644
--- a/indra/newview/llstartup.cpp
+++ b/indra/newview/llstartup.cpp
@@ -2785,6 +2785,7 @@ bool LLStartUp::handleSocksProxy()
 			subs["HOST"] = http_host.getIPString();
 			subs["PORT"] = (S32)http_host.getPort();
 			LLNotificationsUtil::add("PROXY_INVALID_HTTP_HOST", subs);
+			return false;
 		}
 	}
 	else if ((httpProxyType.compare("Socks") == 0) && gSavedSettings.getBOOL("Socks5ProxyEnabled"))
@@ -2798,9 +2799,8 @@ bool LLStartUp::handleSocksProxy()
 			subs["HOST"] = socks_host.getIPString();
 			subs["PORT"] = (S32)socks_host.getPort();
 			LLNotificationsUtil::add("PROXY_INVALID_SOCKS_HOST", subs);
+			return false;
 		}
-
-
 	}
 	else if (httpProxyType.compare("None") == 0)
 	{
@@ -2816,7 +2816,6 @@ bool LLStartUp::handleSocksProxy()
 	// Set up SOCKS proxy (if needed)
 	if (gSavedSettings.getBOOL("Socks5ProxyEnabled"))
 	{	
-
 		// Determine and update LLProxy with the saved authentication system
 		std::string auth_type = gSavedSettings.getString("Socks5AuthType");
 
@@ -2826,13 +2825,13 @@ bool LLStartUp::handleSocksProxy()
 			std::string socks_user = socks_cred->getIdentifier()["username"].asString();
 			std::string socks_password = socks_cred->getAuthenticator()["creds"].asString();
 
-			bool ok;
-			ok = LLProxy::getInstance()->setAuthPassword(socks_user, socks_password);
+			bool ok = LLProxy::getInstance()->setAuthPassword(socks_user, socks_password);
+
 			if (!ok)
 			{
 				LLNotificationsUtil::add("SOCKS_BAD_CREDS");
+				return false;
 			}
-
 		}
 		else if (auth_type.compare("None") == 0)
 		{