LLProxy code review fixes.

 * Removed check_curl_code and check_curl_multi_code from the global namespace.
 * Added comments documenting which thread the public methods of LLProxy should be called from.
 * Corrected grammar in LLSingleton.h
 * Fixed a buffer scope problem in llpacketring.cpp.

7 files changed, 110 insertions, 104 deletions

diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h
index 2ce6a9d438..49d99f2cd0 100644
--- a/indra/llcommon/llsingleton.h
+++ b/indra/llcommon/llsingleton.h
@@ -144,7 +144,7 @@ public:
 	 * method to destroy the singleton early can prevent these crashes.
 	 *
 	 * An example where this is needed is for a LLSingleton that has an APR
-	 * object as a member and make APR calls on destruction. The APR system is
+	 * object as a member that makes APR calls on destruction. The APR system is
 	 * shut down explicitly before main() exits. This causes a crash on exit.
 	 * Using this method before the call to apr_terminate() and NOT calling
 	 * getInstance() again will prevent the crash.
diff --git a/indra/llmessage/llcurl.cpp b/indra/llmessage/llcurl.cpp
index 2d456b8f3f..ee85f3bbd7 100644
--- a/indra/llmessage/llcurl.cpp
+++ b/indra/llmessage/llcurl.cpp
@@ -1213,3 +1213,13 @@ void LLCurl::cleanupClass()
 }
 
 const unsigned int LLCurl::MAX_REDIRECTS = 5;
+
+// Provide access to LLCurl free functions outside of llcurl.cpp without polluting the global namespace.
+void LLCurlFF::check_curl_code(CURLcode code)
+{
+	check_curl_code(code);
+}
+void LLCurlFF::check_curl_multi_code(CURLMcode code)
+{
+	check_curl_multi_code(code);
+}
diff --git a/indra/llmessage/llcurl.h b/indra/llmessage/llcurl.h
index 5ab4dc35b9..154dc23edc 100644
--- a/indra/llmessage/llcurl.h
+++ b/indra/llmessage/llcurl.h
@@ -371,7 +371,12 @@ private:
 	bool mResultReturned;
 };
 
-void check_curl_code(CURLcode code);
-void check_curl_multi_code(CURLMcode code);
+// Provide access to LLCurl free functions outside of llcurl.cpp without polluting the global namespace.
+namespace LLCurlFF
+{
+	void check_curl_code(CURLcode code);
+	void check_curl_multi_code(CURLMcode code);
+}
+
 
 #endif // LL_LLCURL_H
diff --git a/indra/llmessage/llpacketring.cpp b/indra/llmessage/llpacketring.cpp
index 7628984de4..fc6e9c5193 100644
--- a/indra/llmessage/llpacketring.cpp
+++ b/indra/llmessage/llpacketring.cpp
@@ -228,13 +228,13 @@ S32 LLPacketRing::receivePacket (S32 socket, char *datap)
 		if (LLProxy::isSOCKSProxyEnabled())
 		{
 			U8 buffer[NET_BUFFER_SIZE + SOCKS_HEADER_SIZE];
-			packet_size = receive_packet(socket, reinterpret_cast<char *>(buffer));
+			packet_size = receive_packet(socket, static_cast<char*>(static_cast<void*>(buffer)));
 			
 			if (packet_size > SOCKS_HEADER_SIZE)
 			{
 				// *FIX We are assuming ATYP is 0x01 (IPv4), not 0x03 (hostname) or 0x04 (IPv6)
 				memcpy(datap, buffer + SOCKS_HEADER_SIZE, packet_size - SOCKS_HEADER_SIZE);
-				proxywrap_t * header = reinterpret_cast<proxywrap_t *>(buffer);
+				proxywrap_t * header = static_cast<proxywrap_t*>(static_cast<void*>(buffer));
 				mLastSender.setAddress(header->addr);
 				mLastSender.setPort(ntohs(header->port));
 
@@ -353,14 +353,20 @@ BOOL LLPacketRing::sendPacketImpl(int h_socket, const char * send_buffer, S32 bu
 		return send_packet(h_socket, send_buffer, buf_size, host.getAddress(), host.getPort());
 	}
 
-	proxywrap_t *socks_header = reinterpret_cast<proxywrap_t *>(&mProxyWrappedSendBuffer);
+	char headered_send_buffer[NET_BUFFER_SIZE + SOCKS_HEADER_SIZE];
+
+	proxywrap_t *socks_header = static_cast<proxywrap_t*>(static_cast<void*>(&headered_send_buffer));
 	socks_header->rsv   = 0;
 	socks_header->addr  = host.getAddress();
 	socks_header->port  = htons(host.getPort());
 	socks_header->atype = ADDRESS_IPV4;
 	socks_header->frag  = 0;
 
-	memcpy(mProxyWrappedSendBuffer + SOCKS_HEADER_SIZE, send_buffer, buf_size);
+	memcpy(headered_send_buffer + SOCKS_HEADER_SIZE, send_buffer, buf_size);
 
-	return send_packet(h_socket, (const char*) mProxyWrappedSendBuffer, buf_size + 10, LLProxy::getInstance()->getUDPProxy().getAddress(), LLProxy::getInstance()->getUDPProxy().getPort());
+	return send_packet(	h_socket,
+						headered_send_buffer,
+						buf_size + SOCKS_HEADER_SIZE,
+						LLProxy::getInstance()->getUDPProxy().getAddress(),
+						LLProxy::getInstance()->getUDPProxy().getPort());
 }
diff --git a/indra/llmessage/llpacketring.h b/indra/llmessage/llpacketring.h
index 7edcc834db..b214271e78 100644
--- a/indra/llmessage/llpacketring.h
+++ b/indra/llmessage/llpacketring.h
@@ -83,9 +83,6 @@ protected:
 	LLHost mLastSender;
 	LLHost mLastReceivingIF;
 
-
-	U8 mProxyWrappedSendBuffer[NET_BUFFER_SIZE + SOCKS_HEADER_SIZE];
-
 private:
 	BOOL sendPacketImpl(int h_socket, const char * send_buffer, S32 buf_size, LLHost host);
 };
