Skip to content

Commit e5e308d

Browse files
committed
1.6: GTA streamer bug-fix round #1. This heavily reduces bug load, which is very limiting/blocking.
"Bug load" indicates that MTA uses hacks nad workarounds until now to avoid mostly race conditions from SA. Adds streaming GC protection and safety for collision/model access Fix SA streaming garbage collector race conditions - New CStreamingGC class with guard for model protection - New CColModelGuard wrapper for collision model reference counting - Streaming removal callbacks to prevent GC during critical operations - Thread-safe model protection - CVehicleSA: Guard collision access in suspension line operations - CRendererSA: Protect RwObject during rendering with ModelAddRef/RemoveRef - CRenderWareSA: Validate pointers before callbacks, destroy removed atomics properly - CPhysicalSA: Validate collision model before accessing radius in GetBoundRect_ - CColModelSA: Exception safety in constructor/destructor - CModelInfoSA: Add IsCollisionLoaded/IsRwObjectLoaded - CFileLoaderSA: Use type-safe callback shim instead of reinterpret_cast Fixes prevent these too: 1. Collision models being GC'd during suspension line calculations 2. RwObjects being freed during rendering 3. TXD reference leaks during texture swapping 4. Model removal during active entity operations 5. Invalid pointer access after streaming unload # Conflicts: # Client/game_sa/CColModelSA.cpp # Client/game_sa/CWorldSA.cpp
1 parent 3af8182 commit e5e308d

26 files changed

+883
-90
lines changed

Client/game_sa/CAnimManagerSA.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ int CAnimManagerSA::GetNumAnimAssocDefinitions()
7171
return *(int*)(VAR_CAnimManager_NumAnimAssocDefinitions);
7272
}
7373

74+
// Returns wrapper for animation hierarchy
7475
std::unique_ptr<CAnimBlendHierarchy> CAnimManagerSA::GetAnimation(int ID)
7576
{
7677
CAnimBlendHierarchySAInterface* pInterface = nullptr;
@@ -499,6 +500,7 @@ void CAnimManagerSA::RemoveAnimBlock(int ID)
499500
}
500501
}
501502

