Skip to content

Fix use-after-free of the clothes TXD buffer after engineImportTXD - #5357

Open
Flashmyname wants to merge 1 commit into
multitheftauto:masterfrom
Flashmyname:fix/clothing-txd-buffer-ownership
Open

Fix use-after-free of the clothes TXD buffer after engineImportTXD#5357
Flashmyname wants to merge 1 commit into
multitheftauto:masterfrom
Flashmyname:fix/clothing-txd-buffer-ownership

Conversation

@Flashmyname

Copy link
Copy Markdown
Contributor

Summary

AddClothingTexture hands m_FileData.data() to ClothesAddFile, which stores the raw pointer (ClothesReplacing.cpp:164), but never marks the buffer as borrowed. A later engineImportTXD on a normal model frees it at CClientTXD.cpp:135.

The flag is now set when ClothesAddFile succeeds, and the three bools in CClientTXD get default initialisers.

Motivation

The streaming hook finds the freed pointer by name (ClothesReplacing.cpp:267) and passes it to the model loader (:286). ClothesRemoveFile matches by pointer (:186), so the destructor cleans up nothing and the name stays registered - every later engineAddClothingTXD with it fails with "Texture already added".

m_bUsingFileDataForClothes was also read at :134 before anything wrote it - the constructor sets only m_pManager - so the failure was build dependent.

Two intended consequences: the raw bytes now live until the element is destroyed, which is the contract :104-106 already imposes, and a clothes import after a non-clothes import now works instead of failing at :84.

CClientDFF looks the same on its raw data path: AddClothingModel registers m_RawDataBuffer.data() (CClientDFF.cpp:116), GetLoadedClump frees the buffer at :66, and RestoreModels then deregisters a pointer that no longer matches (:253). I haven't tested that path, and fixing it means touching the free that limits a raw data DFF to a single replace, so I've left it out of this one.

Test plan

Three cycles of engineLoadTXD, engineAddClothingTXD with a fresh name, optionally engineImportTXD(txd, 1337), then destroyElement - one cycle per frame, since destroyElement only queues the element for CElementDeleter.

                          cycle 1  2       3
before  without import    ok       ok      ok
        with import       ok       FAILED  FAILED
after   without import    ok       ok      ok
        with import       ok       ok      ok

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