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

.github/workflows/build-docker-image.yml

@@ -20,6 +20,13 @@ jobs:
           username: ${{ secrets.DOCKERHUB_USERNAME }}
           password: ${{ secrets.DOCKERHUB_TOKEN }}
 
+      - name: Login to GitHub Container Registry
+        uses: docker/login-action@v2
+        with:
+          registry: ghcr.io
+          username: ${{ github.actor }}
+          password: ${{ secrets.GITHUB_TOKEN }}
+
       - name: Clone pmmp/PocketMine-Docker repository
         uses: actions/checkout@v3
         with:

changelogs/4.17.md

@@ -11,4 +11,23 @@ Released 14th March 2023.
 
 ## General
 - Added support for Minecraft: Bedrock Edition 1.19.70.
-- Removed support for older versions.
+- Removed support for older versions.
+
+# 4.17.1
+Released 22nd March 2023.
+
+## General
+- Docker images for PocketMine-MP are now published on [GitHub Container Registry](https://github.com/pmmp/PocketMine-MP/pkgs/container/pocketmine-mp). The Docker Hub images will stop being maintained in the future.
+- Updated translations.
+
+## Fixes
+- Fixed server crash on empty packets in certain cases.
+- Fixed mushroom blocks dropping the wrong items when broken with a silk-touch tool.
+- Fixed mushroom blocks giving the wrong items when block-picked.
+- Fixed missing ability flag `PRIVILEGED_BUILDER`.
+
+## Internals
+- `update-updater-api.yml` workflow now uses `github.repository_owner` to make it easier to test the workflow on forks.
+- Added version-specific channels to `update.pmmp.io`, such as `4`, `4.18-beta`, `4.17`, etc.
+- Replaced deprecated `::set-output` commands in GitHub Actions workflows.
+- `build/make-release.php` no longer automatically pushes changes, to avoid accidents when testing release workflows on forks.

changelogs/4.18-alpha.md

@@ -43,3 +43,49 @@ Released 16th March 2023.
 - `StandardPacketBroadcaster` is now locked to a single `PacketSerializer` context, reducing complexity.
 - Introduced `NetworkBroadcastUtils::broadcastPackets()`, replacing `Server->broadcastPackets()`.
 - `Server->broadcastPackets()` has been deprecated. It will be removed in a future version.
+
+# 4.18.0-ALPHA2
+Released 21st March 2023.
+
+## General
+- Included more sections of the network system in Player Network Send timings.
+- Changed the names of some timings to make them more user-friendly.
+- Removed packet IDs from `receivePacket` and `sendPacket` timings, as they were not very useful.
+- Added new specialized timers for the following:
+  - Item entity base ticking (merging)
+  - Player movement processing
+  - Entity movement processing (collision checking section)
+  - Projectile movement (all)
+  - Projectile movement processing (ray tracing section)
+
+## API
+### `pocketmine\crafting`
+- The following new API methods have been added:
+  - `CraftingManager->getCraftingRecipeIndex() : array<int, CraftingRecipe>` - returns a list of all crafting recipes
+  - `CraftingManager->getCraftingRecipeFromIndex(int $index) : ?CraftingRecipe` - returns the crafting recipe at the given index, or null if it doesn't exist
+
+### `pocketmine\inventory\transaction`
+- The following API methods have changed signatures:
+  - `CraftingTransaction->__construct()` now accepts additional arguments `?CraftingRecipe $recipe = null, ?int $repetitions = null`
+- The following new API methods have been added:
+  - `TransactionBuilderInventory->getActualInventory() : Inventory` - returns the actual inventory that this inventory is a proxy for
+
+## Internals
+### Network
+- Introduced support for the `ItemStackRequest` Minecraft: Bedrock network protocol.
+  - This fixes a large number of inventory- and crafting-related bugs.
+  - This also improves server security by closing off many code pathways that might have been used for exploits. `TypeConverter->netItemStackToCore()` is no longer used in server code, and remains for tool usage only.
+  - This system is also significantly more bandwidth-efficient and has lower overhead than the legacy system.
+  - This now opens the gateway to easily implement lots of gameplay features which have been missing for a long time, such as enchanting, anvils, looms, and more.
+  - Significant changes have been made to `pocketmine\network\mcpe\InventoryManager` internals. These shouldn't affect plugins, but may affect plugins which use internal network API.
+  - **No changes have been made to the plugin `InventoryTransaction` API**.
+    - This system has been implemented as a shim for the existing PocketMine-MP transaction system to preserve plugin compatibility. Plugins using `InventoryTransactionEvent` should continue to work seamlessly.
+    - The `InventoryTransaction` API will be redesigned in a future major version to make use of the new information provided by the `ItemStackRequest` system.
+  - `InventoryTransactionPacket` is no longer sent by the client for "regular" inventory actions. However, it is still sent when dropping items, interacting with blocks, and using items.
+- Inventory slot and content syncing is now buffered until the end of the tick. This reduces outbound network usage when the client performs multiple transactions in a single tick (e.g. crafting a stack of items).
+- Renamed some `InventoryManager` internal properties to make them easier to understand.
+- `TypeConverter->createInventoryAction()` has been removed.
+- 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.

composer.json

@@ -55,7 +55,7 @@
       "symfony/filesystem": "^5.4"
    },
    "require-dev": {
-      "phpstan/phpstan": "1.10.6",
+      "phpstan/phpstan": "1.10.7",
       "phpstan/phpstan-phpunit": "^1.1.0",
       "phpstan/phpstan-strict-rules": "^1.2.0",
       "phpunit/phpunit": "^9.2"

composer.lock

@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
         "This file is @generated automatically"
     ],
