From 32924f97573c09696e3b61c41fd3cb841b38fecb Mon Sep 17 00:00:00 2001 From: Mike Primm Date: Sun, 30 Dec 2012 23:14:23 -0600 Subject: [PATCH] [Bleeding] Fix corruption due to thread safety issues. Fixes BUKKIT-3333 The 'tag' NBTTagCompound field of the ItemStack assumes that it is OK to save a reference to an NBT supplied via load() and assumes it is OK to supply a reference to the internal field during a save(). Neither is true, as Chunk NBT structures are required to be read-only once created (due to being written asynchronously off the server thread AND due to the potential to be passed to a new Chunk if the same chunk is reloaded before the writing of the NBT is completed by the File I/O thread). Keeping a live reference to the NBT copy passed in, or to the NBT value passed back during saving, creates serious thread safety issues which can result in corrupted data being written to the world data files. The specific issue here was uncovered by the recent change to use setName("") on the ItemStack.tag object. When a chunk is being loaded again before its save is completed, this results in name of the field in the NBT being set to "". This causes it to be saved as "" instead of "tag" resulting in it not being properly reloaded in the future which results in the itemstack losing all of its metadata. --- src/main/java/net/minecraft/server/ItemStack.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/net/minecraft/server/ItemStack.java b/src/main/java/net/minecraft/server/ItemStack.java index 453ca4ac6e..7c1bbec3ac 100644 --- a/src/main/java/net/minecraft/server/ItemStack.java +++ b/src/main/java/net/minecraft/server/ItemStack.java @@ -95,7 +95,7 @@ public final class ItemStack { nbttagcompound.setByte("Count", (byte) this.count); nbttagcompound.setShort("Damage", (short) this.damage); if (this.tag != null) { - nbttagcompound.set("tag", this.tag); + nbttagcompound.set("tag", this.tag.clone()); // CraftBukkit - make defensive copy, data is going to another thread } return nbttagcompound; @@ -106,8 +106,8 @@ public final class ItemStack { this.count = nbttagcompound.getByte("Count"); this.damage = nbttagcompound.getShort("Damage"); if (nbttagcompound.hasKey("tag")) { - // CraftBukkit - clear name from compound - this.tag = (NBTTagCompound) nbttagcompound.getCompound("tag").setName(""); + // CraftBukkit - clear name from compound and make defensive copy as this data may be coming from the save thread + this.tag = (NBTTagCompound) nbttagcompound.getCompound("tag").clone().setName(""); } }