503+
// Returns a pointer to GTA SA's internal animation association definition
502504
AnimAssocDefinition* CAnimManagerSA::AddAnimAssocDefinition(const char* szBlockName, const char* szAnimName, AssocGroupId animGroup, AnimationId animID,
503505
AnimDescriptor* pDescriptor)
504506
{

Client/game_sa/CColModelGuard.h

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/*****************************************************************************
2+
*
3+
* PROJECT: Multi Theft Auto v1.0
4+
* LICENSE: See LICENSE in the top level directory
5+
* FILE: game_sa/CColModelGuard.h
6+
* PURPOSE: Wrapper for collision model access with automatic reference counting
7+
*
8+
* Multi Theft Auto is available from https://www.multitheftauto.com/
9+
*
10+
*****************************************************************************/
11+
12+
#pragma once
13+
14+
#include "CModelInfoSA.h"
15+
16+
17+
// Guard: Protects collision model from streaming GC
18+
class CColModelGuard
19+
{
20+
public:
21+
22+
// Increment ref count for pModelInfo's collision
23+
explicit CColModelGuard(CModelInfoSA* pModelInfo) : m_pModelInfo(pModelInfo), m_bValid(false), m_pColModel(nullptr), m_pColData(nullptr)
24+
{
25+
if (!m_pModelInfo)
26+
return;
27+
28+
// Check and protect collision atomically
29+
if (m_pModelInfo->IsCollisionLoaded())
30+
{
31+
m_pModelInfo->AddColRef();
32+
bool bRefAdded = true;
33+
34+
try
35+
{
36+
// Cache pointers while protected
37+
CBaseModelInfoSAInterface* pInterface = m_pModelInfo->GetInterface();
38+
if (pInterface && pInterface->pColModel)
39+
{
40+
m_pColModel = pInterface->pColModel;
41+
m_pColData = m_pColModel->m_data;
42+
m_bValid = (m_pColData != nullptr);
43+
}
44+
45+
// Release ref if validation failed
46+
if (!m_bValid)
47+
{
48+
m_pModelInfo->RemoveColRef();
49+
bRefAdded = false;
50+
}
51+
}
52+
catch (...)
53+
{
54+
// Release reference on exception
55+
if (bRefAdded)
56+
m_pModelInfo->RemoveColRef();
57+
throw;
58+
}
59+
60+
// Prevent double-release in destructor
61+
if (!bRefAdded)
62+
m_pModelInfo = nullptr;
63+
}
64+
}
65+
66+
// Decrement ref count on destruction
67+
~CColModelGuard() noexcept
68+
{
69+
if (m_bValid && m_pModelInfo)
70+
{
71+
m_pModelInfo->RemoveColRef();
72+
}
73+
}
74+
75+
// Prevent copying
76+
CColModelGuard(const CColModelGuard&) = delete;
77+
CColModelGuard& operator=(const CColModelGuard&) = delete;
78+
79+
// Allow moving
80+
CColModelGuard(CColModelGuard&& other) noexcept
81+
: m_pModelInfo(other.m_pModelInfo), m_bValid(other.m_bValid), m_pColModel(other.m_pColModel), m_pColData(other.m_pColData)
82+
{
83+
// Transfer ownership
84+
other.m_pModelInfo = nullptr;
85+
other.m_bValid = false;
86+
other.m_pColModel = nullptr;
87+
other.m_pColData = nullptr;
88+
}
89+
90+
CColModelGuard& operator=(CColModelGuard&& other) noexcept
91+
{
92+
if (this != &other)
93+
{
94+
// Release current, transfer from source
95+
if (m_bValid && m_pModelInfo)
96+
m_pModelInfo->RemoveColRef();
97+
98+
// Transfer ownership from source
99+
m_pModelInfo = other.m_pModelInfo;
100+
m_bValid = other.m_bValid;
101+
m_pColModel = other.m_pColModel;
102+
m_pColData = other.m_pColData;
103+
104+
// Prevent double-release
105+
other.m_pModelInfo = nullptr;
106+
other.m_bValid = false;
107+
other.m_pColModel = nullptr;
108+
other.m_pColData = nullptr;
109+
}
110+
return *this;
111+
}
112+
113+
// Check if collision is protected
114+
[[nodiscard]] bool IsValid() const noexcept { return m_bValid; }
115+
116+
// Get collision data (check IsValid first)
117+
// Returns cached pointer or nullptr
118+
[[nodiscard]] CColDataSA* GetColData() const noexcept { return m_bValid ? m_pColData : nullptr; }
119+
120+
// Get collision model (check IsValid first)
121+
// Returns cached pointer or nullptr
122+
[[nodiscard]] CColModelSAInterface* GetColModel() const noexcept { return m_bValid ? m_pColModel : nullptr; }
123+
124+
explicit operator bool() const noexcept { return m_bValid; }
125+
126+
private:
127+
CModelInfoSA* m_pModelInfo;
128+
bool m_bValid;
129+
CColModelSAInterface* m_pColModel; // Cached during construction
130+
CColDataSA* m_pColData; // Cached during construction
131+
};

Client/game_sa/CColModelSA.cpp

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,31 +17,62 @@ CColModelSA::CColModelSA()
1717
m_pInterface = new CColModelSAInterface;
1818
DWORD dwThis = (DWORD)m_pInterface;
1919
DWORD dwFunc = FUNC_CColModel_Constructor;
20-
_asm
20+
21+
try
2122
{
22-
mov ecx, dwThis
23-
call dwFunc
23+
_asm
24+
{
25+
mov ecx, dwThis
26+
call dwFunc
27+
}
28+
}
29+
catch (...)
30+
{
31+
// Clean up on constructor failure
32+
delete m_pInterface;
33+
m_pInterface = nullptr;
34+
throw;
2435
}
36+
2537
m_bDestroyInterface = true;
38+
m_bValid = true;
2639
}
2740

2841
CColModelSA::CColModelSA(CColModelSAInterface* pInterface)
2942
{
3043
m_pInterface = pInterface;
3144
m_bDestroyInterface = false;
45+
m_bValid = true;
3246
}
3347

3448
CColModelSA::~CColModelSA()
3549
{
36-
if (m_bDestroyInterface)
50+
m_bValid = false;
51+
52+
if (m_bDestroyInterface && m_pInterface)
3753
{
3854
DWORD dwThis = (DWORD)m_pInterface;
3955
DWORD dwFunc = FUNC_CColModel_Destructor;
40-
_asm
56+
57+
try
4158
{
42-
mov ecx, dwThis
43-
call dwFunc
59+
_asm
60+
{
61+
mov ecx, dwThis
62+
call dwFunc
63+
}
64+
}
65+
catch (...)
66+
{
67+
// Ensure cleanup completes on exception
4468
}
69+
4570
delete m_pInterface;
71+
m_pInterface = nullptr;
4672
}
4773
}
74+
75+
void CColModelSA::Destroy()
76+
{
77+
delete this;
78+
}

