Skip to content

fix: Properly Drop disguise botkiller and non-disguise weapons #1945

Open
seanmcgeehan wants to merge 1 commit into
ValveSoftware:masterfrom
seanmcgeehan:botkiller-fix
Open

fix: Properly Drop disguise botkiller and non-disguise weapons #1945
seanmcgeehan wants to merge 1 commit into
ValveSoftware:masterfrom
seanmcgeehan:botkiller-fix

Conversation

@seanmcgeehan

@seanmcgeehan seanmcgeehan commented May 31, 2026

Copy link
Copy Markdown
Contributor

Description

CTFWeaponBase::Drop has leaked extra wearables since 2009 due to a bad ordering of RemoveExtraWearables and Drop. CTFWeaponBase::Drop is currently only used for disguise weapons. This was exposed in #1824 due to DetermineDisguiseWeapon actually processing extra wearables, and capped at 5 in #1929 . This fix builds on the fix for the leak in #1929, by fully cleaning up any extra wearables with view models and adds logic to clean up extra wearables for non disguise weapons incase drop is ever used for those in the future. The view model fix handles a visual edge in disguises for botkillers if the spy chose to set up extra wearables multiple times. The disguise wearable cap is left in place to prevent leaks with extra wearables without view models. This additionally allows for full disguise functionality from #1824. ExtraWearables with view models are the ones that should be cleaned up

A full round of my testing is attached in video form below. My testing does not hit line 1145 (drop for non disguise weapons), which is not exposed in game, but should be there to prevent future leaks.

https://youtu.be/87DMgfGqxpA

This PR is similar to the closed pr #1939 , but does not share the downsides of that PR.

And yes i hand wrote this pr :p

@FlaminSarge

Copy link
Copy Markdown
Contributor

Question related to the disguise wearable handling: when a player switches away from their disguise weapon to another one, the current logic is hiding the old weapon's attached wearable models and showing the new ones, rather than deleting and recreating, right? If it's the latter, then the entire fix from the other PRs has the potential to allow someone to spam creating/removing a bunch of entities just by switching disguise weapons rapidly, and I can't currently tell if the logic allows that.

@seanmcgeehan

seanmcgeehan commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@FlaminSarge The cap from #1929 prevents that case. They can create 5 but no more. 5 is safe and under the number of entities you can create in other cases.

@FlaminSarge

Copy link
Copy Markdown
Contributor

@seanmcgeehan My concern is someone staying under the cap by getting the extra wearables deleted, then recreated rapidly a bunch of times.

@seanmcgeehan

Copy link
Copy Markdown
Contributor Author

@FlaminSarge There is no risk before or after this PR.

before this PR: Viewmodel wearables are deleted but world model + the wearable is not. Cap is not decremented and no leak is possible.

After this PR: both View model and world model + the wearable are deleted. Cap is appropriately decremented and no leak is possible.

@FlaminSarge

FlaminSarge commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@FlaminSarge There is no risk before or after this PR.

before this PR: Viewmodel wearables are deleted but world model + the wearable is not. Cap is not decremented and no leak is possible.

After this PR: both View model and world model + the wearable are deleted. Cap is appropriately decremented and no leak is possible.

I understand that there's no possible leak, but I'm saying that the creation and deletion of wearable entities can put load on the server (we saw it happen with Randomizer plugins on round start, where a whole bunch of new weapon entities would get created; all the old ones would get cleaned up beforehand, but just the sheer amount of them getting deleted and created caused the server some stress). Someone rapidly deleting and recreating their disguise wearables every few frames could cause the same server load even if they don't cause the total number of them to go up.

The case I'm trying to avoid is a player being able to rapidly switch disguise weapons (or other action) to trigger deletion and recreation every frame.

@seanmcgeehan

Copy link
Copy Markdown
Contributor Author

@FlaminSarge Just load tested locally with 22 spy bots + myself on itemtest at the same time spamming last disguise and seemed to have no effect. At that point i would think there's a more effective attack vector.

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.

2 participants