Fix crash blending death animations from unloaded groups - #5284
Open
TheCrazy17 wants to merge 1 commit into
Open
Fix crash blending death animations from unloaded groups#5284TheCrazy17 wants to merge 1 commit into
TheCrazy17 wants to merge 1 commit into
Conversation
CAnimManager::BlendAnimation indexes the association groups array and the group's animations with no checks, so an out of range group id or a group whose block isn't streamed in hands back a garbage pointer. The existing clump check hook now validates both and returns null, CTaskSimpleDie::StartAnim retries on that null, and Kill checks the anim ids it receives from the wire.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CAnimManager::BlendAnimation indexes the association groups array and the group's own animation array with no checks. An out of range group id makes it walk a group read from unmapped memory, and a valid group whose IFP block isn't streamed in hands back a garbage pointer; either way whoever dereferences the result crashes. Peds dying in batches hit this reliably, since the death task requests its animation the moment it starts, whether the block is resident or not.
The fix has three small parts: the existing clump check hook on BlendAnimation now also validates the group and animation ids and returns null on a bad request, CTaskSimpleDie::StartAnim handles that null by leaving the association empty so ProcessPed retries on the next tick, and CClientPed::Kill validates the anim ids it receives from the wire before building the death task.
Known limitation: a ped whose animation block never becomes available skips its death animation and dies in its last pose, which beats taking the whole game crash with it.
Without fix: https://streamable.com/f7g7u4
Motivation
Fixes #447.
Despite the original title, setWaterLevel itself was never the problem; raising the water is just what makes a whole batch of peds drown and request death animations at the same instant. Any spawn of many peds dying at once can trigger the same crash.
Test plan
Checklist