Skip to content

Fix out of bounds write when replacing a model with a DFF that has _dam parts - #5356

Open
Flashmyname wants to merge 1 commit into
multitheftauto:masterfrom
Flashmyname:fix/dam-atomic-type-confusion
Open

Fix out of bounds write when replacing a model with a DFF that has _dam parts#5356
Flashmyname wants to merge 1 commit into
multitheftauto:masterfrom
Flashmyname:fix/dam-atomic-type-confusion

Conversation

@Flashmyname

Copy link
Copy Markdown
Contributor

Summary

The _dam branch of CFileLoader_SetRelatedModelInfoCB casts the model info to CDamagableModelInfo without checking. SetDamagedAtomic writes to [this+0x20], which only exists on the damageable subclass, and CBaseModelInfoSAInterface is 0x20 bytes - so on a plain atomic model the write lands past the end of the allocation, on the next model info's vtable pointer.

The game dispatches through AsDamageAtomicModelInfoPtr (vtable slot 2) first. That call is restored, the atomic is left with the clump the caller destroys, and AtomicsReplacer skips its TXD release since nothing took a reference.

Motivation

Reachable from engineReplaceModel with any DFF whose frame name ends in _dam when the target model is not damageable. Slot 2 returns null for a plain atomic (0x4C4A90) and this for a damageable one (0x4C55C0).

Test plan

A vehicle DFF onto model 2232 (plain atomic) and onto 996 (damageable, as a control), logging the dword at modelinfo+0x20:

        model  slot2     pre       post
before  2232   00000000  0085BBF0  2B289380   overwritten, once per _dam atomic
        996    00B1C07C  00000000  2B11CD54   correct
after   2232   00000000  0085BBF0  0085BBF0   untouched
        996    00B1C07C  00000000  2C562158   correct

0085BBF0 is the next model info's vtable pointer. The unfixed client died on it: 0xC0000005, EIP=00000000, ECX=00B154D4 (the address written to), EDX=2B288E08 (the value written).

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.

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.

1 participant