Fix use-after-free of the clothes TXD buffer after engineImportTXD - #5357
Open
Flashmyname wants to merge 1 commit into
Open
Fix use-after-free of the clothes TXD buffer after engineImportTXD#5357Flashmyname wants to merge 1 commit into
Flashmyname wants to merge 1 commit into
Conversation
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
AddClothingTexturehandsm_FileData.data()toClothesAddFile, which stores the raw pointer (ClothesReplacing.cpp:164), but never marks the buffer as borrowed. A laterengineImportTXDon a normal model frees it atCClientTXD.cpp:135.The flag is now set when
ClothesAddFilesucceeds, and the three bools inCClientTXDget default initialisers.Motivation
The streaming hook finds the freed pointer by name (
ClothesReplacing.cpp:267) and passes it to the model loader (:286).ClothesRemoveFilematches by pointer (:186), so the destructor cleans up nothing and the name stays registered - every laterengineAddClothingTXDwith it fails with "Texture already added".m_bUsingFileDataForClotheswas also read at:134before anything wrote it - the constructor sets onlym_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-106already imposes, and a clothes import after a non-clothes import now works instead of failing at:84.CClientDFFlooks the same on its raw data path:AddClothingModelregistersm_RawDataBuffer.data()(CClientDFF.cpp:116),GetLoadedClumpfrees the buffer at:66, andRestoreModelsthen 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,engineAddClothingTXDwith a fresh name, optionallyengineImportTXD(txd, 1337), thendestroyElement- one cycle per frame, sincedestroyElementonly queues the element forCElementDeleter.Checklist