-    "content-hash": "43708f57042d7d40c732c3a3b87a4202",
+    "content-hash": "ebce35eefc0b925838fabd1fea39b0a5",
     "packages": [
         {
             "name": "adhocore/json-comment",
@@ -589,16 +589,16 @@
         },
         {
             "name": "pocketmine/locale-data",
-            "version": "2.19.0",
+            "version": "2.19.5",
             "source": {
                 "type": "git",
                 "url": "https://github.com/pmmp/Language.git",
-                "reference": "f47d1687f21f09d2858f040873184a11746b1cf7"
+                "reference": "71af5f9bd23b4e4bad8920dac7f4fe08e5205f7d"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/pmmp/Language/zipball/f47d1687f21f09d2858f040873184a11746b1cf7",
+                "url": "https://api.github.com/repos/pmmp/Language/zipball/71af5f9bd23b4e4bad8920dac7f4fe08e5205f7d",
-                "reference": "f47d1687f21f09d2858f040873184a11746b1cf7",
+                "reference": "71af5f9bd23b4e4bad8920dac7f4fe08e5205f7d",
                 "shasum": ""
             },
             "type": "library",
@@ -606,9 +606,9 @@
             "description": "Language resources used by PocketMine-MP",
             "support": {
                 "issues": "https://github.com/pmmp/Language/issues",
-                "source": "https://github.com/pmmp/Language/tree/2.19.0"
+                "source": "https://github.com/pmmp/Language/tree/2.19.5"
             },
-            "time": "2023-02-13T13:55:22+00:00"
+            "time": "2023-03-19T16:45:15+00:00"
         },
         {
             "name": "pocketmine/log",
@@ -1775,16 +1775,16 @@
         },
         {
             "name": "phpstan/phpstan",
-            "version": "1.10.6",
+            "version": "1.10.7",
             "source": {
                 "type": "git",
                 "url": "https://github.com/phpstan/phpstan.git",
-                "reference": "50d089a3e0904b0fe7e2cf2d4fd37d427d64235a"
+                "reference": "b10ceb526d9607903c5b2673f1fc8775dbe48975"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/phpstan/phpstan/zipball/50d089a3e0904b0fe7e2cf2d4fd37d427d64235a",
+                "url": "https://api.github.com/repos/phpstan/phpstan/zipball/b10ceb526d9607903c5b2673f1fc8775dbe48975",
-                "reference": "50d089a3e0904b0fe7e2cf2d4fd37d427d64235a",
+                "reference": "b10ceb526d9607903c5b2673f1fc8775dbe48975",
                 "shasum": ""
             },
             "require": {
@@ -1813,8 +1813,11 @@
                 "static analysis"
             ],
             "support": {
+                "docs": "https://phpstan.org/user-guide/getting-started",
+                "forum": "https://github.com/phpstan/phpstan/discussions",
                 "issues": "https://github.com/phpstan/phpstan/issues",
-                "source": "https://github.com/phpstan/phpstan/tree/1.10.6"
+                "security": "https://github.com/phpstan/phpstan/security/policy",
+                "source": "https://github.com/phpstan/phpstan-src"
             },
             "funding": [
                 {
@@ -1830,7 +1833,7 @@
                     "type": "tidelift"
                 }
             ],
-            "time": "2023-03-09T16:55:12+00:00"
+            "time": "2023-03-16T15:24:20+00:00"
         },
         {
             "name": "phpstan/phpstan-phpunit",

phpstan.neon.dist

@@ -4,7 +4,6 @@ includes:
 	- tests/phpstan/configs/impossible-generics.neon
 	- tests/phpstan/configs/php-bugs.neon
 	- tests/phpstan/configs/phpstan-bugs.neon
-	- tests/phpstan/configs/runtime-type-checks.neon
 	- tests/phpstan/configs/spl-fixed-array-sucks.neon
 	- vendor/phpstan/phpstan-phpunit/extension.neon
 	- vendor/phpstan/phpstan-phpunit/rules.neon

src/network/mcpe/NetworkSession.php

@@ -102,6 +102,7 @@ use pocketmine\player\XboxLivePlayerInfo;
 use pocketmine\Server;
 use pocketmine\timings\Timings;
 use pocketmine\utils\AssumptionFailedError;
+use pocketmine\utils\BinaryDataException;
 use pocketmine\utils\BinaryStream;
 use pocketmine\utils\ObjectSet;
 use pocketmine\utils\TextFormat;
@@ -393,7 +394,7 @@ class NetworkSession{
 						throw PacketHandlingException::wrap($e, "Error processing " . $packet->getName());
 					}
 				}
-			}catch(PacketDecodeException $e){
+			}catch(PacketDecodeException|BinaryDataException $e){
 				$this->logger->logException($e);
 				throw PacketHandlingException::wrap($e, "Packet batch decode error");
 			}

