Merge remote-tracking branch 'origin/stable' into minor-next

build/php

@@ -1 +1 @@
-Subproject commit a464454d1ed946baabd32a02ddcf66361374b99c
+Subproject commit 9d8807be825b3fafd420534f2c29351c1bda6398

changelogs/4.18.md

@@ -77,4 +77,16 @@ Released 25th March 2023.
 - Packet batch limit has been lowered to `100` packets. With the introduction of `ItemStackRequest`, this is more than sufficient for normal gameplay.
 
 ### Other
-- Use `Vector3::zero()` instead of `new Vector3()` in some places.
+- Use `Vector3::zero()` instead of `new Vector3()` in some places.
+
+# 4.18.1
+Released 27th March 2023.
+
+## General
+- `RakLibInterface` now logs the name of the currently active session if a crash occurs when processing a packet. This makes it easier to reproduce bugs, which is important to be able to fix them.
+- Added more detailed debugging information to `InventoryManager->syncSelectedHotbarSlot()`.
+
+## Fixes
+- Fixed server crash when attempting to drop more of an item from a stack than available in the inventory.
+- Fixed packet processing errors when editing writable books.
+- Fixed packet processing errors when shift-clicking on the recipe book to craft recipes which draw from a large number of inventory slots.

src/VersionInfo.php

@@ -31,7 +31,7 @@ use function str_repeat;
 
 final class VersionInfo{
 	public const NAME = "PocketMine-MP";
-	public const BASE_VERSION = "4.18.1";
+	public const BASE_VERSION = "4.18.2";
 	public const IS_DEVELOPMENT_BUILD = true;
 	public const BUILD_CHANNEL = "stable";
 

src/network/mcpe/InventoryManager.php

@@ -576,9 +576,13 @@ class InventoryManager{
 		$playerInventory = $this->player->getInventory();
 		$selected = $playerInventory->getHeldItemIndex();
 		if($selected !== $this->clientSelectedHotbarSlot){
-			$itemStackInfo = $this->getItemStackInfo($playerInventory, $selected);
+			$inventoryEntry = $this->inventories[spl_object_id($playerInventory)] ?? null;
+			if($inventoryEntry === null){
+				throw new AssumptionFailedError("Player inventory should always be tracked");
+			}
+			$itemStackInfo = $inventoryEntry->itemStackInfos[$selected] ?? null;
 			if($itemStackInfo === null){
-				throw new AssumptionFailedError("Player inventory slots should always be tracked");
+				throw new AssumptionFailedError("Untracked player inventory slot $selected");
 			}
 
 			$this->session->sendDataPacket(MobEquipmentPacket::create(

src/network/mcpe/handler/InGamePacketHandler.php

@@ -377,13 +377,21 @@ class InGamePacketHandler extends PacketHandler{
 	}
 
 	private function handleNormalTransaction(NormalTransactionData $data, int $itemStackRequestId) : bool{
-		//When the ItemStackRequest system is used, this transaction type is only used for dropping items by pressing Q.
+		//When the ItemStackRequest system is used, this transaction type is used for dropping items by pressing Q.
 		//I don't know why they don't just use ItemStackRequest for that too, which already supports dropping items by
 		//clicking them outside an open inventory menu, but for now it is what it is.
-		//Fortunately, this means we can be extremely strict about the validation criteria.
+		//Fortunately, this means we can be much stricter about the validation criteria.
 
-		if(count($data->getActions()) > 2){
-			throw new PacketHandlingException("Expected exactly 2 actions for dropping an item");
+		$actionCount = count($data->getActions());
+		if($actionCount > 2){
+			if($actionCount > 5){
+				throw new PacketHandlingException("Too many actions ($actionCount) in normal inventory transaction");
+			}
+
+			//Due to a bug in the game, this transaction type is still sent when a player edits a book. We don't need
+			//these transactions for editing books, since we have BookEditPacket, so we can just ignore them.
+			$this->session->getLogger()->debug("Ignoring normal inventory transaction with $actionCount actions (drop-item should have exactly 2 actions)");
+			return false;
 		}
 
 		$sourceSlot = null;
@@ -401,11 +409,13 @@ class InGamePacketHandler extends PacketHandler{
 				$sourceSlot = $networkInventoryAction->inventorySlot;
 				$clientItemStack = $networkInventoryAction->oldItem->getItemStack();
 			}else{
-				throw new PacketHandlingException("Unexpected action type in drop item transaction");
+				$this->session->getLogger()->debug("Unexpected inventory action type $networkInventoryAction->sourceType in drop item transaction");
+				return false;
 			}
 		}
 		if($sourceSlot === null || $clientItemStack === null || $droppedCount === null){
-			throw new PacketHandlingException("Missing information in drop item transaction, need source slot, client item stack and dropped count");
+			$this->session->getLogger()->debug("Missing information in drop item transaction, need source slot, client item stack and dropped count");
+			return false;
 		}
 
 		$inventory = $this->player->getInventory();
@@ -415,6 +425,9 @@ class InGamePacketHandler extends PacketHandler{
 		}
 
 		$sourceSlotItem = $inventory->getItem($sourceSlot);
+		if($sourceSlotItem->getCount() < $droppedCount){
+			return false;
+		}
 		$serverItemStack = TypeConverter::getInstance()->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 :(
@@ -545,8 +558,17 @@ class InGamePacketHandler extends PacketHandler{
 	}
 
 	private function handleSingleItemStackRequest(ItemStackRequest $request) : ItemStackResponse{
-		if(count($request->getActions()) > 20){
-			//TODO: we can probably lower this limit, but this will do for now
+		if(count($request->getActions()) > 60){
+			//recipe book auto crafting can affect all slots of the inventory when consuming inputs or producing outputs
+			//this means there could be as many as 50 CraftingConsumeInput actions or Place (taking the result) actions
+			//in a single request (there are certain ways items can be arranged which will result in the same stack
+			//being taken from multiple times, but this is behaviour with a calculable limit)
+			//this means there SHOULD be AT MOST 53 actions in a single request, but 60 is a nice round number.
+			//n64Stacks = ?
+			//n1Stacks = 45 - n64Stacks
+			//nItemsRequiredFor1Craft = 9
+			//nResults = floor((n1Stacks + (n64Stacks * 64)) / nItemsRequiredFor1Craft)
+			//nTakeActionsTotal = floor(64 / nResults) + max(1, 64 % nResults) + ((nResults * nItemsRequiredFor1Craft) - (n64Stacks * 64))
 			throw new PacketHandlingException("Too many actions in ItemStackRequest");
 		}
 		$executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request);

src/network/mcpe/raklib/RakLibInterface.php

@@ -192,6 +192,7 @@ class RakLibInterface implements ServerEventListener, AdvancedNetworkInterface{
 			$session = $this->sessions[$sessionId];
 			$address = $session->getIp();
 			$buf = substr($packet, 1);
+			$name = $session->getDisplayName();
 			try{
 				$session->handleEncoded($buf);
 			}catch(PacketHandlingException $e){
@@ -204,6 +205,10 @@ class RakLibInterface implements ServerEventListener, AdvancedNetworkInterface{
 				$logger->debug(implode("\n", Utils::printableExceptionInfo($e)));
 				$session->disconnect("Packet processing error (Error ID: $errorId)");
 				$this->interface->blockAddress($address, 5);
+			}catch(\Throwable $e){
+				//record the name of the player who caused the crash, to make it easier to find the reproducing steps
+				$this->server->getLogger()->emergency("Crash occurred while handling a packet from session: $name");
+				throw $e;
 			}
 		}
 	}