Fix crashes and TXD slot leaks in model cleanup#5042
Open
HeresHavi wants to merge 1 commit into
Open
Conversation
Invalid model and TXD IDs could cause an out-of-bounds lookup and crash the client. Additionally, engineFreeTXD reported success without releasing the allocated TXD slot. This change validates IDs and routes TXD cleanup through the established model-manager removal path.
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
Added lower-bound and overflow-safe ID checks to
engineFreeModel,engineFreeTXD, and the shared model lookup.Valid TXD cleanup now uses the model manager's removal path so affected models are reset, the allocation is removed, and the pool slot can be reused.
Motivation
CClientModelManager::FindModelByIDchecked only the upper boundary. A negative model ID could therefore index memory before the model array. Large TXD IDs could also wrap into a negative internal model ID before reaching the same lookup.For example, a resource might cache custom model and TXD IDs so it can free them during a restart. If an invalid model ID or wrapped TXD ID reaches cleanup, the old lookup could crash every player running that resource.
Valid
engineFreeTXDcalls also returnedtruewithout fully removing the allocation. The TXD slot remained occupied for the rest of the client session instead of becoming reusable.engineFreeModel Crash Stack
engineFreeTXD Crash Stack
Test plan
Runtime
engineFreeModel(-1000000)caused an access violation inCClientModelManager::FindModelByID.engineFreeTXD(4293947296)wrapped into the same invalid lookup and crashed at the same client offset.false, and the client stayed open.332and TXD ID3608were freed successfully.3608reset the model using it, and the next TXD allocation reused slot3608.false.Builds and Tests
Debug | Win32: Solution build passed, 304 client tests passed.Release | Win32: Solution build passed, 304 client tests passed.clang-format.Checklist