src/network/mcpe/handler/InGamePacketHandler.php

@@ -383,35 +383,51 @@ class InGamePacketHandler extends PacketHandler{
 			throw new PacketHandlingException("Expected exactly 2 actions for dropping an item");
 		}
 
+		$sourceSlot = null;
+		$clientItemStack = null;
+		$droppedCount = null;
+
 		foreach($data->getActions() as $networkInventoryAction){
-			if($networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_WORLD){
+			if($networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_WORLD && $networkInventoryAction->inventorySlot == NetworkInventoryAction::ACTION_MAGIC_SLOT_DROP_ITEM){
-				//drop item - we don't need to validate this, we only care about the count
+				$droppedCount = $networkInventoryAction->newItem->getItemStack()->getCount();
-				//if the resulting actions don't match the client for some reason, it will trigger an automatic
+				if($droppedCount <= 0){
-				//prediction rollback anyway.
+					throw new PacketHandlingException("Expected positive count for dropped item");
-				//it's technically possible to see this more than once, but a normal client should never do that.
-				$inventory = $this->player->getInventory();
-
-				$heldItemStack = TypeConverter::getInstance()->coreItemStackToNet($inventory->getItemInHand());
-				$droppedItemStack = $networkInventoryAction->newItem->getItemStack();
-				//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($heldItemStack->getCount() < $droppedItemStack->getCount() || !$heldItemStack->equalsWithoutCount($droppedItemStack)){
-					return false;
 				}
-
+			}elseif($networkInventoryAction->sourceType === NetworkInventoryAction::SOURCE_CONTAINER && $networkInventoryAction->windowId === ContainerIds::INVENTORY){
-				$newHeldItem = $inventory->getItemInHand();
+				//mobile players can drop an item from a non-selected hotbar slot
-				$droppedItem = $newHeldItem->pop($droppedItemStack->getCount());
+				$sourceSlot = $networkInventoryAction->inventorySlot;
-
+				$clientItemStack = $networkInventoryAction->oldItem->getItemStack();
-				$builder = new TransactionBuilder();
+			}else{
-				$builder->getInventory($inventory)->setItem($inventory->getHeldItemIndex(), $newHeldItem);
+				throw new PacketHandlingException("Unexpected action type in drop item transaction");
-				$builder->addAction(new DropItemAction($droppedItem));
-
-				$transaction = new InventoryTransaction($this->player, $builder->generateActions());
-				return $this->executeInventoryTransaction($transaction, $itemStackRequestId);
 			}
 		}
+		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");
+		}
 
