Uploaded image for project: 'Spigot'
  1. Spigot
  2. SPIGOT-5608

Modifying the player inventory during trident charge can duplicate it

    XMLWordPrintable

Details

    • This server is running CraftBukkit version git-Spigot-8faa8b4-13ed05d (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)
    • Yes

    Description

      This issue may be something for plugins to fix themselves, but I post this here anyways in case you want to consider resolving this inside CraftBukkit in some way, since this can be easily missed by plugin devs not aware of it.

      Any plugin which uses Inventory#get/setStorageContents to get, modify and then update inventory contents may be affected by this.

      For easy reproduction I attach a dummy plugin (source code: https://pastebin.com/y9dPArzv (this gets simply invoked in the plugin's main class)): The plugin periodically replenishes arrow itemstacks and uses PlayerInventory#get/setStorageContents to update the inventory contents. This is only a dummy usecase to demonstrate the issue and easily reproduce it since this gets run very frequently without any other actions required by the player. (I originally encountered this issue in a different scenario and usecase: https://github.com/Shopkeepers/Shopkeepers/issues/635)

      The essential part is that setStorageContents is used to update the player's inventory and that the player might currently actively use an item in his hand, such as charging a trident (holding right-mouse button). Other items might be affected as well, I haven't checked for that.

      The problem is as follows:

      • When starting to charge the trident, Minecraft remembers the ItemStack instance (see HumanEntity#activeItem)
      • When a plugin modifies the player's inventory via setStorageContents the items in all slots get updated (even those in slots that were not actually affected by the inventory manipulations). setStorageContents calls setItem for all items, which creates a new nms ItemStack instance via CraftItemStack.asNMSCopy, even if the itemstack still matches the previous one.
      • When checking if the player is still using the activeItem, Minecraft only compares the material with the currently held item. Since the held item is still of type trident after the inventory manipulation, minecraft does not abort the charging.
      • Once the trident is actually thrown minecraft attempts to remove the actual ItemStack instance from the player's inventory. This fails, since the ItemStack instance has changed since then.
      • The result: The trident is thrown nevertheless and can be picked up again by the player, while no item is removed for it from the player's inventory. The player is able to duplicate the trident this way.

      A plugin could fix this by:

      • Stoping any active item actions currently in progress (may be interruptive though for the player).
      • Only update those inventory slots that were actually modified. As long as the trident's slot isn't affected by the inventory manipulation this will work. In case the actively used item slot is affected, this issue still applies. Even when using Inventory#setItem directly instead of Inventory#setStorageContents.

      In my plugin I am using get/setStorageContents since it is convenient to implement the inventory manipulations with: It requires copying the ItemStacks only once, even if the inventory is scanned/read multiple times in multiple passes, and one can prepare the inventory update without applying it directly.
      My workaround is currently to implement the setStorageContents myself and skip any slot if the new item is equal to the current item inside the inventory (requires another copy to read the current inventory item). This also has the benefit of not having to send the player inventory slot updates for slots that haven't actually changed. However, this only works as long and the slot of the actively used itemstack isn't actually affected by the inventory manipulation. In my specific usecase it will probably change it to abort any active item action (still have to check if there is a Bukkit API for that yet), but this may not be applicable in other usescases.

      Edit: There does not seem to be an API yet for aborting any active item action.

      Even though this could be fixed by plugins themselves (in some cases), CraftBukkit could fix this as well for all plugins potentially running into this by either:

      • Having Inventory#setItem check if the itemstack is still the same before actually updating the slot with a copy of the given itemstack. (This is basically what I am doing manually now inside the plugin. Having this inside of CraftBukkit would have the benefit of plugin devs not being required to be aware of this potential issue. Also it would avoid sending slot updates for slots that haven't actually changed. And CraftBukkit might even be able to implement this slightly more efficiently, since it could limit the comparison to the handles of CraftItemStacks via instance comparison; not sure if that optimization would be worth it though)
      • Aborting the trident charging (or any other actively used item) if the ItemStack instance has changed. (Problem: This is rather interruptive for players, so plugins will likely revert to manually checking if a slot needs to be updated. But at least it would resolve the item duplication issue.)
      • Skip the trident throwing (item use) if the itemstack instance cannot be found inside the player's inventory anymore. (Also rather interruptive. And the effect is deferred from the actual inventory updating, so it might be harder for plugin devs to figure out how and why their plugin is breaking the trident throwing for players)
      • Search the player's inventory for a equal itemstack rather than only comparing instances when trying to remove the trident item.
      • When comparing the actively used item with the currently held item, and the itemstack instances are no longer the same but the materials still match, one could update the activeItem to the currently held itemstack instance and continue using that instead.

      Especially the last two suggestions might be worth considering, since all other suggestions have some kind of issue as well. As far as I am aware these cannot easily be implemented by plugins.

      Attachments

        Activity

          People

            Unassigned Unassigned
            blablubbabc blablubbabc
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated: