From e19984e2dd8bfc1a9f96477feb61d6029a7694f8 Mon Sep 17 00:00:00 2001 From: Antti Nietosvaara Date: Wed, 10 May 2023 10:49:51 +0300 Subject: [PATCH] Fix a memory leak when using non-wx threads The wxThreadSpecificInfo object created by wxThreadSpecificInfo::Get() was only released by the wxThreadInternal's cleanup, which meant that it was leaked if wxThreadSpecificInfo::Get() was called by a thread not created by wxThread, e.g. when using std::async or std::thread. As we still can't rely on C++11 thread local variables in this branch (see d666d1e222 (Fix a memory leak when using non-wx threads, 2023-05-10) in master), we work around the problem by replacing the ThreadCleanUp call with platform specific implementations for wxThreadSpecificInfo::Get() that free the allocated wxThreadSpecificInfo automatically on thread exit. Note that the Fls functions used for Windows implementation are not supported on Windows XP, and thus the problem still persists there. Closes ##23535. --- docs/changes.txt | 1 + include/wx/private/threadinfo.h | 7 -- src/common/threadinfo.cpp | 80 ++--------------------- src/msw/thread.cpp | 109 +++++++++++++++++++++++++++++++- src/unix/threadpsx.cpp | 53 ++++++++++++++-- 5 files changed, 160 insertions(+), 90 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index e172eaf1fe..baa5eafe5f 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -243,6 +243,7 @@ All: - Don't use invalid iterator in wxString in UTF-8 build (Ian Day, #23305). - Fix wrong description for some languages (Ulrich Telle, #23419). - Fix infinite loop in wxFTP::GetFilesList() (#23519). +- Fix memory leak when using wx from non-wx threads (Antti Nietosvaara, #23535). - Use grep instead of fgrep and egrep in makefiles/scripts (#23537). All (GUI): diff --git a/include/wx/private/threadinfo.h b/include/wx/private/threadinfo.h index 27a71674b0..1ff241bda8 100644 --- a/include/wx/private/threadinfo.h +++ b/include/wx/private/threadinfo.h @@ -52,13 +52,6 @@ public: wxLocaleUntranslatedStrings untranslatedStrings; #endif -#if wxUSE_THREADS - // Cleans up storage for the current thread. Should be called when a thread - // is being destroyed. If it's not called, the only bad thing that happens - // is that the memory is deallocated later, on process termination. - static void ThreadCleanUp(); -#endif - private: wxThreadSpecificInfo() : logger(NULL), loggingDisabled(false) {} }; diff --git a/src/common/threadinfo.cpp b/src/common/threadinfo.cpp index f6d3e3b30f..64026def57 100644 --- a/src/common/threadinfo.cpp +++ b/src/common/threadinfo.cpp @@ -10,86 +10,14 @@ // For compilers that support precompilation, includes "wx.h". #include "wx/wxprec.h" - -#include "wx/private/threadinfo.h" - #if wxUSE_THREADS -#include "wx/tls.h" -#include "wx/thread.h" -#include "wx/sharedptr.h" -#include "wx/vector.h" +// The threaded version for wxThreadSpecificInfo::Get() is defined in platform specific +// thread files: src/msw/thread.cpp or src/unix/threadpsx.cpp -namespace -{ +#else -// All thread info objects are stored in a global list so that they are -// freed when global objects are destroyed and no memory leaks are reported. - -// Notice that we must be using accessor functions instead of simple global -// variables here as this code could be executed during global initialization -// time, i.e. before any globals in this module were initialzied. -inline wxCriticalSection& GetAllThreadInfosCS() -{ - static wxCriticalSection s_csAllThreadInfos; - - return s_csAllThreadInfos; -} - -typedef wxVector< wxSharedPtr > wxAllThreadInfos; -inline wxAllThreadInfos& GetAllThreadInfos() -{ - static wxAllThreadInfos s_allThreadInfos; - - return s_allThreadInfos; -} - -// Pointer to the current thread's instance -inline wxTLS_TYPE_REF(wxThreadSpecificInfo*) GetThisThreadInfo() -{ - static wxTLS_TYPE(wxThreadSpecificInfo*) s_thisThreadInfo; - - return s_thisThreadInfo; -} - -#define wxTHIS_THREAD_INFO wxTLS_VALUE(GetThisThreadInfo()) - -} // anonymous namespace - - -wxThreadSpecificInfo& wxThreadSpecificInfo::Get() -{ - if ( !wxTHIS_THREAD_INFO ) - { - wxTHIS_THREAD_INFO = new wxThreadSpecificInfo; - wxCriticalSectionLocker lock(GetAllThreadInfosCS()); - GetAllThreadInfos().push_back( - wxSharedPtr(wxTHIS_THREAD_INFO)); - } - return *wxTHIS_THREAD_INFO; -} - -void wxThreadSpecificInfo::ThreadCleanUp() -{ - if ( !wxTHIS_THREAD_INFO ) - return; // nothing to do, not used by this thread at all - - // find this thread's instance in GetAllThreadInfos() and destroy it - wxCriticalSectionLocker lock(GetAllThreadInfosCS()); - for ( wxAllThreadInfos::iterator i = GetAllThreadInfos().begin(); - i != GetAllThreadInfos().end(); - ++i ) - { - if ( i->get() == wxTHIS_THREAD_INFO ) - { - GetAllThreadInfos().erase(i); - wxTHIS_THREAD_INFO = NULL; - break; - } - } -} - -#else // !wxUSE_THREADS +#include "wx/private/threadinfo.h" wxThreadSpecificInfo& wxThreadSpecificInfo::Get() { diff --git a/src/msw/thread.cpp b/src/msw/thread.cpp index e684208820..3bd1075279 100644 --- a/src/msw/thread.cpp +++ b/src/msw/thread.cpp @@ -122,6 +122,111 @@ static bool gs_waitingForThread = false; // Windows implementation of thread and related classes // ============================================================================ +// Create a wrapper class for storing wxThreadSpecificInfo +// using FLS or TLS api. Preferably we want to use FLS +// since it supports freeing the created objects automatically +// on thread exit. However, this is only supported from +// Windows Vista and newer, so TLS is used as a fallback. +// Note that with TLS we need to clean up the objects +// manually in wxThreadInternal::WinThreadStart, and +// if the running thread is not a wxThread, the objects +// will not be freed until program exit. As mentioned, +// this will only affect Windows XP. +class wxThreadSpecificInfoTLS +{ +private: + typedef DWORD(WINAPI *AllocCallback_t)(void (WINAPI*)(void*)); + AllocCallback_t AllocCallback; + + typedef DWORD(WINAPI *Alloc_t)(); + Alloc_t Alloc; + + typedef BOOL(WINAPI *Free_t)(DWORD); + Free_t Free; + + typedef void* (WINAPI *GetValue_t)(DWORD); + GetValue_t GetValue; + + typedef BOOL(WINAPI *SetValue_t)(DWORD, void*); + SetValue_t SetValue; + + DWORD m_idx; + + static void WINAPI DeleteThreadSpecificInfo(void* ptr) + { + delete static_cast(ptr); + } + + static const wxThreadSpecificInfoTLS& Instance() + { + static wxThreadSpecificInfoTLS s_instance; + return s_instance; + } + + wxThreadSpecificInfoTLS(): + AllocCallback(reinterpret_cast(GetProcAddress(GetModuleHandle(_T("kernel32.dll")), "FlsAlloc"))), + Alloc(NULL), + Free(reinterpret_cast(GetProcAddress(GetModuleHandle(_T("kernel32.dll")), "FlsFree"))), + GetValue(reinterpret_cast(GetProcAddress(GetModuleHandle(_T("kernel32.dll")), "FlsGetValue"))), + SetValue(reinterpret_cast(GetProcAddress(GetModuleHandle(_T("kernel32.dll")), "FlsSetValue"))) + { + if (AllocCallback && Free && GetValue && SetValue) + { + // FLS API was available, so use it to free objects automatically + m_idx = AllocCallback(&DeleteThreadSpecificInfo); + } + else + { + // FLS API was not available (Windows XP?), so use TLS as fallback + AllocCallback = NULL; + Alloc = reinterpret_cast(TlsAlloc); + Free = reinterpret_cast(TlsFree); + GetValue = reinterpret_cast(TlsGetValue); + SetValue = reinterpret_cast(TlsSetValue); + m_idx = Alloc(); + } + } + +public: + static wxThreadSpecificInfo* Get() + { + return static_cast(Instance().GetValue(Instance().m_idx)); + } + + // wxThreadSpecificInfo will try to delete info when thread ends + static bool Set(wxThreadSpecificInfo* info) + { + return Instance().SetValue(Instance().m_idx, info); + } + + static void CleanUp() + { + if (!Instance().AllocCallback) + { + // FLS API was not available, which means that objects will not be freed automatically. + delete Get(); + Set(NULL); + } + } +}; + +wxThreadSpecificInfo& wxThreadSpecificInfo::Get() +{ + wxThreadSpecificInfo* info = wxThreadSpecificInfoTLS::Get(); + if (!info) + { + info = new wxThreadSpecificInfo; + if (!wxThreadSpecificInfoTLS::Set(info)) + { + // This will crash, but we'd leak memory otherwise which + // could be even worse and less immediately discoverable. + delete info; + info = NULL; + } + } + return *info; +} + // ---------------------------------------------------------------------------- // wxCriticalSection // ---------------------------------------------------------------------------- @@ -587,7 +692,9 @@ THREAD_RETVAL THREAD_CALLCONV wxThreadInternal::WinThreadStart(void *param) // Do this as the very last thing to ensure that thread-specific info is // not recreated any longer. - wxThreadSpecificInfo::ThreadCleanUp(); + // Note thatg this is necessary only if the wxThreadSpecificInfoTLS cannot + // perform the cleanup automatically. That is, with Windows XP or older. + wxThreadSpecificInfoTLS::CleanUp(); return rc; } diff --git a/src/unix/threadpsx.cpp b/src/unix/threadpsx.cpp index 7f920f76e2..c86b8874e8 100644 --- a/src/unix/threadpsx.cpp +++ b/src/unix/threadpsx.cpp @@ -27,7 +27,6 @@ #include "wx/thread.h" #include "wx/except.h" -#include "wx/scopeguard.h" #include "wx/private/threadinfo.h" @@ -858,11 +857,6 @@ void *wxPthreadStart(void *ptr) void *wxThreadInternal::PthreadStart(wxThread *thread) { - // Ensure that we clean up thread-specific data before exiting the thread - // and do it as late as possible as wxLog calls can recreate it and may - // happen until the very end. - wxON_BLOCK_EXIT0(wxThreadSpecificInfo::ThreadCleanUp); - wxThreadInternal *pthread = thread->m_internal; wxLogTrace(TRACE_THREADS, wxT("Thread %p started."), THR_ID(pthread)); @@ -2031,6 +2025,53 @@ void wxMutexGuiLeaveImpl() #endif +wxThreadSpecificInfo& wxThreadSpecificInfo::Get() +{ + // Since wxTlsValue only allows POD types, we need to use TLS API directly instead + // to free the allocated object automatically on thread exit. + class wxThreadSpecificInfoTLS + { + private: + static void DeleteThreadSpecificInfo(void *ptr) + { + delete static_cast(ptr); + } + pthread_key_t m_key; + + public: + wxThreadSpecificInfoTLS() + { + pthread_key_create(&m_key, &DeleteThreadSpecificInfo); + } + + ~wxThreadSpecificInfoTLS() + { + pthread_key_delete(m_key); + } + + wxThreadSpecificInfo& Get() const + { + wxThreadSpecificInfo* info = + static_cast(pthread_getspecific(m_key)); + if (!info) + { + info = new wxThreadSpecificInfo; + if (pthread_setspecific(m_key, info) != 0) + { + // This will crash, but we'd leak memory otherwise which + // could be even worse and less immediately discoverable. + delete info; + info = NULL; + } + } + return *info; + } + }; + + static const wxThreadSpecificInfoTLS s_info; + return s_info.Get(); +} + // ---------------------------------------------------------------------------- // include common implementation code // ----------------------------------------------------------------------------