From a0cca53f52c614fc39a1285eb39cd0ebb7c3862f Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Tue, 27 Feb 2024 16:07:43 +0000 Subject: [PATCH] Fixed mismatched predictions due to NBT key order differences this is a pain :( It appears the client always sorts the keys in alphabetical order due to use of std::map. However I'm not sure of the exact ordering behaviour, so it needs to be investigated. --- src/network/mcpe/InventoryManager.php | 37 ++++++++++++++++++- src/network/mcpe/convert/TypeConverter.php | 12 ++++-- .../mcpe/handler/InGamePacketHandler.php | 15 ++++++-- 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 938b5c82c..e2b71ab31 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -423,6 +423,41 @@ class InventoryManager{ } } + /** + * Compares itemstack extra data for equality. This is used to verify legacy InventoryTransaction slot predictions. + * + * TODO: It would be preferable if we didn't have to deserialize this, to improve performance and reduce attack + * surface. However, the raw data may not match due to differences in ordering. Investigate whether the + * client-provided NBT is consistently sorted. + */ + private function itemStackExtraDataEqual(ItemStack $left, ItemStack $right) : bool{ + if($left->getRawExtraData() === $right->getRawExtraData()){ + return true; + } + + $typeConverter = $this->session->getTypeConverter(); + $leftExtraData = $typeConverter->deserializeItemStackExtraData($left->getRawExtraData(), $left->getId()); + $rightExtraData = $typeConverter->deserializeItemStackExtraData($right->getRawExtraData(), $right->getId()); + + $leftNbt = $leftExtraData->getNbt(); + $rightNbt = $rightExtraData->getNbt(); + return + $leftExtraData->getCanPlaceOn() === $rightExtraData->getCanPlaceOn() && + $leftExtraData->getCanDestroy() === $rightExtraData->getCanDestroy() && ( + $leftNbt === $rightNbt || //this covers null === null and fast object identity + ($leftNbt !== null && $rightNbt !== null && $leftNbt->equals($rightNbt)) + ); + } + + private function itemStacksEqual(ItemStack $left, ItemStack $right) : bool{ + return + $left->getId() === $right->getId() && + $left->getMeta() === $right->getMeta() && + $left->getBlockRuntimeId() === $right->getBlockRuntimeId() && + $left->getCount() === $right->getCount() && + $this->itemStackExtraDataEqual($left, $right); + } + public function onSlotChange(Inventory $inventory, int $slot) : void{ $inventoryEntry = $this->inventories[spl_object_id($inventory)] ?? null; if($inventoryEntry === null){ @@ -432,7 +467,7 @@ class InventoryManager{ } $currentItem = $this->session->getTypeConverter()->coreItemStackToNet($inventory->getItem($slot)); $clientSideItem = $inventoryEntry->predictions[$slot] ?? null; - if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ + if($clientSideItem === null || !$this->itemStacksEqual($currentItem, $clientSideItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request $this->trackItemStack($inventoryEntry, $slot, $currentItem, null); $inventoryEntry->pendingSyncs[$slot] = $currentItem; diff --git a/src/network/mcpe/convert/TypeConverter.php b/src/network/mcpe/convert/TypeConverter.php index 0f9789de7..e886b2b8b 100644 --- a/src/network/mcpe/convert/TypeConverter.php +++ b/src/network/mcpe/convert/TypeConverter.php @@ -248,10 +248,7 @@ class TypeConverter{ if($itemStack->getId() === 0){ return VanillaItems::AIR(); } - $extraDataDeserializer = PacketSerializer::decoder($itemStack->getRawExtraData(), 0); - $extraData = $itemStack->getId() === $this->shieldRuntimeId ? - ItemStackExtraDataShield::read($extraDataDeserializer) : - ItemStackExtraData::read($extraDataDeserializer); + $extraData = $this->deserializeItemStackExtraData($itemStack->getRawExtraData(), $itemStack->getId()); $compound = $extraData->getNbt(); @@ -272,4 +269,11 @@ class TypeConverter{ return $itemResult; } + + public function deserializeItemStackExtraData(string $extraData, int $id) : ItemStackExtraData{ + $extraDataDeserializer = PacketSerializer::decoder($extraData, 0); + return $id === $this->shieldRuntimeId ? + ItemStackExtraDataShield::read($extraDataDeserializer) : + ItemStackExtraData::read($extraDataDeserializer); + } } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 8c3449d41..750502172 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -444,9 +444,18 @@ class InGamePacketHandler extends PacketHandler{ return false; } $serverItemStack = $this->session->getTypeConverter()->coreItemStackToNet($sourceSlotItem); - //because the client doesn't tell us the expected itemstack ID, we have to deep-compare our known - //itemstack info with the one the client sent. This is costly, but we don't have any other option :( - if(!$serverItemStack->equals($clientItemStack)){ + //Sadly we don't have itemstack IDs here, so we have to compare the basic item properties to ensure that we're + //dropping the item the client expects (inventory might be out of sync with the client). + if( + $serverItemStack->getId() !== $clientItemStack->getId() || + $serverItemStack->getMeta() !== $clientItemStack->getMeta() || + $serverItemStack->getCount() !== $clientItemStack->getCount() || + $serverItemStack->getBlockRuntimeId() !== $clientItemStack->getBlockRuntimeId() + //Raw extraData may not match because of TAG_Compound key ordering differences, and decoding it to compare + //is costly. Assume that we're in sync if id+meta+count+runtimeId match. + //NB: Make sure $clientItemStack isn't used to create the dropped item, as that would allow the client + //to change the item NBT since we're not validating it. + ){ return false; }