Client/game_sa/CColModelSA.h

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,12 @@ struct CSphereSA
6565
{
6666
CVector m_center;
6767
float m_radius;
68+
69+
// Validate radius is finite and non-negative
70+
bool IsValidRadius() const
71+
{
72+
return std::isfinite(m_radius) && m_radius >= 0.0f && m_radius < 1000000.0f;
73+
}
6874
};
6975
static_assert(sizeof(CSphereSA) == 0x10, "Invalid size for CSphereSA");
7076

@@ -101,6 +107,12 @@ struct CColSphereSA : CSphereSA
101107
CSphereSA{ sp }
102108
{
103109
}
110+
111+
// Validate material enum is in valid range
112+
bool IsValidMaterial() const
113+
{
114+
return static_cast<std::uint8_t>(m_material) < static_cast<std::uint8_t>(EColSurfaceValue::SIZE);
115+
}
104116
};
105117
static_assert(sizeof(CColSphereSA) == 0x14, "Invalid size for CColSphereSA");
106118

@@ -109,6 +121,20 @@ struct CColTriangleSA
109121
std::uint16_t m_indices[3];
110122
EColSurface m_material;
111123
CColLighting m_lighting;
124+
125+
// Validate material enum is in valid range
126+
bool IsValidMaterial() const
127+
{
128+
return static_cast<std::uint8_t>(m_material) < static_cast<std::uint8_t>(EColSurfaceValue::SIZE);
129+
}
130+
131+
// Validate indices are within vertex count bounds
132+
bool IsValidIndices(std::uint16_t numVertices) const
133+
{
134+
return m_indices[0] < numVertices &&
135+
m_indices[1] < numVertices &&
136+
m_indices[2] < numVertices;
137+
}
112138
};
113139
static_assert(sizeof(CColTriangleSA) == 0x8, "Invalid size for CColTriangleSA");
114140

@@ -178,10 +204,12 @@ class CColModelSA : public CColModel
178204
CColModelSA(CColModelSAInterface* pInterface);
179205
~CColModelSA();
180206

181-
CColModelSAInterface* GetInterface() override { return m_pInterface; }
182-
void Destroy() override { delete this; }
207+
CColModelSAInterface* GetInterface() override { return m_bValid ? m_pInterface : nullptr; }
208+
void Destroy() override;
209+
bool IsValid() const { return m_bValid; }
183210

184211
private:
185212
CColModelSAInterface* m_pInterface;
186213
bool m_bDestroyInterface;
214+
bool m_bValid;
187215
};

Client/game_sa/CCoronasSA.cpp

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,22 @@ CRegisteredCorona* CCoronasSA::FindCorona(DWORD Identifier)
8383

8484
RwTexture* CCoronasSA::GetTexture(CoronaType type)
8585
{
86-
if ((DWORD)type < MAX_CORONA_TEXTURES)
87-
return (RwTexture*)(*(DWORD*)(ARRAY_CORONA_TEXTURES + static_cast<DWORD>(type) * sizeof(DWORD)));
88-
else
89-
return NULL;
86+
// Validate enum is within valid range
87+
if (static_cast<DWORD>(type) >= MAX_CORONA_TEXTURES) [[unlikely]]
88+
return nullptr;
89+
90+
// Read texture pointer from array with validation
91+
DWORD* pTextureArray = reinterpret_cast<DWORD*>(ARRAY_CORONA_TEXTURES);
92+
if (!pTextureArray) [[unlikely]]
93+
return nullptr;
94+
95+
DWORD textureAddr = pTextureArray[static_cast<DWORD>(type)];
96+
if (!textureAddr) [[unlikely]]
97+
return nullptr;
98+
99+
RwTexture* pTexture = reinterpret_cast<RwTexture*>(textureAddr);
100+
101+
return pTexture;
90102
}
91103

92104
void CCoronasSA::DisableSunAndMoon(bool bDisabled)

Client/game_sa/CEntitySA.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,7 @@ class CEntitySA : public virtual CEntity
237237

