-
-
Notifications
You must be signed in to change notification settings - Fork 562
Fix hugely popular crash in RpAnimBlendAllocateData (SA 0x000D5F6F) #4894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2832,29 +2832,32 @@ static void __declspec(naked) HOOK_CVolumetricShadowMgr_Update() | |
| //////////////////////////////////////////////////////////////////////// | ||
| // CAnimManager::CreateAnimAssocGroups | ||
| // | ||
| // CModelInfo::ms_modelInfoPtrs at the given index is a null pointer | ||
| // Missing model info or RwObject before the clump is used for associations | ||
| //////////////////////////////////////////////////////////////////////// | ||
| void OnMY_CAnimManager_CreateAnimAssocGroups(uint uiModelId) | ||
| bool OnMY_CAnimManager_CreateAnimAssocGroups(uint uiModelId) | ||
| { | ||
| CModelInfo* pModelInfo = pGameInterface->GetModelInfo(uiModelId); | ||
| CBaseModelInfoSAInterface* pInterface = pModelInfo ? pModelInfo->GetInterface() : nullptr; | ||
| if (!pInterface || pInterface->pRwObject == nullptr) | ||
| if (!pInterface || !pInterface->pRwObject) | ||
| { | ||
| // Crash will occur at offset 00349b7b | ||
| OnCrashAverted(816); | ||
| LogEvent(816, "Model not loaded", "CAnimManager_CreateAnimAssocGroups", SString("No RwObject for model:%d", uiModelId), 5416); | ||
| CArgMap argMap; | ||
| argMap.Set("id", uiModelId); | ||
| argMap.Set("reason", "createanim"); | ||
| SetApplicationSetting("diagnostics", "gta-model-fail", argMap.ToString()); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // Hook info | ||
| #define HOOKPOS_CAnimManager_CreateAnimAssocGroups 0x4D3D52 | ||
| #define HOOKSIZE_CAnimManager_CreateAnimAssocGroups 5 | ||
| #define HOOKCHECK_CAnimManager_CreateAnimAssocGroups 0x8B | ||
| DWORD RETURN_CAnimManager_CreateAnimAssocGroups = 0x4D3D59; | ||
| static void __declspec(naked) HOOK_CAnimManager_CreateAnimAssocGroups() | ||
| DWORD RETURN_CAnimManager_CreateAnimAssocGroups = 0x4D3D59; | ||
| DWORD RETURN_CAnimManager_CreateAnimAssocGroups_Skip = 0x4D3D71; | ||
| void _declspec(naked) HOOK_CAnimManager_CreateAnimAssocGroups() | ||
| { | ||
| MTA_VERIFY_HOOK_LOCAL_SIZE; | ||
|
|
||
|
|
@@ -2865,15 +2868,91 @@ static void __declspec(naked) HOOK_CAnimManager_CreateAnimAssocGroups() | |
| push eax | ||
| call OnMY_CAnimManager_CreateAnimAssocGroups | ||
| add esp, 4*1 | ||
| test al, al | ||
| popad | ||
| jz skipCreateInstance | ||
|
|
||
| // Replaced code | ||
| // Replaced code | ||
| push ecx | ||
| mov ecx, dword ptr[ARRAY_ModelInfo] | ||
| mov eax, dword ptr[ecx + eax*4] | ||
| pop ecx | ||
|
|
||
| jmp RETURN_CAnimManager_CreateAnimAssocGroups | ||
|
|
||
| skipCreateInstance: | ||
| xor ebx, ebx | ||
| jmp RETURN_CAnimManager_CreateAnimAssocGroups_Skip | ||
| } | ||
| // clang-format on | ||
| } | ||
|
|
||
| void OnMY_CAnimBlendAssocGroup_CreateAssociations(CBaseModelInfoSAInterface* pModelInfo) | ||
| { | ||
| OnCrashAverted(816); | ||
|
|
||
| int iModelId = -1; | ||
| CBaseModelInfoSAInterface** ppModelInfo = (CBaseModelInfoSAInterface**)ARRAY_ModelInfo; | ||
| const int maximumModelId = pGameInterface->GetBaseIDforTXD(); | ||
| for (int i = 0; i < maximumModelId; i++) | ||
| { | ||
| if (ppModelInfo[i] == pModelInfo) | ||
| { | ||
| iModelId = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| LogEvent(816, "Model not loaded", "CAnimBlendAssocGroup_CreateAssociations", SString("No RwObject for model:%d", iModelId), 5416); | ||
| CArgMap argMap; | ||
| argMap.Set("id", iModelId); | ||
| argMap.Set("reason", "createassoc"); | ||
| SetApplicationSetting("diagnostics", "gta-model-fail", argMap.ToString()); | ||
| } | ||
|
|
||
| #define HOOKPOS_CAnimBlendAssocGroup_CreateAssociations 0x4CE2F7 | ||
| #define HOOKSIZE_CAnimBlendAssocGroup_CreateAssociations 7 | ||
| #define HOOKCHECK_CAnimBlendAssocGroup_CreateAssociations 0x8B | ||
| DWORD RETURN_CAnimBlendAssocGroup_CreateAssociations = 0x4CE2FE; | ||
| DWORD RETURN_CAnimBlendAssocGroup_CreateAssociations_Skip = 0x4CE36F; | ||
| void _declspec(naked) HOOK_CAnimBlendAssocGroup_CreateAssociations() | ||
|
Comment on lines
+2913
to
+2918
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please could you write some comments explaining exactly what the hook does?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Dutchman101 please can you add the requested comments here and we'll get this merged |
||
| { | ||
| // clang-format off | ||
|
|
||
| MTA_VERIFY_HOOK_LOCAL_SIZE; | ||
| __asm | ||
| { | ||
| // CModelInfo::GetModelInfoFromHashKey returned the model info for | ||
| // this animation. SA already skips the entry when it returns null. | ||
| test eax, eax | ||
| jz skipCreateAssociation | ||
|
|
||
| // CBaseModelInfo::pRwObject is at +0x1C. The original instructions | ||
| // immediately call CBaseModelInfo::CreateInstance at vtable+0x2C. | ||
| cmp dword ptr[eax+1Ch], 0 | ||
| jnz continueCreateAssociation | ||
|
|
||
| // CreateInstance requires the RenderWare object. Record its absence | ||
| // and follow SA's existing per-animation skip path. | ||
| pushad | ||
| push eax | ||
| call OnMY_CAnimBlendAssocGroup_CreateAssociations | ||
| add esp, 4*1 | ||
| popad | ||
| jmp skipCreateAssociation | ||
|
|
||
| continueCreateAssociation: | ||
| // Restore the seven overwritten bytes: load the vtable, pass the | ||
| // model info as this, then call CreateInstance. | ||
| mov edx, [eax] | ||
| mov ecx, eax | ||
| call dword ptr[edx+2Ch] | ||
| jmp RETURN_CAnimBlendAssocGroup_CreateAssociations | ||
|
|
||
| skipCreateAssociation: | ||
| // 0x4CE36F increments the created-association count and advances | ||
| // the animation and static-association indices for the next entry. | ||
| jmp RETURN_CAnimBlendAssocGroup_CreateAssociations_Skip | ||
| } | ||
| // clang-format on | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes here seem to be guarding against
m_pModelInfopointing to invalid data, but do we know why it's invalid in the first place?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Seems like a use after free or something?)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qaisjp
No, we don't know it.. the 'fix' acts on an emerging crash as per the crash stats. There's users complaining of this crash as well, but none of them could pinpoint the exact circumstances. After i couldn't find a 100% confirmed root cause, i just looked at, and prevented, additional paths i could find for it to occur.
Averting crashes based on stats and collected dumps alone is an established practice, we don't always get much to work with.