setAmount can be used to create invalid items which break inventory saving

[Malfrador]

Expected behavior

ItemStack#setAmount should be restricted to 1-99 and refuse values outside the allowed range, instead of directly setting the count field of the underlying NMS stack without any checks.

Observed/Actual behavior

ItemStack#setAmount can be used to set the count to any value. Setting it to a value outside the allowed range of 1-99 causes saving to break for the inventory the item stack is in (e.g. the player's), as the ItemStack codec will throw when trying to parse the item: IllegalStateException: Value must be within range [1;99]: 128

The entire inventory will not save, not just the invalid stack:

[03:38:25 WARN]: Failed to save player data for Malfrador
net.minecraft.ReportedException: Saving entity NBT

Steps/models to reproduce

1. Set an ItemStack count to an invalid value, for example with player.getInventory().getItem(0).setAmount(128);
2. Disconnect to trigger an inventory save
3. An exception will be thrown.
4. Log back in and notice your inventory did not save

Plugin and Datapack List

not relevant

Paper version

This server is running Paper version 1.21-109-master@5a5035b (2024-07-23T08:16:30Z) (Implementing API version 1.21-R0.1-SNAPSHOT) Will affect all versions since 1.20.6.

Other

While its not really the job of the API to protect people from doing stupid things, this is imo bad enough that there should be a check in place. This is additionally made worse by the fact that before 1.20.6, item stack counts up to 127 were handled by the game perfectly fine. Someone may use an older plugin that uses this behaviour and accidentally break their inventories in the process. IMO throwing an exception here would be needed

[electronicboy]

I was debating on talking about this, the big issue however is all the legacy logic that exists surrounding this, I'm not sure it's tenable to make this a hard fail right now, especially as many plugins use oversized stacks internally, it only becomes an issue if placed into inventories or the world

I wonder if this is one where it kinda makes sense to record the causing trace if an oversized stack is created and then print that if added into something where it would cause issues, i.e. inventories, the world, etc

[Lulu13022002]

This affect the constructor/static methods too. But it's a bit tricky to enforce those limit since some api support oversized stack things like Inventory#addItem/removeItem and maybe more.

[Malfrador]

My initial reason for creating this issue was that I did update and only noticed that it made inventories unsavable after a plugin added such stacks to every inventory.

Maybe instead of hard failing with an exception just limit the stack to 99 when it's added to an inventory? Not the biggest fan of such a silent behaviour change either though tbh.

The addItem method correctly splits up an oversized stack into two when adding it to an inventory.

[electronicboy]

depends on how you do the limit, you either risk losing items or potentially introducing unexpected behavior, I would probably just add a debug mode that causes it to store a stack trace whenever the amount limit is exceeded and print a warning when it's sent somewhere precarious, maybe on saving it makes sense to cap it as a "better losing some than everything", but, I'm not sure what the best behavior would be otherwise

[Brokkonaut]

Would it be possible to just enforce the limits when oversized stacks get added to some inventory or blockstate or so?

For example maybe CraftItemStack.asNMSCopy and CraftItemStack.unwrap could enforce the size

[Machine-Maker]

"mirror" ItemStacks are already in inventories. Having some system to know, only on CraftItemStack, that the stack is in an inventory sounds super jank. I think it's best if we just check directly on setAmount. Plugins doing things like overstacking, but not actually doing anything with the stack seems like not a good reason to keep that behavior.

[Brokkonaut]

"mirror" ItemStacks are already in inventories.

Yes I forgot about them... This makes things much more complicated

Plugins doing things like overstacking, but not actually doing anything with the stack seems like not a good reason to keep that behavior.

It's not that plugins could not do anything, things like Inventory.addItem and Inventory.removeItem always worked with oversized stacks and splitted them.. Many of our plugins currently use that behaviour and it is currently explicitly allowed as it is described in those javadocs

[CoPokBl]

Is there a technical change that prevents supporting any size? I think that breaking something that has always worked is a bug with Paper. But also I just tested on Paper and it exists there so is this even relevant to Paper? It seems like an upstream issue.

[Malfrador]

Is there a technical change that prevents supporting any size? I think that breaking something that has always worked is a bug with Paper. But also I just tested on Paper and it exists there so is this even relevant to Paper? It seems like an upstream issue.

Vanilla only supports sizes from 1 to 99. Changing that would make Paper worlds incompatible with vanilla, which of course isn't an option. And yes, technically this could also be fixed by Spigot.

[Brokkonaut]

How about deprecating oversized stacks for removal, like print an "author nag message" when oversized stacks are ever created and enforce the limits in CraftItemStack.asNMSCopy and CraftItemStack.unwrap and also prevent oversizing stacks that are created from CraftItemStack.asCraftMirror, so that definitely no unsafe itemstacks could be added to minecraft internal data, but most plugins will continue to work.

and after some time, maybe in 1.23 or so completely remove the ability to create oversized stacks.

[Machine-Maker]

All itemstacks are created via CraftItemStack.asCraftMirror now. There really isn't an "api-only" ItemStack anymore. Every single ItemStack instance in the API is either a CraftItemStack, or just wraps a CraftItemStack.

[masmc05]

depends on how you do the limit, you either risk losing items or potentially introducing unexpected behavio

An addition to this: since vanilla often clamps the count, a lot of affected API already has losing behaviour, like Inventory#setItem having an inconsistent behaviour of sometimes clamping and sometimes leaving the count as it is (it depends on the nms Container implementation)