fix: Properly Drop disguise botkiller and non-disguise weapons #1945
fix: Properly Drop disguise botkiller and non-disguise weapons #1945seanmcgeehan wants to merge 1 commit into
Conversation
|
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. |
|
@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. |
|
@seanmcgeehan My concern is someone staying under the cap by getting the extra wearables deleted, then recreated rapidly a bunch of times. |
|
@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. |
|
@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. |
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