Release 4.18.3

closes #5670

I'm not super happy with this fix, since it can still be broken if StandardPacketBroadcaster is replaced by something else. However, fixing that problem is probably going to require internal BC breaks, which are not suitable for a patch release.

changelogs/4.17.md

@@ -31,3 +31,9 @@ Released 22nd March 2023.
 - 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.
+
+# 4.17.2
+Released 29th March 2023.
+
+## Fixes
+- Fixed players being unable to join due to the appearance of a new `x5t` field in the JWT header of Xbox Live authentication tokens.

changelogs/4.18.md

@@ -90,3 +90,22 @@ Released 27th March 2023.
 - 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.
+
+# 4.18.2
+Released 29th March 2023.
+
+## Fixes
+- Fixed players being unable to join due to the appearance of a new `x5t` field in the JWT header of Xbox Live authentication tokens.
+- Fixed items' durability appearing to reset when moving them around in the inventory.
+
+# 4.18.3
+Released 5th April 2023.
+
+## Fixes
+- Fixed Average Players not being shown on timings reports when custom player classes are used.
+- Fixed incorrect tick violation calculation in timings reports.
+- Fixed not being able to add or remove items from the offhand slot.
+- Fixed creative inventory item count corruption when taking items (some players would see 64x items in the creative inventory after rejoining or changing gamemode).
+- Fixed not being able to drop items directly from the creative inventory on mobile.
+- Fixed `DataPacketReceiveEvent` not being called for packets sent by `EntityEventBroadcaster`.
+- `CreativeInventory::getItem()` and `CreativeInventory::getAll()` now return cloned itemstacks, to prevent accidental modification of the creative inventory.

composer.json

@@ -37,7 +37,7 @@
       "pocketmine/bedrock-block-upgrade-schema": "~1.1.1+bedrock-1.19.70",
       "pocketmine/bedrock-data": "~2.1.1+bedrock-1.19.70",
       "pocketmine/bedrock-item-upgrade-schema": "~1.1.0+bedrock-1.19.70",
-      "pocketmine/bedrock-protocol": "~20.1.0+bedrock-1.19.70",
+      "pocketmine/bedrock-protocol": "~20.1.1+bedrock-1.19.70",
       "pocketmine/binaryutils": "^0.2.1",
       "pocketmine/callback-validator": "^1.0.2",
       "pocketmine/classloader": "^0.2.0",

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": "956553d402f9c522cfcfe19260af2d2b",
+    "content-hash": "7c779ace575b5ef662eb5a6b0f173418",
     "packages": [
         {
             "name": "adhocore/json-comment",
@@ -328,16 +328,16 @@
         },
         {
             "name": "pocketmine/bedrock-protocol",
-            "version": "20.1.0+bedrock-1.19.70",
+            "version": "20.1.1+bedrock-1.19.70",
             "source": {
                 "type": "git",
                 "url": "https://github.com/pmmp/BedrockProtocol.git",
-                "reference": "91d67c8b1bced3c82d0841b1041c0c1f4e93eb68"
+                "reference": "455dbad93d29b4489b60910a41e38b8007b26563"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/91d67c8b1bced3c82d0841b1041c0c1f4e93eb68",
+                "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/455dbad93d29b4489b60910a41e38b8007b26563",
-                "reference": "91d67c8b1bced3c82d0841b1041c0c1f4e93eb68",
+                "reference": "455dbad93d29b4489b60910a41e38b8007b26563",
                 "shasum": ""
             },
             "require": {
@@ -369,9 +369,9 @@
             "description": "An implementation of the Minecraft: Bedrock Edition protocol in PHP",
             "support": {
                 "issues": "https://github.com/pmmp/BedrockProtocol/issues",
-                "source": "https://github.com/pmmp/BedrockProtocol/tree/20.1.0+bedrock-1.19.70"
+                "source": "https://github.com/pmmp/BedrockProtocol/tree/20.1.1+bedrock-1.19.70"
             },
-            "time": "2023-03-20T01:17:00+00:00"
+            "time": "2023-03-29T22:38:17+00:00"
         },
         {
             "name": "pocketmine/binaryutils",

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.3";
 	public const IS_DEVELOPMENT_BUILD = false;
 	public const BUILD_CHANNEL = "stable";
 

src/inventory/CreativeInventory.php

@@ -27,6 +27,7 @@ use pocketmine\item\Durable;
 use pocketmine\item\Item;
 use pocketmine\utils\Filesystem;
 use pocketmine\utils\SingletonTrait;
+use pocketmine\utils\Utils;
 use Symfony\Component\Filesystem\Path;
 use function json_decode;
 
@@ -60,11 +61,11 @@ final class CreativeInventory{
 	 * @return Item[]
 	 */
 	public function getAll() : array{
-		return $this->creative;
+		return Utils::cloneObjectArray($this->creative);
 	}
 
 	public function getItem(int $index) : ?Item{
-		return $this->creative[$index] ?? null;
+		return isset($this->creative[$index]) ? clone $this->creative[$index] : null;
 	}
 
 	public function getItemIndex(Item $item) : int{

src/network/mcpe/NetworkBroadcastUtils.php

@@ -23,7 +23,6 @@ declare(strict_types=1);
 
 namespace pocketmine\network\mcpe;
 
-use pocketmine\event\server\DataPacketSendEvent;
 use pocketmine\network\mcpe\protocol\ClientboundPacket;
 use pocketmine\player\Player;
 use pocketmine\timings\Timings;
@@ -57,13 +56,6 @@ final class NetworkBroadcastUtils{
 				return false;
 			}
 
-			$ev = new DataPacketSendEvent($sessions, $packets);
-			$ev->call();
-			if($ev->isCancelled()){
-				return false;
-			}
-			$sessions = $ev->getTargets();
-
 			/** @var PacketBroadcaster[] $uniqueBroadcasters */
 			$uniqueBroadcasters = [];
 			/** @var NetworkSession[][] $broadcasterTargets */

src/network/mcpe/StandardPacketBroadcaster.php

@@ -23,6 +23,7 @@ declare(strict_types=1);
 
 namespace pocketmine\network\mcpe;
 
+use pocketmine\event\server\DataPacketSendEvent;
 use pocketmine\network\mcpe\protocol\serializer\PacketBatch;
 use pocketmine\network\mcpe\protocol\serializer\PacketSerializer;
 use pocketmine\network\mcpe\protocol\serializer\PacketSerializerContext;
@@ -41,6 +42,15 @@ final class StandardPacketBroadcaster implements PacketBroadcaster{
 	){}
 
 	public function broadcastPackets(array $recipients, array $packets) : void{
+		//TODO: this shouldn't really be called here, since the broadcaster might be replaced by an alternative
+		//implementation that doesn't fire events
+		$ev = new DataPacketSendEvent($recipients, $packets);
+		$ev->call();
+		if($ev->isCancelled()){
+			return;
+		}
+		$packets = $ev->getPackets();
+
 		$compressors = [];
 
 		/** @var NetworkSession[][] $targetsByCompressor */

src/network/mcpe/handler/ItemStackRequestExecutor.php

@@ -113,7 +113,12 @@ class ItemStackRequestExecutor{
 	 */
 	protected function getBuilderInventoryAndSlot(ItemStackRequestSlotInfo $info) : array{
 		$windowId = ItemStackContainerIdTranslator::translate($info->getContainerId(), $this->inventoryManager->getCurrentWindowId());
-		$windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $info->getSlotId());
+		$slotId = $info->getSlotId();
+		if($info->getContainerId() === ContainerUIIds::OFFHAND && $slotId === 1){
+			//TODO: HACK! The client sends an incorrect slot ID for the offhand as of 1.19.70
+			$slotId = 0;
+		}
+		$windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId);
 		if($windowAndSlot === null){
 			throw new ItemStackRequestProcessException("No open inventory matches container UI ID: " . $info->getContainerId() . ", slot ID: " . $info->getSlotId());
 		}
@@ -142,6 +147,11 @@ class ItemStackRequestExecutor{
 	 * @throws ItemStackRequestProcessException
 	 */
 	protected function removeItemFromSlot(ItemStackRequestSlotInfo $slotInfo, int $count) : Item{
+		if($slotInfo->getContainerId() === ContainerUIIds::CREATED_OUTPUT && $slotInfo->getSlotId() === UIInventorySlotOffset::CREATED_ITEM_OUTPUT){
+			//special case for the "created item" output slot
+			//TODO: do we need to send a response for this slot info?
+			return $this->takeCreatedItem($count);
+		}
 		$this->requestSlotInfos[] = $slotInfo;
 		[$inventory, $slot] = $this->getBuilderInventoryAndSlot($slotInfo);
 		if($count < 1){
@@ -183,6 +193,13 @@ class ItemStackRequestExecutor{
 		$inventory->setItem($slot, $newItem);
 	}
 
+	protected function dropItem(Item $item, int $count) : void{
+		if($count < 1){
+			throw new ItemStackRequestProcessException("Cannot drop less than 1 of an item");
+		}
+		$this->builder->addAction(new DropItemAction((clone $item)->setCount($count)));
+	}
+
 	/**
 	 * @throws ItemStackRequestProcessException
 	 */
@@ -249,7 +266,7 @@ class ItemStackRequestExecutor{
 	/**
 	 * @throws ItemStackRequestProcessException
 	 */
-	protected function takeCreatedItem(ItemStackRequestSlotInfo $destination, int $count) : void{
+	protected function takeCreatedItem(int $count) : Item{
 		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");
@@ -267,10 +284,12 @@ class ItemStackRequestExecutor{
 		}
 
 		$this->createdItemsTakenCount += $count;
-		$this->addItemToSlot($destination, $createdItem, $count);
+		$createdItem = clone $createdItem;
+		$createdItem->setCount($count);
 		if(!$this->createdItemFromCreativeInventory && $this->createdItemsTakenCount >= $createdItem->getCount()){
 			$this->setNextCreatedItem(null);
 		}
+		return $createdItem;
 	}
 
 	/**
@@ -294,14 +313,7 @@ class ItemStackRequestExecutor{
 			$action instanceof TakeStackRequestAction ||
 			$action instanceof PlaceStackRequestAction
 		){
-			$source = $action->getSource();
+			$this->transferItems($action->getSource(), $action->getDestination(), $action->getCount());
-			$destination = $action->getDestination();
-
-			if($source->getContainerId() === ContainerUIIds::CREATED_OUTPUT && $source->getSlotId() === UIInventorySlotOffset::CREATED_ITEM_OUTPUT){
-				$this->takeCreatedItem($destination, $action->getCount());
-			}else{
-				$this->transferItems($source, $destination, $action->getCount());
-			}
 		}elseif($action instanceof SwapStackRequestAction){
 			$this->requestSlotInfos[] = $action->getSlot1();
 			$this->requestSlotInfos[] = $action->getSlot2();

src/network/mcpe/handler/ItemStackResponseBuilder.php

@@ -24,6 +24,7 @@ declare(strict_types=1);
 namespace pocketmine\network\mcpe\handler;
 
 use pocketmine\inventory\Inventory;
+use pocketmine\item\Durable;
 use pocketmine\network\mcpe\InventoryManager;
 use pocketmine\network\mcpe\protocol\types\inventory\ContainerUIIds;
 use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse;
@@ -52,6 +53,10 @@ final class ItemStackResponseBuilder{
 	 * @phpstan-return array{Inventory, int}
 	 */
 	private function getInventoryAndSlot(int $containerInterfaceId, int $slotId) : ?array{
+		if($containerInterfaceId === ContainerUIIds::OFFHAND && $slotId === 1){
+			//TODO: HACK! The client sends an incorrect slot ID for the offhand as of 1.19.70
+			$slotId = 0;
+		}
 		$windowId = ItemStackContainerIdTranslator::translate($containerInterfaceId, $this->inventoryManager->getCurrentWindowId());
 		$windowAndSlot = $this->inventoryManager->locateWindowAndSlot($windowId, $slotId);
 		if($windowAndSlot === null){
@@ -91,7 +96,7 @@ final class ItemStackResponseBuilder{
 					$item->getCount(),
 					$itemStackInfo->getStackId(),
 					$item->getCustomName(),
-					0
+					$item instanceof Durable ? $item->getDamage() : 0,
 				);
 			}
 		}

src/timings/Timings.php

@@ -27,6 +27,7 @@ use pocketmine\block\tile\Tile;
 use pocketmine\entity\Entity;
 use pocketmine\network\mcpe\protocol\ClientboundPacket;
 use pocketmine\network\mcpe\protocol\ServerboundPacket;
+use pocketmine\player\Player;
 use pocketmine\scheduler\TaskHandler;
 
 abstract class Timings{
@@ -236,8 +237,14 @@ abstract class Timings{
 	}
 
 	public static function getEntityTimings(Entity $entity) : TimingsHandler{
-		$entityType = (new \ReflectionClass($entity))->getShortName();
+		$reflect = new \ReflectionClass($entity);
+		$entityType = $reflect->getShortName();
 		if(!isset(self::$entityTypeTimingMap[$entityType])){
+			//the timings viewer calculates average player count by looking at this timer, so we need to ensure it has
+			//a name it can identify. However, we also want to make it obvious if this is a custom Player class.
+			if($entity instanceof Player && $reflect->getName() !== Player::class){
+				$entityType = "Player (" . $reflect->getName() . ")";
+			}
 			self::$entityTypeTimingMap[$entityType] = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Entity Tick - " . $entityType, self::$tickEntity);
 		}
 

src/timings/TimingsRecord.php

@@ -24,7 +24,7 @@ declare(strict_types=1);
 namespace pocketmine\timings;
 
 use pocketmine\Server;
-use function round;
+use function floor;
 use function spl_object_id;
 
 /**
@@ -56,7 +56,7 @@ final class TimingsRecord{
 		if($measure){
 			foreach(self::$records as $record){
 				if($record->curTickTotal > Server::TARGET_NANOSECONDS_PER_TICK){
-					$record->violations += (int) round($record->curTickTotal / Server::TARGET_NANOSECONDS_PER_TICK);
+					$record->violations += (int) floor($record->curTickTotal / Server::TARGET_NANOSECONDS_PER_TICK);
 				}
 				$record->curTickTotal = 0;
 				$record->curCount = 0;