Merge pull request #71 from CookiePLMonster/cdstream-deadlock

Fixed a deadlock in CdStream and LoadLibrary path translation heuristics

Author: thelink2012

include/modloader/gta3/gta3.hpp

@@ -79,8 +79,8 @@ namespace modloader
 
     template<uintptr_t addr, class Traits = dtraits::WinCreateFileA>
     using WinCreateFileA = modloader::basic_file_detour<Traits,
-                                                injector::function_hooker_stdcall<addr, HANDLE(LPCTSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE)>,
-                                                                                        HANDLE, LPCTSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE>;
+                                                injector::function_hooker_stdcall<addr, HANDLE(LPCSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE)>,
+                                                                                        HANDLE, LPCSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE>;
 
 
     template<uintptr_t addr, class Traits = dtraits::LoadAtomic2Return>

include/modloader/modloader.h

@@ -29,7 +29,7 @@ extern "C" {
 /* Version */
 #define MODLOADER_VERSION_MAJOR         0
 #define MODLOADER_VERSION_MINOR         3
-#define MODLOADER_VERSION_REVISION      5
+#define MODLOADER_VERSION_REVISION      7
 #ifdef NDEBUG
 #define MODLOADER_VERSION_ISDEV         0
 #else

include/modloader/util/path.hpp

@@ -311,9 +311,9 @@ namespace modloader
      *      WinAPI-like function to check if a directory exists
      *      @szPath: Directory to check
      */
-    inline BOOL IsDirectoryA(LPCTSTR szPath)
+    inline BOOL IsDirectoryA(LPCSTR szPath)
     {
-      DWORD dwAttrib = GetFileAttributes(szPath);
+      DWORD dwAttrib = GetFileAttributesA(szPath);
       return (dwAttrib != INVALID_FILE_ATTRIBUTES && 
              (dwAttrib & FILE_ATTRIBUTE_DIRECTORY));
     }
@@ -323,7 +323,7 @@ namespace modloader
      *      WinAPI-like function to check if a file or directory exists
      *      @szPath: Directory to check
      */
-    inline BOOL IsPathA(LPCTSTR szPath)
+    inline BOOL IsPathA(LPCSTR szPath)
     {
       DWORD dwAttrib = GetFileAttributesA(szPath);
       return (dwAttrib != INVALID_FILE_ATTRIBUTES);
@@ -352,7 +352,7 @@ namespace modloader
      *      WinAPI-like function to make sure a directory exists, if not, create it
      *      @szPath: Directory to check
      */
-    inline BOOL MakeSureDirectoryExistA(LPCTSTR szPath)
+    inline BOOL MakeSureDirectoryExistA(LPCSTR szPath)
     {
         if(!IsDirectoryA(szPath))
         {
@@ -371,7 +371,7 @@ namespace modloader
      *      WinAPI-like function that copies the full directory @szFrom to @szTo
      *      If @szTo doesn't exist, it is created
      */
-    inline BOOL CopyDirectoryA(LPCTSTR szFrom, LPCTSTR szTo)
+    inline BOOL CopyDirectoryA(LPCSTR szFrom, LPCSTR szTo)
     {
         if(CreateDirectoryA(szTo, NULL))
         {
@@ -403,7 +403,7 @@ namespace modloader
      *  DestroyDirectoryA
      *      WinAPI-like function that deletes the path @szPath fully
      */
-    inline BOOL DestroyDirectoryA(LPCTSTR szPath)
+    inline BOOL DestroyDirectoryA(LPCSTR szPath)
     {
         FilesWalk(szPath, "*.*", false, [&szPath](FileWalkInfo& file)
         {
@@ -429,7 +429,7 @@ namespace modloader
      *  GetFileSize
      *      WinAPI-like function that gets the file size of @szPath
      */
-    inline LONGLONG GetFileSize(LPCTSTR szPath)
+    inline LONGLONG GetFileSize(LPCSTR szPath)
     {
         WIN32_FILE_ATTRIBUTE_DATA fad;
         return GetFileAttributesExA(szPath, GetFileExInfoStandard, &fad)?
@@ -471,7 +471,8 @@ namespace modloader
 
         /* Enter on ctor, Leave on dtor */
         scoped_lock(CRITICAL_SECTION& cs)
-        { c = &cs; EnterCriticalSection(&cs); }
+			: c(&cs)
+		{ EnterCriticalSection(&cs); }
         ~scoped_lock()
         { LeaveCriticalSection(c); }
     };

src/plugins/gta3/std.asi/args_translator/hacks/FindCleoScripts.hpp

@@ -247,7 +247,7 @@ namespace hacks
      *  Hacked FindFirstFileA
      */
     template<>
-    bool FindCleoScripts::Finder<aFindFirstFileA>(HANDLE& result, LPCTSTR& lpFileName, LPWIN32_FIND_DATAA& lpFindFileData)
+    bool FindCleoScripts::Finder<aFindFirstFileA>(HANDLE& result, LPCSTR& lpFileName, LPWIN32_FIND_DATAA& lpFindFileData)
     {
         char version;
 

src/plugins/gta3/std.asi/args_translator/translator_basic.hpp

@@ -77,6 +77,7 @@ struct path_translator_base
     bool bFindFirstFile;            // ^
     bool bFindNextFile;             // ^
     bool bFindClose;                // ^
+    bool bLoadLibrary;              // ^
     bool bDoBassHack;               // ^
 
     // Almost static vars (he), the value is always the same in all objects
@@ -463,6 +464,7 @@ struct path_translator_basic : public path_translator_base
             bFindFirstFile     = (Symbol == aFindFirstFileA) || (Symbol == aFindFirstFileW);
             bFindNextFile      = (Symbol == aFindNextFileA) || (Symbol == aFindNextFileW);
             bFindClose         = (Symbol == aFindClose);
+            bLoadLibrary	   = (Symbol == aLoadLibraryA) || (Symbol == aLoadLibraryW) || (Symbol == aLoadLibraryExA) || (Symbol == aLoadLibraryExW);
             bIniOperations     = false;
         }
     }

src/plugins/gta3/std.asi/args_translator/translator_stdcall.hpp

@@ -72,7 +72,21 @@ struct path_translator_stdcall<Symbol, LibName, Ret(Args...)> : public path_tran
             }
 
             if(!bDetoured)
-                info.TranslateForCall(a...); // Translate the paths
+			{
+				if(info.base->bLoadLibrary)
+				{
+					auto f = (func_type) info.base->fun;
+					result = f(a...);
+					if(result != NULL)
+					{
+						// If DLL loaded without translating the path, abort translation and don't try to call it again
+						bDetoured = true;
+					}
+
+				}
+				if(!bDetoured)
+					info.TranslateForCall(a...); // Translate the paths
+			}
         }
 
         if(!bDetoured)

src/plugins/gta3/std.stream/backend.cpp

@@ -78,6 +78,53 @@ extern "C"
 };
 
 
+void __stdcall CdStreamShutdownSync_Stub( CdStream* stream, size_t idx )
+{
+	streaming->cdStreamSyncFuncs.Shutdown( &stream[idx] );
+}
+
+uint32_t CdStreamSync( int32_t streamID )
+{
+	static bool bInitFields = false;
+	static CdStream** ppStreams;
+	static BOOL* streamingInitialized;
+	static BOOL* overlappedIO;
+
+	if ( !bInitFields )
+	{
+		if( gvm.IsSA() )
+		{
+			auto& cdinfo = *memory_pointer(0x8E3FE0).get<CdStreamInfoSA>();
+			ppStreams = &cdinfo.pStreams;
+			streamingInitialized = &cdinfo.streamingInitialized;
+			overlappedIO = &cdinfo.overlappedIO;
+		}
+		else if( gvm.IsVC() || gvm.IsIII() )
+		{
+			ppStreams = memory_pointer(xVc(0x6F76FC)).get<CdStream*>();
+			streamingInitialized = memory_pointer(xVc(0x6F7718)).get<BOOL>();
+			overlappedIO = memory_pointer(xVc(0x6F7714)).get<BOOL>();
+		}
+	
+		bInitFields = true;
+	}
+
+	CdStream* stream = &((*ppStreams)[streamID]);
+	if ( *streamingInitialized )
+	{
+		scoped_lock lock( streaming->cdStreamSyncLock );
+		streaming->cdStreamSyncFuncs.SleepCS( stream, &streaming->cdStreamSyncLock );
+		stream->bInUse = 0;
+		return stream->status;
+	}
+
+	if ( *overlappedIO && stream->hFile != nullptr )
+	{
+		DWORD numBytesRead;
+		return GetOverlappedResult( stream->hFile, &stream->overlapped, &numBytesRead, TRUE ) != 0 ? 0 : 254;
+	}
+	return 0;
+}
 
 /*
  *  Streaming thread
@@ -92,7 +139,7 @@ int __stdcall CdStreamThread()
     // Get reference to the addresses we'll use.
     if(gvm.IsSA())
     {
-        auto& cdinfo = *memory_pointer(0x8E3FEC).get<CdStreamInfoSA>();
+        auto& cdinfo = *memory_pointer(0x8E3FE0).get<CdStreamInfoSA>();
         pSemaphore = &cdinfo.semaphore;
         pQueue = &cdinfo.queue;
         ppStreams = &cdinfo.pStreams;
@@ -192,9 +239,14 @@ int __stdcall CdStreamThread()
 
         // Cleanup
         if(bIsAbstract) streaming->CloseModel(sfile);
-        cd->nSectorsToRead = 0;
-        if(cd->bLocked) ReleaseSemaphore(cd->semaphore, 1, 0);
-        cd->bInUse = false;
+		{
+			// This critical section fixes a deadlock with CdStreamThread present in original code
+			scoped_lock xlock(streaming->cdStreamSyncLock);
+
+			cd->nSectorsToRead = 0;
+			streaming->cdStreamSyncFuncs.Wake(cd);
+			cd->bInUse = false;
+		}
     }
     return 0;
 }
@@ -442,6 +494,36 @@ void CAbstractStreaming::Patch()
         // Making our our code for the stream thread would make things so much better
         MakeJMP(0x406560, raw_ptr(CdStreamThread));
 
+		// These are required so we can fix CdStream race condition
+		MakeJMP( 0x406460, raw_ptr(CdStreamSync) );
+		if( gvm.IsSA() )
+		{
+			const uint8_t mem[] = { 0xFF, 0x15 };
+			WriteMemoryRaw( 0x406910, mem, sizeof(mem), true );
+			WriteMemory( 0x406910 + 2, &streaming->cdStreamSyncFuncs.Initialize, true );
+			MakeNOP( 0x406910 + 6, 4 );
+			MakeNOP( 0x406910 + 0x16, 2 );
+		}
+		else if( gvm.IsVC() || gvm.IsIII() )
+		{
+			MakeNOP( xVc(0x4088F7), 8 );
+			WriteMemory( xVc(0x4088F7) + 10, &streaming->cdStreamSyncFuncs.Initialize, true );
+			WriteMemory( xVc(0x408919), uint8_t(0xEB), true );
+		}
+
+		if( gvm.IsSA() )
+		{
+			const uint8_t mem[] = { 0x56, 0x50 };
+			WriteMemoryRaw( 0x4063B5, mem, sizeof(mem), true );
+			MakeCALL( 0x4063B5 + 2, raw_ptr(CdStreamShutdownSync_Stub), true );
+		}
+		else if( gvm.IsVC() || gvm.IsIII() )
+		{
+			const uint8_t mem[] = { 0x8D, 0x04, 0x29, 0x90 };
+			WriteMemoryRaw( xVc(0x4086B6), mem, sizeof(mem), true );
+			WriteMemory( xVc(0x4086B6) + 5 + 2, &streaming->cdStreamSyncFuncs.Shutdown, true );
+		}
+
         // We need to know the next model to be read before the CdStreamRead call happens
         if(gvm.IsSA())
         {
@@ -731,3 +813,12 @@ void CAbstractStreaming::Patch()
     }
 }
 
+/*
+*  Export for SilentPatch so it knows that this ML version patches the race condition
+*/
+extern "C" __declspec(dllexport)
+uint32_t CdStreamRaceConditionAware()
+{
+	return 1;
+}
+

src/plugins/gta3/std.stream/streaming.cpp

@@ -6,6 +6,7 @@
  */
 #include <stdinc.hpp>
 #include "streaming.hpp"
+#include "cdstreamsync.inl"
 using namespace modloader;
 
 CAbstractStreaming* streaming;
@@ -17,10 +18,13 @@ CAbstractStreaming* streaming;
 CAbstractStreaming::CAbstractStreaming()
 {
     InitializeCriticalSection(&cs);
+    InitializeCriticalSectionAndSpinCount(&cdStreamSyncLock, 10);
+	cdStreamSyncFuncs = CdStreamSyncFix::InitializeSyncFuncs();
 }
 
 CAbstractStreaming::~CAbstractStreaming()
 {
+    DeleteCriticalSection(&cdStreamSyncLock);
     DeleteCriticalSection(&cs);
     Fastman92LimitAdjusterDestroy(this->f92la);
 }

src/plugins/gta3/std.stream/streaming.hpp

@@ -20,6 +20,8 @@
 #include <traits/gta3/vc.hpp>
 #include <traits/gta3/iii.hpp>
 
+#include "cdstreamsync.hpp"
+
 using namespace modloader;
 
 // Streaming file type
@@ -124,6 +126,8 @@ class CAbstractStreaming
     public: // Friends
         template<class T> friend class Refresher;
         friend int __stdcall CdStreamThread();
+		friend void __stdcall CdStreamShutdownSync_Stub( CdStream* stream, size_t idx );
+		friend uint32_t CdStreamSync( int32_t streamID );
 
     private:
         LibF92LA f92la;                             //
@@ -134,6 +138,9 @@ class CAbstractStreaming
         std::string fbuffer;                        // File buffer to avoid a dynamic allocation everytime we open a model
         std::list<const modloader::file*> imgFiles; // List of img files imported with Mod Loader
 
+        CRITICAL_SECTION cdStreamSyncLock;			// Used to bugfix a deadlock in CdStream
+		CdStreamSyncFix::SyncFuncs cdStreamSyncFuncs;	// Initialize/Finalize/Sleep/Wake functions for CdStream synchronization
+
     public:
         // Basic types
         using id_t      = uint32_t;     // should have sizeof(int) to allow -1 comparision

src/shared/cdstreamsync.hpp

@@ -0,0 +1,64 @@
+#pragma once
+#include <windows.h>
+#include "CdStreamInfo.h"
+
+struct CdStream;
+
+namespace CdStreamSyncFix
+{
+	union SyncObj
+	{
+		HANDLE semaphore;
+		CONDITION_VARIABLE cv;
+	};
+
+	struct SyncFuncs
+	{
+		SyncObj (__stdcall* Initialize)();
+		void (__stdcall* Shutdown)( CdStream* stream );
+		void (__stdcall* SleepCS)( CdStream* stream, PCRITICAL_SECTION critSec );
+		void (__stdcall* Wake)( CdStream* stream );
+	};
+
+	namespace Sema
+	{
+		SyncObj __stdcall Initialize();
+		void __stdcall Shutdown( CdStream* stream );
+		void __stdcall SleepCS( CdStream* stream, PCRITICAL_SECTION critSec );
+		void __stdcall Wake( CdStream* stream );
+	}
+
+	namespace CV
+	{
+		SyncObj __stdcall Initialize();
+		void __stdcall Shutdown( CdStream* stream );
+		void __stdcall SleepCS( CdStream* stream, PCRITICAL_SECTION critSec );
+		void __stdcall Wake( CdStream* stream );
+	}
+
+	bool TryInitCV();
+
+	inline SyncFuncs InitializeSyncFuncs()
+	{
+		SyncFuncs funcs;
+		if ( TryInitCV() )
+		{
+			using namespace CV;
+			funcs.Initialize = Initialize;
+			funcs.Shutdown = Shutdown;
+			funcs.SleepCS = SleepCS;
+			funcs.Wake = Wake;
+		}
+		else
+		{
+			using namespace Sema;
+			funcs.Initialize = Initialize;
+			funcs.Shutdown = Shutdown;
+			funcs.SleepCS = SleepCS;
+			funcs.Wake = Wake;
+		}
+		return funcs;
+	}
+}
+
+static_assert(sizeof(CdStreamSyncFix::SyncObj) == sizeof(HANDLE), "Incorrect struct size: CdStreamSync::SyncObj");