Properly clone custom nbt tags inside ItemMeta

[SoSeDiK]

TLDR; Unhandled nbt tags inside ItemMeta are not cloned upon cloning ItemMeta, which, in some very specific cases, leads to unintentionally modifying the original item when modifying the nbt of its clone.

---

ItemMeta stores Map of unhandled nbt tags and upon cloning it (usually either via clone() or new ItemStack(oldItem)) just puts all existing keys into the new map without cloning them.

Normally this would stay unnoticed (and was for years) and have no side effects since while working with nbt you do not modify the existing objects directly, BUT this is not the case at least for the compound tag, changes to which will be applied to every cloned item.

I've run into this issue while messing around with NBT-API via the following code:

ItemStack test1 = new ItemStack(Material.STONE);
new NBTItem(test1, true).getOrCreateCompound("TestCompound").setString("key1", "value1");
ItemStack test2 = test1.clone();
getLogger().info("item1: " + new NBTItem(test1));
getLogger().info("item2: " + new NBTItem(test2));
new NBTItem(test1, true).getOrCreateCompound("TestCompound").setString("key2", "value2");
getLogger().info("item1: " + new NBTItem(test1));
getLogger().info("item2: " + new NBTItem(test2));

Behavior before this patch:

[TestPlugin] item1: {TestCompound:{key1:"value1"}}
[TestPlugin] item2: {TestCompound:{key1:"value1"}}
[TestPlugin] item1: {TestCompound:{key1:"value1",key2:"value2"}}
[TestPlugin] item2: {TestCompound:{key1:"value1",key2:"value2"}}

Notice how the nbt of item2 was affected too despite the item being the cloned one because they still share the same compound tag. After the patch:

[TestPlugin] item1: {TestCompound:{key1:"value1"}}
[TestPlugin] item2: {TestCompound:{key1:"value1"}}
[TestPlugin] item1: {TestCompound:{key1:"value1",key2:"value2"}}
[TestPlugin] item2: {TestCompound:{key1:"value1"}}

Vanilla uses similar logic and also clones tags, so I'm not sure why this was not the case for Bukkit's implementation. I'm open to opinions on this if any.

---

Edit: Simplified explanation in case it's not clear what's going on:

1. Upon cloning an item its meta gets separate name, lore, enchantments, etc. (all vanilla tags)
2. Items also have Map<String, Tag> of unhanled tags that plugins can use for storing custom data.
3. Upon cloning the new map is created and all the previous unhanded tags are put into the new map. (shallow copy)
4. Now we have two items that have their own vanilla tags (that are equals()) and shared custom tags (that are ==).
5. NBT-API accesses the compound tag of the first item and puts a new nbt value into it.
6. Since both the original and the cloned item share the same objects, the second item now has that newly created nbt value too, even though the second item was never touched within the code.

[Machine-Maker]

Is this not a thing that NBT-API has to deal with themselves? Custom nbt isn't supported by the API so if you can't have any issues using the bukkit ItemMeta api then I think this is "won't fix"

[SoSeDiK]

There's not much that NBT-API can do.

The bug here is that both items share the same exact nbt tag objects despite the cloning operation. So when NBT-API modifies the inner compound tag of the first item it also unintentionally does that for the second one too, because they both refer to the same nbt object.

Also, as already noted, Minecraft itself clones nbt tags upon cloning the item too, so this is more of a problem with Bukkit's implementation.

Edit: By the way, custom nbt is supported by the use of PersistentDataContainer, but I just really prefer NBT-API over it by a lot 😅

[electronicboy]

I'm honestly not sure on introducing an inherent performance hit on API which already has its issues, and behavioural change, especially when the primary way of exposing this behaviour even exists is through unsupported operations; this at the very least to me should be a separate method

[SoSeDiK]

I'm honestly not sure on introducing an inherent performance hit on API which already has its issues, and behavioural change, especially when the primary way of exposing this behaviour even exists is through unsupported operations; this at the very least to me should be a separate method

It's definitely a bug though. Modifications to cloned items shouldn't apply to the original, no matter what methods you use.

I highly doubt someone uses this very specific bug to do some intentional behavioral stuff, and would've happily fixed this upstream, but I simply don't have access to it despite requesting a few times.

An extra method for a "proper" clone operation would add a manual workaround in some places, but wouldn't fix the core issue, and would add extra performance costs in other places. For example, BlockDispenseEvent#getItem() already returns item.clone(), and now I would need to perform one extra cloning operation, so I could add my own tags without touching the original item.

[electronicboy]

1. There is no API contract here that promises that data which the API literally cannot touch is defensively copied, and there is no API contract here that ItemMeta is a deep clone, the entire thing is a shallow clone; Maybe there is an argument that for unhandled tags, the set (apply to item) should do a deep clone as that would follow the behavior of creating an ItemMeta instance in terms of ensuring that data here can't be mutated back given that only vanilla (and unsupported things) can touch that data

What your saying is that the entire implementation of bukkits ItemMeta API is flawed, and you'd be correct, I don't think that special casing a single thing in here out of everything else which is shallow copied is fixing any bugs here outside of your hopes that we'll modify the API behavior to cater towards your unsupported behavior

2. deep cloning the PDC is 100% redundant, mutations of the PDC do not modify the source tags, so only chance of that even being an issue is through unsupported mutation of data

[SoSeDiK]

Made a simplified version that fixes ItemStack#clone by cloning unhandled tags inside CraftItemMeta's constructor. ItemMeta#clone will still have this issue, but it's less of a load and mess for already *terrible* meta.

The reasoning behind cloning PDC was that NBT-API includes universal APIs for them, and in my tests, they were affected too since it touched them specifically. Though I agree that it's not intended behavior and out of the scope of Paper's API.

This is already a niche feature in itself, and I doubt many more clone ItemMeta specifically and mess with overly complicated compound tags via provided API, so in the end, I agree that it's not worth the cost and meta's PDC and #clone can be dusted from this. Nobody has reported this in years, and I stumbled upon it only when cloning items, so I'm more than OK with it too :)

---

I'd be happy to see this fix merged at least for ItemStack#clone since this allows creating custom scopes like {"MyPluginNamespace":{"MyKey":"MyValue"}} and storing plugin-specific data in them instead of using {"MyPluginNamespace.MyKey":"MyData"} for every value, and now cloned item stacks won't have shared data.

[Owen1212055]

Hopefully at some point we will see the whole meta system be yeeted.

In general, although this is unsupported, I don't entirely see an issue with having this fix for now as this behavior will most likely be fixed when item meta is redone anyways.

[SoSeDiK]

Made the fix apply to the ItemMeta#clone and brought back the PDC deep clone. The latter can be yeeted again upon request/merge if there's any interest in the fix and deemed unneeded, but just noting that it's prone to the *bug* too:

ItemMeta meta1 = new ItemStack(Material.STONE).getItemMeta();
new NBTPersistentDataContainer(meta1.getPersistentDataContainer()).getOrCreateCompound("TestCompound").setString("key1", "value1");

ItemMeta meta2 = meta1.clone();
new NBTPersistentDataContainer(meta1.getPersistentDataContainer()).getOrCreateCompound("TestCompound").setString("key2", "value2");

getLogger().info("meta1: " + new NBTPersistentDataContainer(meta1.getPersistentDataContainer()));
getLogger().info("meta2: " + new NBTPersistentDataContainer(meta2.getPersistentDataContainer()));
// Both meta1 and meta2 now have "key2":"value2" inside Bukkit's container even though meta2 was cloned

(but again, I'm using third-party API for setting the tags (Bukkit's is a mess IMHO), so it's definitely not up to me whether the fix should apply)