From 7b0816e42fd61d7a1ec580920ac35a98831ccdf5 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 00:52:26 +0000 Subject: [PATCH] Properly handle transaction building errors instead of kicking the player --- .../TransactionBuilderInventory.php | 4 + src/network/mcpe/InventoryManager.php | 23 ---- .../mcpe/handler/InGamePacketHandler.php | 13 ++- .../mcpe/handler/ItemStackRequestExecutor.php | 108 +++++++++++++----- .../mcpe/handler/ItemStackResponseBuilder.php | 19 +-- 5 files changed, 104 insertions(+), 63 deletions(-) diff --git a/src/inventory/transaction/TransactionBuilderInventory.php b/src/inventory/transaction/TransactionBuilderInventory.php index 4995284a0..95b6c4a14 100644 --- a/src/inventory/transaction/TransactionBuilderInventory.php +++ b/src/inventory/transaction/TransactionBuilderInventory.php @@ -50,6 +50,10 @@ final class TransactionBuilderInventory extends BaseInventory{ $this->changedSlots = new \SplFixedArray($this->actualInventory->getSize()); } + public function getActualInventory() : Inventory{ + return $this->actualInventory; + } + protected function internalSetContents(array $items) : void{ for($i = 0, $size = $this->getSize(); $i < $size; ++$i){ if(!isset($items[$i])){ diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index a0a28a3a2..1ea14b33c 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -555,27 +555,4 @@ class InventoryManager{ $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); return $this->itemStackInfos[spl_object_id($inventory)][$slotId] = $info; } - - public function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : bool{ - $inventoryObjectId = spl_object_id($inventory); - if(!isset($this->itemStackInfos[$inventoryObjectId])){ - $this->session->getLogger()->debug("Attempted to match item preimage unsynced inventory " . get_class($inventory) . "#" . $inventoryObjectId); - return false; - } - $info = $this->itemStackInfos[$inventoryObjectId][$slotId] ?? null; - if($info === null){ - $this->session->getLogger()->debug("Attempted to match item preimage for unsynced slot $slotId in " . get_class($inventory) . "#$inventoryObjectId that isn't synced"); - return false; - } - - if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){ - $this->session->getLogger()->debug( - "Mismatched expected itemstack: " . get_class($inventory) . "#" . $inventoryObjectId . ", " . - "slot: $slotId, client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") - ); - return false; - } - - return true; - } } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index dbf2aa5ab..70bb827a2 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -110,11 +110,13 @@ use pocketmine\player\Player; use pocketmine\utils\AssumptionFailedError; use pocketmine\utils\Limits; use pocketmine\utils\TextFormat; +use pocketmine\utils\Utils; use pocketmine\world\format\Chunk; use function array_push; use function base64_encode; use function count; use function fmod; +use function implode; use function in_array; use function is_bool; use function is_infinite; @@ -531,9 +533,14 @@ class InGamePacketHandler extends PacketHandler{ private function handleSingleItemStackRequest(ItemStackRequest $request) : ItemStackResponse{ $executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request); - $transaction = $executor->generateInventoryTransaction(); - $result = $this->executeInventoryTransaction($transaction, $request->getRequestId()); - $this->session->getLogger()->debug("Item stack request " . $request->getRequestId() . " result: " . ($result ? "success" : "failure")); + try{ + $transaction = $executor->generateInventoryTransaction(); + $result = $this->executeInventoryTransaction($transaction, $request->getRequestId()); + }catch(ItemStackRequestProcessException $e){ + $result = false; + $this->session->getLogger()->debug("ItemStackRequest #" . $request->getRequestId() . " failed: " . $e->getMessage()); + $this->session->getLogger()->debug(implode("\n", Utils::printableExceptionInfo($e))); + } return $executor->buildItemStackResponse($result); } diff --git a/src/network/mcpe/handler/ItemStackRequestExecutor.php b/src/network/mcpe/handler/ItemStackRequestExecutor.php index b904f279d..c2ae8e305 100644 --- a/src/network/mcpe/handler/ItemStackRequestExecutor.php +++ b/src/network/mcpe/handler/ItemStackRequestExecutor.php @@ -25,6 +25,7 @@ namespace pocketmine\network\mcpe\handler; use pocketmine\crafting\CraftingGrid; use pocketmine\inventory\CreativeInventory; +use pocketmine\inventory\Inventory; use pocketmine\inventory\transaction\action\CreateItemAction; use pocketmine\inventory\transaction\action\DestroyItemAction; use pocketmine\inventory\transaction\action\DropItemAction; @@ -81,25 +82,49 @@ final class ItemStackRequestExecutor{ $this->builder = new TransactionBuilder(); } + private function prettyInventoryAndSlot(Inventory $inventory, int $slot) : string{ + if($inventory instanceof TransactionBuilderInventory){ + $inventory = $inventory->getActualInventory(); + } + return (new \ReflectionClass($inventory))->getShortName() . "#" . spl_object_id($inventory) . ", slot: $slot"; + } + + /** + * @throws ItemStackRequestProcessException + */ + private function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : void{ + $info = $this->inventoryManager->getItemStackInfo($inventory, $slotId); + if($info === null){ + throw new AssumptionFailedError("The inventory is tracked and the slot is valid, so this should not be null"); + } + + if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){ + throw new ItemStackRequestProcessException( + $this->prettyInventoryAndSlot($inventory, $slotId) . ": " . + "Mismatched expected itemstack, " . + "client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none") + ); + } + } + /** * @phpstan-return array{TransactionBuilderInventory, int} + * + * @throws ItemStackRequestProcessException */ private function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{ $windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId()); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId()); if($windowAndSlot === null){ - throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + throw new ItemStackRequestProcessException("No open inventory matches container UI ID: " . $info->getContainerId() . ", slot ID: " . $info->getSlotId()); } [$inventory, $slot] = $windowAndSlot; if(!$inventory->slotExists($slot)){ - throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + throw new ItemStackRequestProcessException("No such inventory slot :" . $this->prettyInventoryAndSlot($inventory, $slot)); } - if( - $info->getStackId() !== $this->request->getRequestId() && //using TransactionBuilderInventory enables this to work - !$this->inventoryManager->matchItemStack($inventory, $slot, $info->getStackId()) - ){ - throw new PacketHandlingException("Inventory " . $info->getContainerId() . ", slot " . $slot . ": server-side item does not match expected"); + if($info->getStackId() !== $this->request->getRequestId()){ //the itemstack may have been modified by the current request + $this->matchItemStack($inventory, $slot, $info->getStackId()); } return [$this->builder->getInventory($inventory), $slot]; @@ -112,6 +137,7 @@ final class ItemStackRequestExecutor{ /** * Deducts items from an inventory slot, returning a stack containing the removed items. + * @throws ItemStackRequestProcessException */ private function removeItemFromSlot(ItemStackRequestSlotInfo $slotInfo, int $count) : Item{ $this->requestSlotInfos[] = $slotInfo; @@ -119,7 +145,7 @@ final class ItemStackRequestExecutor{ $existingItem = $inventory->getItem($slot); if($existingItem->getCount() < $count){ - throw new PacketHandlingException("Cannot take $count items from a stack of " . $existingItem->getCount()); + throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Cannot take $count items from a stack of " . $existingItem->getCount()); } $removed = $existingItem->pop($count); @@ -137,7 +163,7 @@ final class ItemStackRequestExecutor{ $existingItem = $inventory->getItem($slot); if(!$existingItem->isNull() && !$existingItem->canStackWith($item)){ - throw new PacketHandlingException("Can only add items to an empty slot, or a slot containing the same item"); + throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Can only add items to an empty slot, or a slot containing the same item"); } //we can't use the existing item here; it may be an empty stack @@ -146,6 +172,9 @@ final class ItemStackRequestExecutor{ $inventory->setItem($slot, $newItem); } + /** + * @throws ItemStackRequestProcessException + */ private function setNextCreatedItem(?Item $item, bool $creative = false) : void{ if($item !== null && $item->isNull()){ $item = null; @@ -157,7 +186,7 @@ final class ItemStackRequestExecutor{ $this->nextCreatedItem->setCount($this->createdItemsTakenCount); $this->builder->addAction(new CreateItemAction($this->nextCreatedItem)); }elseif($this->createdItemsTakenCount < $this->nextCreatedItem->getCount()){ - throw new PacketHandlingException("Not all of the previous created item was taken"); + throw new ItemStackRequestProcessException("Not all of the previous created item was taken"); } } $this->nextCreatedItem = $item; @@ -165,24 +194,27 @@ final class ItemStackRequestExecutor{ $this->createdItemsTakenCount = 0; } + /** + * @throws ItemStackRequestProcessException + */ private function beginCrafting(int $recipeId, int $repetitions) : void{ if($this->specialTransaction !== null){ - throw new PacketHandlingException("Cannot perform more than 1 special action per request"); + throw new ItemStackRequestProcessException("Another special transaction is already in progress"); } if($repetitions < 1){ //TODO: upper bound? - throw new PacketHandlingException("Cannot craft a recipe less than 1 time"); + throw new ItemStackRequestProcessException("Cannot craft a recipe less than 1 time"); } $craftingManager = $this->player->getServer()->getCraftingManager(); $recipe = $craftingManager->getCraftingRecipeFromIndex($recipeId); if($recipe === null){ - throw new PacketHandlingException("Unknown crafting recipe ID $recipeId"); + throw new ItemStackRequestProcessException("No such crafting recipe index: $recipeId"); } $this->specialTransaction = new CraftingTransaction($this->player, $craftingManager, [], $recipe, $repetitions); $currentWindow = $this->player->getCurrentWindow(); if($currentWindow !== null && !($currentWindow instanceof CraftingGrid)){ - throw new PacketHandlingException("Cannot complete crafting when the player's current window is not a crafting grid"); + throw new ItemStackRequestProcessException("Player's current window is not a crafting grid"); } $craftingGrid = $currentWindow ?? $this->player->getCraftingGrid(); @@ -200,13 +232,13 @@ final class ItemStackRequestExecutor{ private function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{ $createdItem = $this->nextCreatedItem; if($createdItem === null){ - throw new PacketHandlingException("No created item is waiting to be taken"); + throw new ItemStackRequestProcessException("No created item is waiting to be taken"); } if(!$this->createdItemFromCreativeInventory){ $availableCount = $createdItem->getCount() - $this->createdItemsTakenCount; if($count > $availableCount){ - throw new PacketHandlingException("Not enough created items available to be taken (have $availableCount, tried to take $count)"); + throw new ItemStackRequestProcessException("Not enough created items available to be taken (have $availableCount, tried to take $count)"); } } @@ -217,6 +249,22 @@ final class ItemStackRequestExecutor{ } } + /** + * @throws ItemStackRequestProcessException + */ + private function assertDoingCrafting() : void{ + if(!$this->specialTransaction instanceof CraftingTransaction){ + if($this->specialTransaction === null){ + throw new ItemStackRequestProcessException("Expected CraftRecipe or CraftRecipeAuto action to precede this action"); + }else{ + throw new ItemStackRequestProcessException("A different special transaction is already in progress"); + } + } + } + + /** + * @throws ItemStackRequestProcessException + */ private function processItemStackRequestAction(ItemStackRequestAction $action) : void{ if( $action instanceof TakeStackRequestAction || @@ -253,10 +301,7 @@ final class ItemStackRequestExecutor{ }elseif($action instanceof CreativeCreateStackRequestAction){ $item = CreativeInventory::getInstance()->getItem($action->getCreativeItemId()); if($item === null){ - //TODO: the item may have been unregistered after the client was sent the creative contents, leaving a - //gap in the creative item list. This probably shouldn't be a violation, but I'm not sure how else to - //handle it right now. - throw new PacketHandlingException("Tried to create nonexisting creative item " . $action->getCreativeItemId()); + throw new ItemStackRequestProcessException("No such creative item index: " . $action->getCreativeItemId()); } $this->setNextCreatedItem($item, true); @@ -265,31 +310,34 @@ final class ItemStackRequestExecutor{ }elseif($action instanceof CraftRecipeAutoStackRequestAction){ $this->beginCrafting($action->getRecipeId(), $action->getRepetitions()); }elseif($action instanceof CraftingConsumeInputStackRequestAction){ - if(!$this->specialTransaction instanceof CraftingTransaction){ - throw new PacketHandlingException("Cannot consume crafting input when no crafting transaction is in progress"); - } + $this->assertDoingCrafting(); $this->removeItemFromSlot($action->getSource(), $action->getCount()); //output discarded - we allow CraftingTransaction to verify the balance }elseif($action instanceof CraftingCreateSpecificResultStackRequestAction){ - if(!$this->specialTransaction instanceof CraftingTransaction){ - throw new AssumptionFailedError("Cannot mark crafting result index when no crafting transaction is in progress"); - } + $this->assertDoingCrafting(); $nextResultItem = $this->craftingResults[$action->getResultIndex()] ?? null; if($nextResultItem === null){ - throw new PacketHandlingException("No such crafting result index " . $action->getResultIndex()); + throw new ItemStackRequestProcessException("No such crafting result index: " . $action->getResultIndex()); } $this->setNextCreatedItem($nextResultItem); }elseif($action instanceof DeprecatedCraftingResultsStackRequestAction){ //no obvious use }else{ - throw new PacketHandlingException("Unhandled item stack request action: " . get_class($action)); + throw new ItemStackRequestProcessException("Unhandled item stack request action"); } } + /** + * @throws ItemStackRequestProcessException + */ public function generateInventoryTransaction() : InventoryTransaction{ - foreach($this->request->getActions() as $action){ - $this->processItemStackRequestAction($action); + foreach($this->request->getActions() as $k => $action){ + try{ + $this->processItemStackRequestAction($action); + }catch(ItemStackRequestProcessException $e){ + throw new ItemStackRequestProcessException("Error processing action $k (" . (new \ReflectionClass($action))->getShortName() . "): " . $e->getMessage(), 0, $e); + } } $this->setNextCreatedItem(null); $inventoryActions = $this->builder->generateActions(); diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 782404ebe..35a07425e 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -29,7 +29,7 @@ use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseContainerInfo; use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponseSlotInfo; -use pocketmine\network\PacketHandlingException; +use pocketmine\utils\AssumptionFailedError; final class ItemStackResponseBuilder{ @@ -51,15 +51,15 @@ final class ItemStackResponseBuilder{ /** * @phpstan-return array{Inventory, int} */ - private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : array{ + private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : ?array{ $windowId = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId()); $windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId); if($windowAndSlot === null){ - throw new PacketHandlingException("Stack request action cannot target an inventory that is not open"); + return null; } [$inventory, $slot] = $windowAndSlot; if(!$inventory->slotExists($slot)){ - throw new PacketHandlingException("Stack request action cannot target an inventory slot that does not exist"); + return null; } return [$inventory, $slot]; @@ -72,16 +72,21 @@ final class ItemStackResponseBuilder{ continue; } foreach($slotIds as $slotId){ - [$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + $inventoryAndSlot = $this->getInventoryAndSlot($containerInterfaceId, $slotId); + if($inventoryAndSlot === null){ + //a plugin may have closed the inventory during an event, or the slot may have been invalid + continue; + } + [$inventory, $slot] = $inventoryAndSlot; $itemStackInfo = $this->inventoryManager->getItemStackInfo($inventory, $slot); if($itemStackInfo === null){ - //TODO: what if a plugin closes the inventory while the transaction is ongoing? - throw new \LogicException("ItemStackInfo should never be null for an open inventory"); + throw new AssumptionFailedError("ItemStackInfo should never be null for an open inventory"); } if($itemStackInfo->getRequestId() !== $this->requestId){ //the itemstack may have been synced due to transaction producing results that the client did not //predict correctly, which will wipe out the tracked request ID (intentionally) + //TODO: is this the correct behaviour? continue; } $item = $inventory->getItem($slot);