Skip to content

Fix crash blending death animations from unloaded groups - #5284

Open
TheCrazy17 wants to merge 1 commit into
multitheftauto:masterfrom
TheCrazy17:fix/death-anim-crash
Open

Fix crash blending death animations from unloaded groups#5284
TheCrazy17 wants to merge 1 commit into
multitheftauto:masterfrom
TheCrazy17:fix/death-anim-crash

Conversation

@TheCrazy17

Copy link
Copy Markdown
Contributor

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

  1. Spawn a large batch of peds and raise the water level so they all drown at once.
  2. Before this change the client crashed in the animation lookup; now they all die normally.

Checklist

  • Your code should follow the coding guidelines.
  • Smaller pull requests are easier to review. If your pull request is beefy, your pull request should be reviewable commit-by-commit.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setWaterLevel causes clientcrash after some time

1 participant