diff --git a/indra/llmessage/llproxy.cpp b/indra/llmessage/llproxy.cpp
index 611a40579d..827a66dea6 100644
--- a/indra/llmessage/llproxy.cpp
+++ b/indra/llmessage/llproxy.cpp
@@ -43,7 +43,7 @@
 bool LLProxy::sUDPProxyEnabled = false;
 
 // Some helpful TCP static functions.
-static S32 tcp_blocking_handshake(LLSocket::ptr_t handle, char * dataout, apr_size_t outlen, char * datain, apr_size_t maxinlen); // Do a TCP data handshake
+static apr_status_t tcp_blocking_handshake(LLSocket::ptr_t handle, char * dataout, apr_size_t outlen, char * datain, apr_size_t maxinlen); // Do a TCP data handshake
 static LLSocket::ptr_t tcp_open_channel(apr_pool_t* pool, LLHost host); // Open a TCP channel to a given host
 static void tcp_close_channel(LLSocket::ptr_t* handle_ptr); // Close an open TCP channel
 
@@ -52,7 +52,6 @@ LLProxy::LLProxy():
 		mProxyMutex(0),
 		mUDPProxy(),
 		mTCPProxy(),
-		mPool(gAPRPoolp),
 		mHTTPProxy(),
 		mProxyType(LLPROXY_SOCKS),
 		mAuthMethodSelected(METHOD_NOAUTH),
@@ -64,14 +63,13 @@ LLProxy::LLProxy():
 LLProxy::~LLProxy()
 {
 	stopSOCKSProxy();
-	sUDPProxyEnabled  = false;
-	mHTTPProxyEnabled = false;
+	disableHTTPProxy();
 }
 
 /**
  * @brief Open the SOCKS 5 TCP control channel.
  *
- * Perform a SOCKS 5 authentication and UDP association to the proxy server.
+ * Perform a SOCKS 5 authentication and UDP association with the proxy server.
  *
  * @param proxy The SOCKS 5 server to connect to.
  * @return SOCKS_OK if successful, otherwise a socks error code from llproxy.h.
@@ -84,11 +82,15 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
 	socks_auth_request_t  socks_auth_request;
 	socks_auth_response_t socks_auth_response;
 
-	socks_auth_request.version     = SOCKS_VERSION;       // SOCKS version 5
-	socks_auth_request.num_methods = 1;                   // Sending 1 method.
-	socks_auth_request.methods     = getSelectedAuthMethod(); // Send only the selected method.
+	socks_auth_request.version		= SOCKS_VERSION;				// SOCKS version 5
+	socks_auth_request.num_methods	= 1;							// Sending 1 method.
+	socks_auth_request.methods		= getSelectedAuthMethod();		// Send only the selected method.
 
-	result = tcp_blocking_handshake(mProxyControlChannel, (char*)&socks_auth_request, sizeof(socks_auth_request), (char*)&socks_auth_response, sizeof(socks_auth_response));
+	result = tcp_blocking_handshake(mProxyControlChannel,
+									static_cast<char*>(static_cast<void*>(&socks_auth_request)),
+									sizeof(socks_auth_request),
+									static_cast<char*>(static_cast<void*>(&socks_auth_response)),
+									sizeof(socks_auth_response));
 	if (result != APR_SUCCESS)
 	{
 		LL_WARNS("Proxy") << "SOCKS authentication request failed, error on TCP control channel : " << result << LL_ENDL;
@@ -103,7 +105,7 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
 		return SOCKS_NOT_ACCEPTABLE;
 	}
 
-	// SOCKS 5 USERNAME/PASSWORD authentication
+	/* SOCKS 5 USERNAME/PASSWORD authentication */
 	if (socks_auth_response.method == METHOD_PASSWORD)
 	{
 		// The server has requested a username/password combination
@@ -115,11 +117,15 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
 		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());
