From 71f6b139f8de3ea6bf2b925e095fc0f7864c0274 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev Date: Fri, 29 May 2020 20:10:04 +0300 Subject: SL-13348 Thread crashing singleton #1 --- indra/llcorehttp/_httpoprequest.cpp | 14 ++++-------- indra/llmessage/llproxy.cpp | 45 +++++++++++++++++++++++-------------- indra/llmessage/llproxy.h | 8 ++++++- indra/newview/llappviewer.cpp | 1 + 4 files changed, 40 insertions(+), 28 deletions(-) (limited to 'indra') diff --git a/indra/llcorehttp/_httpoprequest.cpp b/indra/llcorehttp/_httpoprequest.cpp index 0f76ff23ea..6909a650da 100644 --- a/indra/llcorehttp/_httpoprequest.cpp +++ b/indra/llcorehttp/_httpoprequest.cpp @@ -567,16 +567,9 @@ HttpStatus HttpOpRequest::prepareRequest(HttpService * service) // Use the viewer-based thread-safe API which has a // fast/safe check for proxy enable. Would like to // encapsulate this someway... - if (LLProxy::instanceExists()) - { - // Make sure proxy won't be initialized from here, - // it might conflict with LLStartUp::startLLProxy() - LLProxy::getInstance()->applyProxySettings(mCurlHandle); - } - else - { - LL_WARNS() << "Proxy is not initialized!" << LL_ENDL; - } + // Make sure proxy won't be getInstance() from here, + // it is not thread safe + LLProxy::applyProxySettings(mCurlHandle); } else if (gpolicy.mHttpProxy.size()) @@ -817,6 +810,7 @@ size_t HttpOpRequest::readCallback(void * data, size_t size, size_t nmemb, void const size_t do_size((std::min)(req_size, body_size - op->mCurlBodyPos)); const size_t read_size(op->mReqBody->read(op->mCurlBodyPos, static_cast(data), do_size)); + // FIXME: singleton's instance() is Thread unsafe! Even if stats accumulators inside are. HTTPStats::instance().recordDataUp(read_size); op->mCurlBodyPos += read_size; return read_size; diff --git a/indra/llmessage/llproxy.cpp b/indra/llmessage/llproxy.cpp index 950599217f..d11a1487ab 100644 --- a/indra/llmessage/llproxy.cpp +++ b/indra/llmessage/llproxy.cpp @@ -40,6 +40,7 @@ // incoming packet just to do a simple bool test. The getter for this // member is also static bool LLProxy::sUDPProxyEnabled = false; +LLProxy* LLProxy::sProxyInstance = NULL; // Some helpful TCP static functions. 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 @@ -60,11 +61,21 @@ LLProxy::LLProxy(): LLProxy::~LLProxy() { - if (ll_apr_is_initialized()) - { - stopSOCKSProxy(); - disableHTTPProxy(); - } + if (ll_apr_is_initialized()) + { + // locks mutex + stopSOCKSProxy(); + disableHTTPProxy(); + } + // The primary safety of sProxyInstance is the fact that by the + // point SUBSYSTEM_CLEANUP(LLProxy) gets called, nothing should + // be capable of using proxy + sProxyInstance = NULL; +} + +void LLProxy::initSingleton() +{ + sProxyInstance = this; } /** @@ -424,28 +435,28 @@ void LLProxy::cleanupClass() void LLProxy::applyProxySettings(CURL* handle) { // Do a faster unlocked check to see if we are supposed to proxy. - if (mHTTPProxyEnabled) + if (sProxyInstance && sProxyInstance->mHTTPProxyEnabled) { - // We think we should proxy, lock the proxy mutex. - LLMutexLock lock(&mProxyMutex); + // We think we should proxy, lock the proxy mutex. sProxyInstance is not protected by mutex + LLMutexLock lock(&sProxyInstance->mProxyMutex); // Now test again to verify that the proxy wasn't disabled between the first check and the lock. - if (mHTTPProxyEnabled) + if (sProxyInstance->mHTTPProxyEnabled) { - LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXY, mHTTPProxy.getIPString().c_str()), CURLOPT_PROXY); - LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, mHTTPProxy.getPort()), CURLOPT_PROXYPORT); + LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXY, sProxyInstance->mHTTPProxy.getIPString().c_str()), CURLOPT_PROXY); + LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYPORT, sProxyInstance->mHTTPProxy.getPort()), CURLOPT_PROXYPORT); - if (mProxyType == LLPROXY_SOCKS) + if (sProxyInstance->mProxyType == LLPROXY_SOCKS) { - LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5), CURLOPT_PROXYTYPE); - if (mAuthMethodSelected == METHOD_PASSWORD) + LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_SOCKS5), CURLOPT_PROXYTYPE); + if (sProxyInstance->mAuthMethodSelected == METHOD_PASSWORD) { - std::string auth_string = mSocksUsername + ":" + mSocksPassword; - LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYUSERPWD, auth_string.c_str()), CURLOPT_PROXYUSERPWD); + std::string auth_string = sProxyInstance->mSocksUsername + ":" + sProxyInstance->mSocksPassword; + LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYUSERPWD, auth_string.c_str()), CURLOPT_PROXYUSERPWD); } } else { - LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP), CURLOPT_PROXYTYPE); + LLCore::LLHttp::check_curl_code(curl_easy_setopt(handle, CURLOPT_PROXYTYPE, CURLPROXY_HTTP), CURLOPT_PROXYTYPE); } } } diff --git a/indra/llmessage/llproxy.h b/indra/llmessage/llproxy.h index 87891901ad..f2eefb26d0 100644 --- a/indra/llmessage/llproxy.h +++ b/indra/llmessage/llproxy.h @@ -225,6 +225,8 @@ class LLProxy: public LLSingleton LLSINGLETON(LLProxy); LOG_CLASS(LLProxy); + /*virtual*/ void initSingleton(); + public: // Static check for enabled status for UDP packets. Call from main thread only. static bool isSOCKSProxyEnabled() { return sUDPProxyEnabled; } @@ -250,7 +252,7 @@ public: // 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); + static void applyProxySettings(CURL* handle); // Start a connection to the SOCKS 5 proxy. Call from main thread only. S32 startSOCKSProxy(LLHost host); @@ -343,6 +345,10 @@ private: /*########################################################################################### END OF SHARED MEMBERS ###########################################################################################*/ + + // A hack to get arround getInstance() and capture_dependency() which are unsafe to use inside threads + // set/reset on init/cleanup, strictly for use in applyProxySettings + static LLProxy* sProxyInstance; }; #endif diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index ff921dcfdb..da1a58efd9 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -2116,6 +2116,7 @@ bool LLAppViewer::cleanup() LLWeb::loadURLExternal( gLaunchFileOnQuit, false ); LL_INFOS() << "File launched." << LL_ENDL; } + // make sure nothing uses applyProxySettings by this point. LL_INFOS() << "Cleaning up LLProxy." << LL_ENDL; SUBSYSTEM_CLEANUP(LLProxy); LLCore::LLHttp::cleanup(); -- cgit v1.2.3