bugfix(audio): Prevent race condition when accessing deleted drawables from audio thread

Author: seer-by-sentry[bot]

Core/GameEngine/Source/Common/Audio/AudioEventRTS.cpp

@@ -56,6 +56,7 @@
 
 #include "GameClient/Drawable.h"	// For getPosition
 #include "GameClient/GameClient.h"	// For getDrawableByID
+#include "Common/ScopedMutex.h"		// For ScopedMutex
 
 
 //-------------------------------------------------------------------------------------------------
@@ -741,13 +742,21 @@ const Coord3D *AudioEventRTS::getCurrentPosition()
 		return &m_positionOfAudio;
 
 	case OT_Drawable:
-		if (Drawable *draw = TheGameClient->findDrawableByID(m_drawableID))
 		{
-			m_positionOfAudio.set( draw->getPosition() );
-		}
-		else
-		{
-			m_ownerType = OT_Dead;
+			// Hold the drawable lookup mutex for the duration of the find-and-read to prevent
+			// the main thread from destroying the drawable (and freeing its memory) between
+			// findDrawableByID() returning a non-null pointer and getPosition() being called.
+			// This function can be invoked from the Miles audio background thread via
+			// notifyOfAudioCompletion(), so the mutex is needed to close this race window.
+			ScopedMutex mut(TheGameClient->getDrawableLookupMutex());
+			if (Drawable *draw = TheGameClient->findDrawableByID(m_drawableID))
+			{
+				m_positionOfAudio.set( draw->getPosition() );
+			}
+			else
+			{
+				m_ownerType = OT_Dead;
+			}
 		}
 		return &m_positionOfAudio;
 

Generals/Code/GameEngine/Include/GameClient/GameClient.h

@@ -99,6 +99,8 @@ class GameClient : public SubsystemInterface,
 
 	virtual Drawable *findDrawableByID( const DrawableID id );					///< Given an ID, return the associated drawable
 
+	HANDLE getDrawableLookupMutex() const { return m_drawableLookupMutex; }	///< Returns the mutex that guards drawable hash access and drawable lifetime
+
 	void setDrawableIDCounter( DrawableID nextDrawableID ) { m_nextDrawableID = nextDrawableID; }
 	DrawableID getDrawableIDCounter() { return m_nextDrawableID; }
 
@@ -160,6 +162,7 @@ class GameClient : public SubsystemInterface,
 
 	Drawable *m_drawableList;																		///< All of the drawables in the world
 	DrawablePtrHash m_drawableHash;															///< Used for DrawableID lookups
+	HANDLE m_drawableLookupMutex;																///< Guards drawable hash access and drawable lifetime against concurrent audio thread access
 
 	DrawableID m_nextDrawableID;																///< For allocating drawable id's
 	DrawableID allocDrawableID();													///< Returns a new unique drawable id

Generals/Code/GameEngine/Source/GameClient/GameClient.cpp

@@ -33,6 +33,7 @@
 
 // USER INCLUDES //////////////////////////////////////////////////////////////
 #include "Common/ActionManager.h"
+#include "Common/ScopedMutex.h"
 #include "Common/GameEngine.h"
 #include "Common/GameState.h"
 #include "Common/GameUtility.h"
@@ -107,6 +108,8 @@ GameClient::GameClient()
 
 	m_nextDrawableID = (DrawableID)1;
 	TheDrawGroupInfo = new DrawGroupInfo;
+
+	m_drawableLookupMutex = CreateMutex(nullptr, FALSE, nullptr);
 }
 
 //std::vector<std::string>	preloadTextureNamesGlobalHack;
@@ -223,6 +226,9 @@ GameClient::~GameClient()
 	delete TheEva;
 	TheEva = nullptr;
 
+	CloseHandle(m_drawableLookupMutex);
+	m_drawableLookupMutex = nullptr;
+
 }
 
 //-------------------------------------------------------------------------------------------------
@@ -821,11 +827,13 @@ void GameClient::destroyDrawable( Drawable *draw )
 
 	}
 
-	// remove the drawable from our hash of drawables
-	removeDrawableFromLookupTable( draw );
-
-	// free storage
-	deleteInstance(draw);
+	// Remove from hash and free storage under the mutex so that audio threads looking up
+	// drawables by ID cannot observe a drawable pointer after its memory has been freed.
+	{
+		ScopedMutex mut(m_drawableLookupMutex);
+		removeDrawableFromLookupTable( draw );
+		deleteInstance(draw);
+	}
 
 }
 

GeneralsMD/Code/GameEngine/Include/GameClient/GameClient.h

@@ -104,6 +104,8 @@ class GameClient : public SubsystemInterface,
 
 	virtual Drawable *findDrawableByID( const DrawableID id );					///< Given an ID, return the associated drawable
 
+	HANDLE getDrawableLookupMutex() const { return m_drawableLookupMutex; }	///< Returns the mutex that guards drawable lookup access and drawable lifetime
+
 	void setDrawableIDCounter( DrawableID nextDrawableID ) { m_nextDrawableID = nextDrawableID; }
 	DrawableID getDrawableIDCounter() { return m_nextDrawableID; }
 
@@ -181,6 +183,7 @@ class GameClient : public SubsystemInterface,
 	Drawable *m_drawableList;																		///< All of the drawables in the world
 //	DrawablePtrHash m_drawableHash;															///< Used for DrawableID lookups
 	DrawablePtrVector m_drawableVector;
+	HANDLE m_drawableLookupMutex;																///< Guards drawable lookup access and drawable lifetime against concurrent audio thread access
 
 	DrawableID m_nextDrawableID;																///< For allocating drawable id's
 	DrawableID allocDrawableID();													///< Returns a new unique drawable id

GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp

@@ -33,6 +33,7 @@
 
 // USER INCLUDES //////////////////////////////////////////////////////////////
 #include "Common/ActionManager.h"
+#include "Common/ScopedMutex.h"
 #include "Common/GameEngine.h"
 #include "Common/GameState.h"
 #include "Common/GameUtility.h"
@@ -119,6 +120,8 @@ GameClient::GameClient()
 
 	m_nextDrawableID = (DrawableID)1;
 	TheDrawGroupInfo = new DrawGroupInfo;
+
+	m_drawableLookupMutex = CreateMutex(nullptr, FALSE, nullptr);
 }
 
 //std::vector<std::string>	preloadTextureNamesGlobalHack;
@@ -241,6 +244,9 @@ GameClient::~GameClient()
 	delete TheSnowManager;
 	TheSnowManager = nullptr;
 
+	CloseHandle(m_drawableLookupMutex);
+	m_drawableLookupMutex = nullptr;
+
 }
 
 //-------------------------------------------------------------------------------------------------
@@ -884,11 +890,13 @@ void GameClient::destroyDrawable( Drawable *draw )
 
 	}
 
-	// remove the drawable from our hash of drawables
-	removeDrawableFromLookupTable( draw );
-
-	// free storage
-	deleteInstance(draw);
+	// Remove from lookup and free storage under the mutex so that audio threads looking up
+	// drawables by ID cannot observe a drawable pointer after its memory has been freed.
+	{
+		ScopedMutex mut(m_drawableLookupMutex);
+		removeDrawableFromLookupTable( draw );
+		deleteInstance(draw);
+	}
 
 }