+		memcpy(&password_auth[socks_username.size() + 3], socks_password.c_str(), socks_password.size());
 
 		authmethod_password_reply_t password_reply;
 
-		result = tcp_blocking_handshake(mProxyControlChannel, password_auth, request_size, (char*)&password_reply, sizeof(password_reply));
+		result = tcp_blocking_handshake(mProxyControlChannel,
+										password_auth,
+										request_size,
+										static_cast<char*>(static_cast<void*>(&password_reply)),
+										sizeof(password_reply));
 		delete[] password_auth;
 
 		if (result != APR_SUCCESS)
@@ -151,7 +157,11 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
 	// "If the client is not in possession of the information at the time of the UDP ASSOCIATE,
 	//  the client MUST use a port number and address of all zeros. RFC 1928"
 
-	result = tcp_blocking_handshake(mProxyControlChannel, (char*)&connect_request, sizeof(connect_request), (char*)&connect_reply, sizeof(connect_reply));
+	result = tcp_blocking_handshake(mProxyControlChannel,
+									static_cast<char*>(static_cast<void*>(&connect_request)),
+									sizeof(connect_request),
+									static_cast<char*>(static_cast<void*>(&connect_reply)),
+									sizeof(connect_reply));
 	if (result != APR_SUCCESS)
 	{
 		LL_WARNS("Proxy") << "SOCKS connect request failed, error on TCP control channel : " << result << LL_ENDL;
@@ -170,6 +180,8 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
 	mUDPProxy.setAddress(proxy.getAddress());
 	// The connection was successful. We now have the UDP port to send requests that need forwarding to.
 	LL_INFOS("Proxy") << "SOCKS 5 UDP proxy connected on " << mUDPProxy << LL_ENDL;
+	sUDPProxyEnabled = true;
+
 	return SOCKS_OK;
 }
 
@@ -177,7 +189,8 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
  * @brief Initiates a SOCKS 5 proxy session.
  *
  * Performs basic checks on host to verify that it is a valid address. Opens the control channel
- * and then negotiates the proxy connection with the server.
+ * and then negotiates the proxy connection with the server. Closes any existing SOCKS
+ * connection before proceeding. Also disables an HTTP proxy if it is using SOCKS as the proxy.
  *
  *
  * @param host Socks server to connect to.
@@ -185,45 +198,32 @@ S32 LLProxy::proxyHandshake(LLHost proxy)
  */
 S32 LLProxy::startSOCKSProxy(LLHost host)
 {
-	S32 status = SOCKS_OK;
-
 	if (host.isOk())
 	{
 		mTCPProxy = host;
 	}
 	else
 	{
-		status = SOCKS_INVALID_HOST;
+		return SOCKS_INVALID_HOST;
 	}
 
-	if (mProxyControlChannel && status == SOCKS_OK)
-	{
-		tcp_close_channel(&mProxyControlChannel);
-	}
+	// Close any running SOCKS connection.
+	stopSOCKSProxy();
 
-	if (status == SOCKS_OK)
+	mProxyControlChannel = tcp_open_channel(gAPRPoolp, mTCPProxy);
+	if (!mProxyControlChannel)
 	{
-		mProxyControlChannel = tcp_open_channel(mPool, mTCPProxy);
-		if (!mProxyControlChannel)
-		{
-			status = SOCKS_HOST_CONNECT_FAILED;
-		}
+		return SOCKS_HOST_CONNECT_FAILED;
 	}
 
-	if (status == SOCKS_OK)
-	{
-		status = proxyHandshake(mTCPProxy);
-	}
+	S32 status = proxyHandshake(mTCPProxy);
 
-	if (status == SOCKS_OK)
-	{
-		sUDPProxyEnabled = true;
-	}
-	else
+	if (status != SOCKS_OK)
 	{
 		// Shut down the proxy if any of the above steps failed.
 		stopSOCKSProxy();
 	}
+
 	return status;
 }
 