-		throw new PacketHandlingException("Legacy 'normal' transactions should only be used for dropping items");
+		$inventory = $this->player->getInventory();
+
+		if(!$inventory->slotExists($sourceSlot)){
+			return false; //TODO: size desync??
+		}
+
+		$sourceSlotItem = $inventory->getItem($sourceSlot);
+		$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 :(
+		if(!$serverItemStack->equals($clientItemStack)){
+			return false;
+		}
+
+		//this modifies $sourceSlotItem
+		$droppedItem = $sourceSlotItem->pop($droppedCount);
+
+		$builder = new TransactionBuilder();
+		$builder->getInventory($inventory)->setItem($sourceSlot, $sourceSlotItem);
+		$builder->addAction(new DropItemAction($droppedItem));
+
+		$transaction = new InventoryTransaction($this->player, $builder->generateActions());
+		return $this->executeInventoryTransaction($transaction, $itemStackRequestId);
 	}
 
 	private function handleUseItemTransaction(UseItemTransactionData $data) : bool{

src/network/mcpe/handler/ItemStackRequestExecutor.php

@@ -58,7 +58,7 @@ use function array_key_first;
 use function count;
 use function spl_object_id;
 
-final class ItemStackRequestExecutor{
+class ItemStackRequestExecutor{
 	private TransactionBuilder $builder;
 
 	/** @var ItemStackRequestSlotInfo[] */
@@ -81,7 +81,7 @@ final class ItemStackRequestExecutor{
 		$this->builder = new TransactionBuilder();
 	}
 
-	private function prettyInventoryAndSlot(Inventory $inventory, int $slot) : string{
+	protected function prettyInventoryAndSlot(Inventory $inventory, int $slot) : string{
 		if($inventory instanceof TransactionBuilderInventory){
 			$inventory = $inventory->getActualInventory();
 		}
@@ -111,7 +111,7 @@ final class ItemStackRequestExecutor{
 	 *
 	 * @throws ItemStackRequestProcessException
 	 */
-	private function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{
+	protected function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{
 		$windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId());
 		$windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId());
 		if($windowAndSlot === null){
@@ -129,7 +129,10 @@ final class ItemStackRequestExecutor{
 		return [$this->builder->getInventory($inventory), $slot];
 	}
 
-	private function transferItems(ItemStackRequestSlotInfo $source, ItemStackRequestSlotInfo $destination, int $count) : void{
+	/**
+	 * @throws ItemStackRequestProcessException
+	 */
+	protected function transferItems(ItemStackRequestSlotInfo $source, ItemStackRequestSlotInfo $destination, int $count) : void{
 		$removed = $this->removeItemFromSlot($source, $count);
 		$this->addItemToSlot($destination, $removed, $count);
 	}
@@ -138,9 +141,13 @@ 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{
+	protected function removeItemFromSlot(ItemStackRequestSlotInfo $slotInfo, int $count) : Item{
 		$this->requestSlotInfos[] = $slotInfo;
 		[$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo);
+		if($count < 1){
+			//this should be impossible at the protocol level, but in case of buggy core code this will prevent exploits
+			throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Cannot take less than 1 items from a stack");
+		}
 
 		$existingItem = $inventory->getItem($slot);
 		if($existingItem->getCount() < $count){
@@ -155,10 +162,15 @@ final class ItemStackRequestExecutor{
 
 	/**
 	 * Adds items to the target slot, if they are stackable.
+	 * @throws ItemStackRequestProcessException
 	 */
-	private function addItemToSlot(ItemStackRequestSlotInfo $slotInfo, Item $item, int $count) : void{
+	protected function addItemToSlot(ItemStackRequestSlotInfo $slotInfo, Item $item, int $count) : void{
 		$this->requestSlotInfos[] = $slotInfo;
 		[$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo);
+		if($count < 1){
+			//this should be impossible at the protocol level, but in case of buggy core code this will prevent exploits
+			throw new ItemStackRequestProcessException($this->prettyInventoryAndSlot($inventory, $slot) . ": Cannot take less than 1 items from a stack");
+		}
 
 		$existingItem = $inventory->getItem($slot);
 		if(!$existingItem->isNull() && !$existingItem->canStackWith($item)){
@@ -174,7 +186,7 @@ final class ItemStackRequestExecutor{
 	/**
 	 * @throws ItemStackRequestProcessException
 	 */
-	private function setNextCreatedItem(?Item $item, bool $creative = false) : void{
+	protected function setNextCreatedItem(?Item $item, bool $creative = false) : void{
 		if($item !== null && $item->isNull()){
 			$item = null;
 		}
@@ -196,13 +208,19 @@ final class ItemStackRequestExecutor{
 	/**
 	 * @throws ItemStackRequestProcessException
 	 */
-	private function beginCrafting(int $recipeId, int $repetitions) : void{
+	protected function beginCrafting(int $recipeId, int $repetitions) : void{
 		if($this->specialTransaction !== null){
 			throw new ItemStackRequestProcessException("Another special transaction is already in progress");
 		}
-		if($repetitions < 1){ //TODO: upper bound?
+		if($repetitions < 1){
 			throw new ItemStackRequestProcessException("Cannot craft a recipe less than 1 time");
 		}
+		if($repetitions > 256){
+			//TODO: we can probably lower this limit to 64, but I'm unsure if there are cases where the client may
+			//request more than 64 repetitions of a recipe.
+			//It's already hard-limited to 256 repetitions in the protocol, so this is just a sanity check.
+			throw new ItemStackRequestProcessException("Cannot craft a recipe more than 256 times");
+		}
 		$craftingManager = $this->player->getServer()->getCraftingManager();
 		$recipe = $craftingManager->getCraftingRecipeFromIndex($recipeId);
 		if($recipe === null){
@@ -228,7 +246,14 @@ final class ItemStackRequestExecutor{
 		}
 	}
 
-	private function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{
+	/**
+	 * @throws ItemStackRequestProcessException
+	 */
+	protected function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{
+		if($count < 1){
+			//this should be impossible at the protocol level, but in case of buggy core code this will prevent exploits
+			throw new ItemStackRequestProcessException("Cannot take less than 1 created item");
+		}
 		$createdItem = $this->nextCreatedItem;
 		if($createdItem === null){
 			throw new ItemStackRequestProcessException("No created item is waiting to be taken");
@@ -264,7 +289,7 @@ final class ItemStackRequestExecutor{
 	/**
 	 * @throws ItemStackRequestProcessException
 	 */
-	private function processItemStackRequestAction(ItemStackRequestAction $action) : void{
+	protected function processItemStackRequestAction(ItemStackRequestAction $action) : void{
 		if(
 			$action instanceof TakeStackRequestAction ||
 			$action instanceof PlaceStackRequestAction

tests/phpstan/configs/runtime-type-checks.neon

@@ -1,7 +0,0 @@
-parameters:
-	ignoreErrors:
-		-
-			message: "#^Instanceof between pocketmine\\\\nbt\\\\tag\\\\CompoundTag and pocketmine\\\\nbt\\\\tag\\\\CompoundTag will always evaluate to true\\.$#"
-			count: 1
-			path: ../../../src/network/mcpe/handler/InGamePacketHandler.php
-