238238
CEntitySA();
239239
CEntitySAInterface* GetInterface() { return m_pInterface; };
240+
CEntitySAInterface* GetInterface() const { return m_pInterface; };
240241
void SetInterface(CEntitySAInterface* intInterface) { m_pInterface = intInterface; };
241242

242243
bool IsPed() { return GetEntityType() == ENTITY_TYPE_PED; }

Client/game_sa/CFileLoaderSA.cpp

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,14 @@ static void CVehicleModelInfo_StopUsingCommonVehicleTexDicationary()
127127
static auto CModelInfo_ms_modelInfoPtrs = (CBaseModelInfoSAInterface**)ARRAY_ModelInfo;
128128
static unsigned int& gAtomicModelId = *reinterpret_cast<unsigned int*>(DWORD_AtomicsReplacerModelID);
129129

130+
namespace
131+
{
132+
bool RelatedModelInfoShim(RpAtomic* atomic, void* context)
133+
{
134+
return CFileLoader_SetRelatedModelInfoCB(atomic, static_cast<SRelatedModelInfo*>(context)) != nullptr;
135+
}
136+
}
137+
130138
bool CFileLoader_LoadAtomicFile(RwStream* stream, unsigned int modelId)
131139
{
132140
CBaseModelInfoSAInterface* pBaseModelInfo = CModelInfo_ms_modelInfoPtrs[modelId];
@@ -157,7 +165,7 @@ bool CFileLoader_LoadAtomicFile(RwStream* stream, unsigned int modelId)
157165
relatedModelInfo.pClump = pReadClump;
158166
relatedModelInfo.bDeleteOldRwObject = false;
159167

160-
RpClumpForAllAtomics(pReadClump, reinterpret_cast<RpClumpForAllAtomicsCB_t>(CFileLoader_SetRelatedModelInfoCB), &relatedModelInfo);
168+
RpClumpForAllAtomics(pReadClump, RelatedModelInfoShim, &relatedModelInfo);
161169
RpClumpDestroy(pReadClump);
162170
}
163171

Client/game_sa/CGameSA.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
#include "CRopesSA.h"
4848
#include "CSettingsSA.h"
4949
#include "CStatsSA.h"
50+
#include "CStreamingGC.h"
5051
#include "CTaskManagementSystemSA.h"
5152
#include "CTasksSA.h"
5253
#include "CVisibilityPluginsSA.h"
@@ -277,6 +278,9 @@ CGameSA::CGameSA()
277278

278279
CGameSA::~CGameSA()
279280
{
281+
// Shutdown streaming GC hooks
282+
CStreamingGC::Shutdown();
283+
280284
delete reinterpret_cast<CPlayerInfoSA*>(m_pPlayerInfo);
281285

282286
for (int i = 0; i < NUM_WeaponInfosTotal; i++)
@@ -493,10 +497,18 @@ void CGameSA::Initialize()
493497
SetupBrokenModels();
494498
m_pRenderWare->Initialize();
495499

500+
// Initialize streaming GC protection hooks
501+
CStreamingGC::Initialize();
502+
496503
// *Sebas* Hide the GTA:SA Main menu.
497504
MemPutFast<BYTE>(CLASS_CMenuManager + 0x5C, 0);
498505
}
499506

507+
StreamingRemoveModelCallback CGameSA::GetStreamingRemoveModelCallback() const noexcept
508+
{
509+
return &CStreamingGC::OnRemoveModel;
510+
}
511+
500512
eGameVersion CGameSA::GetGameVersion()
501513
{
502514
return m_eGameVersion;

Client/game_sa/CGameSA.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,8 @@ class CGameSA : public CGame
313313
void SetTaskSimpleBeHitHandler(TaskSimpleBeHitHandler* pTaskSimpleBeHitHandler) { m_pTaskSimpleBeHitHandler = pTaskSimpleBeHitHandler; }
314314
CAnimBlendClumpDataSAInterface** GetClumpData(RpClump* clump) { return RWPLUGINOFFSET(CAnimBlendClumpDataSAInterface*, clump, ClumpOffset); }
315315

316+
StreamingRemoveModelCallback GetStreamingRemoveModelCallback() const noexcept override;
317+
316318
PreWeaponFireHandler* m_pPreWeaponFireHandler;
317319
PostWeaponFireHandler* m_pPostWeaponFireHandler;
318320
TaskSimpleBeHitHandler* m_pTaskSimpleBeHitHandler;

0 commit comments

Comments
 (0)