@@ -244,7 +244,7 @@ void LLProxy::stopSOCKSProxy()
 
 	if (LLPROXY_SOCKS == getHTTPProxyType())
 	{
-		void disableHTTPProxy();
+		disableHTTPProxy();
 	}
 
 	if (mProxyControlChannel)
@@ -353,16 +353,6 @@ void LLProxy::disableHTTPProxy()
 }
 
 /**
- * @brief Get the HTTP proxy address and port
- */
-//
-LLHost LLProxy::getHTTPProxy() const
-{
-	LLMutexLock lock(&mProxyMutex);
-	return mHTTPProxy;
-}
-
-/**
  * @brief Get the currently selected HTTP proxy type
  */
 LLHttpProxyType LLProxy::getHTTPProxyType() const
@@ -443,21 +433,21 @@ void LLProxy::applyProxySettings(CURL* handle)
 		// 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_PROXY, mHTTPProxy.getIPString().c_str()));
-			check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, mHTTPProxy.getPort()));
+			LLCurlFF::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXY, mHTTPProxy.getIPString().c_str()));
+			LLCurlFF::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, mHTTPProxy.getPort()));
 
 			if (mProxyType == LLPROXY_SOCKS)
 			{
-				check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5));
+				LLCurlFF::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()));
+					LLCurlFF::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));
+				LLCurlFF::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP));
 			}
 		}
 	}
@@ -476,7 +466,7 @@ void LLProxy::applyProxySettings(CURL* handle)
  * @param maxinlen		Maximum possible length of received data.  Short reads are allowed.
  * @return 				Indicates APR status code of exchange. APR_SUCCESS if exchange was successful, -1 if invalid data length was received.
  */
