Add explicit size storage for thread names to protect against overrun…

… on very long thread names

include/SpecialK/diagnostics/debug_utils.h

@@ -664,9 +664,9 @@ using SymLoadModule64_pfn = DWORD64 (IMAGEAPI *)( _In_     HANDLE  hProcess,
                                                   _In_     DWORD   SizeOfDll );
 
 
-std::wstring& SK_Thread_GetName    (DWORD  dwTid);
-std::wstring& SK_Thread_GetName    (HANDLE hThread);
-DWORD         SK_Thread_FindByName (std::wstring name);
+const wchar_t* SK_Thread_GetName    (DWORD  dwTid);
+const wchar_t* SK_Thread_GetName    (HANDLE hThread);
+DWORD          SK_Thread_FindByName (std::wstring name);
 
 
 #include <cassert>
@@ -866,7 +866,7 @@ SK_SetThreadPriority ( HANDLE hThread,
 
 
 extern SK_LazyGlobal <
-  concurrency::concurrent_unordered_map <DWORD, std::wstring>
+  concurrency::concurrent_unordered_map <DWORD, std::array <wchar_t, SK_MAX_THREAD_NAME_LEN+1>>
 > _SK_ThreadNames;
 extern SK_LazyGlobal <
   concurrency::concurrent_unordered_set <DWORD>

include/SpecialK/tls.h

@@ -1,4 +1,4 @@
-/**
+/**
  * This file is part of Special K.
  *
  * Special K is free software : you can redistribute it
@@ -20,6 +20,8 @@
 **/
 #pragma once
 
+#define SK_MAX_THREAD_NAME_LEN MAX_PATH*2
+
 // Useless warning:  'typedef ': ignored on left of '' when no variable is declared
 #pragma warning (disable: 4091)
 
@@ -61,8 +63,6 @@ struct SK_MMCS_TaskEntry;
 #endif
 
 
-#define MAX_THREAD_NAME_LEN MAX_PATH
-
 class SK_ModuleAddrMap
 {
 public:
@@ -791,7 +791,7 @@ class SK_TLS
     LONG             exception_repeats =     0;
     callsite_list_t  suppressed_addrs  = {   };
     wchar_t          name
-         [MAX_THREAD_NAME_LEN]         = {   };
+         [SK_MAX_THREAD_NAME_LEN+1]    = {   };
     SK_AutoHandle    handle            =
       SK_AutoHandle (INVALID_HANDLE_VALUE);
     DWORD            tls_idx           =     0;

src/diagnostics/debug_utils.cpp

@@ -2045,10 +2045,10 @@ ZwSetInformationThread_Detour (
     {
       if (pTLS != nullptr) pTLS->debug.hidden = true;
 
-      SK_LOG0 ( ( L"tid=%x (%s) tried to hide itself from debuggers; please "
+      SK_LOG0 ( ( L"tid=%x (%ws) tried to hide itself from debuggers; please "
                   L"attach one and investigate!",
                   GetThreadId (ThreadHandle),
-            SK_Thread_GetName (ThreadHandle).c_str () ),
+            SK_Thread_GetName (ThreadHandle) ),
                   L"DieAntiDbg" );
     }
 
@@ -2213,8 +2213,7 @@ ZwCreateThreadEx_Detour (
     auto& ThreadNames =
      *_SK_ThreadNames;
 
-    if ( ThreadNames.find (tid) ==
-         ThreadNames.cend (   ) )
+    if (ThreadNames.count (tid) == 0)
     {
       ulLen =
         SK_GetSymbolNameFromModuleAddr (
@@ -2244,17 +2243,16 @@ ZwCreateThreadEx_Detour (
         thread_name       )
       );
 
-      ThreadNames.insert (
-        std::make_pair ( tid, thr_name )
-      );
+      wcsncpy_s (ThreadNames [tid].data (), SK_MAX_THREAD_NAME_LEN,
+                         thr_name.c_str (),             _TRUNCATE);
 
       SK_TLS* pTLS =
         SK_TLS_BottomEx (tid);
 
       if (pTLS != nullptr)
       {
         wcsncpy_s (
-          pTLS->debug.name,          MAX_THREAD_NAME_LEN,
+          pTLS->debug.name,          SK_MAX_THREAD_NAME_LEN,
                   thr_name.c_str (), _TRUNCATE
         );
       }
@@ -2423,17 +2421,16 @@ NtCreateThreadEx_Detour (
         thread_name       )
       );
 
-      ThreadNames.insert (
-        std::make_pair ( tid, thr_name )
-      );
+      wcsncpy_s (ThreadNames [tid].data (), SK_MAX_THREAD_NAME_LEN,
+                         thr_name.c_str (),             _TRUNCATE);
 
       SK_TLS* pTLS =
         SK_TLS_BottomEx (tid);
 
       if (pTLS != nullptr)
       {
         wcsncpy_s (
-          pTLS->debug.name,          MAX_THREAD_NAME_LEN,
+          pTLS->debug.name,          SK_MAX_THREAD_NAME_LEN,
                   thr_name.c_str (), _TRUNCATE
         );
       }
@@ -2910,15 +2907,16 @@ SK_Exception_HandleThreadName (
       {
         wcsncpy_s (
           pTLS->debug.name,
-          std::min (len+1, (size_t)MAX_THREAD_NAME_LEN-1),
+          std::min (len+1, (size_t)SK_MAX_THREAD_NAME_LEN-1),
           wide_name.c_str (),
           _TRUNCATE );
       }
 
-      // Do not move the string, resize the existing name, clear it, and append a copy!
-      ThreadNames [dwTid].resize (MAX_THREAD_NAME_LEN+1);
-      ThreadNames [dwTid].assign (wide_name.c_str ());
-      // * Ideally this would be a wchar_t array of fixed size
+      // Do not move the string; truncate the string,
+      //   and replace any existing name with a copy!
+                *ThreadNames [dwTid].data () = L'\0';
+      wcsncpy_s (ThreadNames [dwTid].data (),SK_MAX_THREAD_NAME_LEN,
+                          wide_name.c_str (),            _TRUNCATE);
 
 #ifdef _M_AMD64
       if (SK_GetCurrentGameID () == SK_GAME_ID::EldenRing)

src/injection/injection.cpp

@@ -1260,27 +1260,27 @@ SK_Inject_SpawnUnloadListener (void)
       CreateThread (nullptr, 0, [](LPVOID) ->
       DWORD
       {
-        HMODULE hModKernel32 =
-          SK_LoadLibraryW (L"Kernel32");
+        if (SK_IsInjected ())
+        { // Avoid changing thread names in processes we do not care about
+          HMODULE hModKernel32 =
+            SK_LoadLibraryW (L"Kernel32");
 
-        if (hModKernel32 != nullptr)
-        {
-          // Try the Windows 10 API for Thread Names first, it's ideal unless ... not Win10 :)
-           SetThreadDescription_pfn
-          _SetThreadDescriptionWin10 =
-          (SetThreadDescription_pfn)SK_GetProcAddress (hModKernel32, "SetThreadDescription");
+          if (hModKernel32 != nullptr)
+          {
+            // Try the Windows 10 API for Thread Names first, it's ideal unless ... not Win10 :)
+             SetThreadDescription_pfn
+            _SetThreadDescriptionWin10 =
+            (SetThreadDescription_pfn)SK_GetProcAddress (hModKernel32, "SetThreadDescription");
 
-          if (SK_IsInjected ())
-          { // Avoid changing thread names in processes we do not care about
             if (_SetThreadDescriptionWin10 != nullptr) {
                 _SetThreadDescriptionWin10 (
                   g_hPacifierThread,
                     L"[SK] Global Hook Pacifier"
                 );
             }
-          }
 
-          SK_FreeLibrary (hModKernel32);
+            SK_FreeLibrary (hModKernel32);
+          }
         }
 
         SetThreadPriority (g_hPacifierThread, THREAD_PRIORITY_TIME_CRITICAL);

src/plugins/sekiro.cpp

@@ -188,8 +188,8 @@ WSAAPI WSAWaitForMultipleEvents_Detour (
 
   if (! disable_network_code)
   {
-    dll_log->Log ( L" >> Calling Thread for Network Activity: %s",
-                     SK_Thread_GetName (SK_Thread_GetCurrentId ()).c_str () );
+    dll_log->Log ( L" >> Calling Thread for Network Activity: %ws",
+                     SK_Thread_GetName (SK_Thread_GetCurrentId ()) );
   }
 
   UNREFERENCED_PARAMETER (cEvents);
@@ -226,8 +226,8 @@ WSAAPI WSASocketW_Detour (
 
   if (! disable_network_code)
   {
-    dll_log->Log ( L" >> Calling Thread for Network Activity: %s",
-                     SK_Thread_GetName (SK_Thread_GetCurrentId () ).c_str () );
+    dll_log->Log ( L" >> Calling Thread for Network Activity: %ws",
+                     SK_Thread_GetName (SK_Thread_GetCurrentId ()) );
   }
 
   UNREFERENCED_PARAMETER (af);

src/thread.cpp

@@ -44,9 +44,9 @@ SetThreadDescription_pfn SetThreadDescription_Original = nullptr;
 
 static constexpr DWORD MAGIC_THREAD_EXCEPTION = 0x406D1388;
 
-SK_LazyGlobal <concurrency::concurrent_unordered_map <DWORD, std::wstring>> _SK_ThreadNames;
-SK_LazyGlobal <concurrency::concurrent_unordered_set <DWORD>>               _SK_SelfTitledThreads;
-SK_LazyGlobal <concurrency::concurrent_unordered_set <DWORD>>               _SK_UntitledThreads;
+SK_LazyGlobal <concurrency::concurrent_unordered_map <DWORD, std::array <wchar_t, SK_MAX_THREAD_NAME_LEN+1>>> _SK_ThreadNames;
+SK_LazyGlobal <concurrency::concurrent_unordered_set <DWORD>>                                                 _SK_SelfTitledThreads;
+SK_LazyGlobal <concurrency::concurrent_unordered_set <DWORD>>                                                 _SK_UntitledThreads;
 
 void __make_self_titled (DWORD dwTid);
 
@@ -97,7 +97,7 @@ SK_GetThreadDescription ( _In_              HANDLE hThread,
 
 static std::wstring _noname = L"";
 
-std::wstring&
+const wchar_t*
 SK_Thread_QueryNameFromOS (DWORD dwTid)
 {
   if (_SK_UntitledThreads->find (dwTid) == _SK_UntitledThreads->cend ())
@@ -115,16 +115,15 @@ SK_Thread_QueryNameFromOS (DWORD dwTid)
         if ( wszThreadName != nullptr &&
             *wszThreadName != L'\0' ) // Empty strings are not useful :)
         {
-          auto& names =
-            _SK_ThreadNames.get ();
+          auto name =
+            _SK_ThreadNames.get()[dwTid].data();
 
           __make_self_titled (dwTid);
-                       names [dwTid] = wszThreadName;
+          wcsncpy_s (name, SK_MAX_THREAD_NAME_LEN, wszThreadName, _TRUNCATE);
 
           LocalFree (wszThreadName);
 
-          return
-            names [dwTid];
+          return name;
         }
 
         LocalFree (wszThreadName);
@@ -135,7 +134,7 @@ SK_Thread_QueryNameFromOS (DWORD dwTid)
   }
 
   return
-    _noname;
+    L"";
 }
 
 DWORD
@@ -144,16 +143,16 @@ SK_Thread_FindByName (std::wstring name)
   auto& threads =
     _SK_ThreadNames.get ();
 
-  for (auto &thread : threads)
+  for (const auto &thread : threads)
   {
-    if (thread.second._Equal (name.c_str ()))
+    if (!_wcsicmp (thread.second.data (), name.c_str ()))
       return thread.first;
   }
 
   return 0;
 }
 
-std::wstring&
+const wchar_t*
 SK_Thread_GetName (DWORD dwTid)
 {
   auto& names =
@@ -163,13 +162,13 @@ SK_Thread_GetName (DWORD dwTid)
     names.find (dwTid);
 
   if (it != names.cend ())
-    return (*it).second;
+    return (*it).second.data ();
 
   return
     SK_Thread_QueryNameFromOS (dwTid);
 }
 
-std::wstring&
+const wchar_t*
 SK_Thread_GetName (HANDLE hThread)
 {
   return
@@ -280,9 +279,9 @@ SetCurrentThreadDescription (_In_ PCWSTR lpThreadDescription)
 
   bool non_empty =
     SUCCEEDED ( StringCbLengthW (
-                  lpThreadDescription, MAX_THREAD_NAME_LEN-1, &len
+                  lpThreadDescription, SK_MAX_THREAD_NAME_LEN-1, &len
                 )
-              )                                             && len > 0;
+              )                                                && len > 0;
 
   if (non_empty)
   {
@@ -293,24 +292,25 @@ SetCurrentThreadDescription (_In_ PCWSTR lpThreadDescription)
     SK_TLS *pTLS =
       SK_TLS_Bottom ();
 
-    DWORD               dwTid  = SK_Thread_GetCurrentId ();
-    __make_self_titled (dwTid);
-           ThreadNames [dwTid] = lpThreadDescription;
+    DWORD                   dwTid  = SK_Thread_GetCurrentId ();
+    __make_self_titled     (dwTid);
+    wcsncpy_s (ThreadNames [dwTid].data (), SK_MAX_THREAD_NAME_LEN,
+               lpThreadDescription,                     _TRUNCATE);
 
     if (pTLS != nullptr)
     {
       // Push this to the TLS data-store so we can get thread names even
       //   when no debugger is attached.
       wcsncpy_s (
         pTLS->debug.name,
-          std::min (MAX_THREAD_NAME_LEN, (int)len+1),
+          std::min (SK_MAX_THREAD_NAME_LEN, (int)len+1),
             lpThreadDescription,
               _TRUNCATE
       );
     }
 
-    char      szDesc                      [MAX_THREAD_NAME_LEN] = { };
-    wcstombs (szDesc, lpThreadDescription, MAX_THREAD_NAME_LEN-1);
+    char      szDesc                      [SK_MAX_THREAD_NAME_LEN] = { };
+    wcstombs (szDesc, lpThreadDescription, SK_MAX_THREAD_NAME_LEN-1);
 
     THREADNAME_INFO info = {       };
     info.dwType          =      4096;
@@ -342,10 +342,10 @@ GetCurrentThreadDescription (_Out_  PWSTR  *threadDescription)
   {
     // This is not freed here; the caller is expected to free it!
     *threadDescription =
-      (wchar_t *)SK_LocalAlloc (LPTR, sizeof (wchar_t) * MAX_THREAD_NAME_LEN);
+      (wchar_t *)SK_LocalAlloc (LPTR, sizeof (wchar_t) * SK_MAX_THREAD_NAME_LEN);
 
     wcsncpy_s (
-      *threadDescription, MAX_THREAD_NAME_LEN-1,
+      *threadDescription, SK_MAX_THREAD_NAME_LEN-1,
         pTLS->debug.name, _TRUNCATE
     );
 
@@ -434,8 +434,8 @@ SetThreadDescription_Detour (HANDLE hThread, PCWSTR lpThreadDescription)
          SK_ValidatePointer ((void *)lpThreadDescription, true)
        )
     {
-      char      szDesc                      [MAX_THREAD_NAME_LEN] = { };
-      wcstombs (szDesc, lpThreadDescription, MAX_THREAD_NAME_LEN-1);
+      char      szDesc                      [SK_MAX_THREAD_NAME_LEN] = { };
+      wcstombs (szDesc, lpThreadDescription, SK_MAX_THREAD_NAME_LEN-1);
 
       THREADNAME_INFO info = {       };
       info.dwType          =      4096;