-static S32 tcp_blocking_handshake(LLSocket::ptr_t handle, char * dataout, apr_size_t outlen, char * datain, apr_size_t maxinlen)
+static apr_status_t tcp_blocking_handshake(LLSocket::ptr_t handle, char * dataout, apr_size_t outlen, char * datain, apr_size_t maxinlen)
 {
 	apr_socket_t* apr_socket = handle->getSocket();
 	apr_status_t rv = APR_SUCCESS;
@@ -544,7 +534,7 @@ static LLSocket::ptr_t tcp_open_channel(apr_pool_t* pool, LLHost host)
 /**
  * @brief Close the socket.
  *
- * @param handle_ptr A pointer-to-pointer to avoid increasing the use count.
+ * @param handle_ptr The handle of the socket being closed. A pointer-to-pointer to avoid increasing the use count.
  */
 static void tcp_close_channel(LLSocket::ptr_t* handle_ptr)
 {
diff --git a/indra/llmessage/llproxy.h b/indra/llmessage/llproxy.h
index f8478bda30..a919370540 100644
--- a/indra/llmessage/llproxy.h
+++ b/indra/llmessage/llproxy.h
@@ -226,63 +226,68 @@ public:
 	/*###########################################################################################
 	METHODS THAT DO NOT LOCK mProxyMutex!
 	###########################################################################################*/
+	// Constructor, cannot have parameters due to LLSingleton parent class. Call from main thread only.
 	LLProxy();
 
-	// static check for enabled status for UDP packets
+	// Static check for enabled status for UDP packets. Call from main thread only.
 	static bool isSOCKSProxyEnabled() { return sUDPProxyEnabled; }
 
-	// check for enabled status for HTTP packets
-	// mHTTPProxyEnabled is atomic, so no locking is required for thread safety.
-	bool isHTTPProxyEnabled() const { return mHTTPProxyEnabled; }
-
-	// Get the UDP proxy address and port
+	// Get the UDP proxy address and port. Call from main thread only.
 	LLHost getUDPProxy() const { return mUDPProxy; }
 
-	// Get the SOCKS 5 TCP control channel address and port
-	LLHost getTCPProxy() const { return mTCPProxy; }
-
 	/*###########################################################################################
 	END OF NON-LOCKING METHODS
 	###########################################################################################*/
 
 	/*###########################################################################################
-	METHODS THAT DO LOCK mProxyMutex! DO NOT CALL WHILE mProxyMutex IS LOCKED!
+	METHODS THAT LOCK mProxyMutex! DO NOT CALL WHILE mProxyMutex IS LOCKED!
 	###########################################################################################*/
+	// Destructor, closes open connections. Do not call directly, use cleanupClass().
 	~LLProxy();
 
-	// Start a connection to the SOCKS 5 proxy
+	// Delete LLProxy singleton. Allows the apr_socket used in the SOCKS 5 control channel to be
+	// destroyed before the call to apr_terminate. Call from main thread only.
+	static void cleanupClass();
+
+	// Apply the current proxy settings to a curl request. Doesn't do anything if mHTTPProxyEnabled is false.
+	// Safe to call from any thread.
+	void applyProxySettings(CURL* handle);
+	void applyProxySettings(LLCurl::Easy* handle);
+	void applyProxySettings(LLCurlEasyRequest* handle);
+
+	// Start a connection to the SOCKS 5 proxy. Call from main thread only.
 	S32 startSOCKSProxy(LLHost host);
 
-	// Disconnect and clean up any connection to the SOCKS 5 proxy
+	// Disconnect and clean up any connection to the SOCKS 5 proxy. Call from main thread only.
 	void stopSOCKSProxy();
 
-	// Delete LLProxy singleton, destroying the APR pool used by the control channel.
-	static void cleanupClass();
-
-	// Set up to use Password auth when connecting to the SOCKS proxy
+	// Use Password auth when connecting to the SOCKS proxy. Call from main thread only.
 	bool setAuthPassword(const std::string &username, const std::string &password);
 
-	// Set up to use No Auth when connecting to the SOCKS proxy
+	// Disable authentication when connecting to the SOCKS proxy. Call from main thread only.
 	void setAuthNone();
 
-	// Get the currently selected auth method.
-	LLSocks5AuthType getSelectedAuthMethod() const;
-
-	// Proxy HTTP packets via httpHost, which can be a SOCKS 5 or a HTTP proxy
-	// as specified in type
+	// Proxy HTTP packets via httpHost, which can be a SOCKS 5 or a HTTP proxy.
+	// as specified in type. Call from main thread only.
 	bool enableHTTPProxy(LLHost httpHost, LLHttpProxyType type);
 	bool enableHTTPProxy();
 
-	// Stop proxying HTTP packets
+	// Stop proxying HTTP packets. Call from main thread only.
 	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);
+	/*###########################################################################################
+	END OF LOCKING METHODS
+	###########################################################################################*/
+private:
+	/*###########################################################################################
+	METHODS THAT LOCK mProxyMutex! DO NOT CALL WHILE mProxyMutex IS LOCKED!
+	###########################################################################################*/
 
-	// Get the HTTP proxy address and port
-	LLHost getHTTPProxy() const;
+	// Perform a SOCKS 5 authentication and UDP association with the proxy server.
+	S32 proxyHandshake(LLHost proxy);
+
+	// Get the currently selected auth method.
+	LLSocks5AuthType getSelectedAuthMethod() const;
 
 	// Get the currently selected HTTP proxy type
 	LLHttpProxyType getHTTPProxyType() const;
@@ -293,17 +298,13 @@ public:
 	/*###########################################################################################
 	END OF LOCKING METHODS
 	###########################################################################################*/
-private:
-	// Open a communication channel to the SOCKS 5 proxy proxy, at port messagePort.
-	S32 proxyHandshake(LLHost proxy);
 
 private:
-	// Is the HTTP proxy enabled?
-	// Safe to read in any thread, do not write directly,
-	// use enableHTTPProxy() and disableHTTPProxy() instead.
+	// Is the HTTP proxy enabled? Safe to read in any thread, but do not write directly.
+	// Instead use enableHTTPProxy() and disableHTTPProxy() instead.
 	mutable LLAtomic32<bool> mHTTPProxyEnabled;
 
-	// Mutex to protect shared members in non-main thread calls to applyProxySettings()
+	// Mutex to protect shared members in non-main thread calls to applyProxySettings().
 	mutable LLMutex mProxyMutex;
 
 	/*###########################################################################################
@@ -321,9 +322,6 @@ private:
 	// socket handle to proxy TCP control channel
 	LLSocket::ptr_t mProxyControlChannel;
 
-	// APR pool for the socket
-	apr_pool_t* mPool;
-
 	/*###########################################################################################
 	END OF UNSHARED MEMBERS
 	###########################################################################################*/