diff --git a/build/php b/build/php index a464454d1..9d8807be8 160000 --- a/build/php +++ b/build/php @@ -1 +1 @@ -Subproject commit a464454d1ed946baabd32a02ddcf66361374b99c +Subproject commit 9d8807be825b3fafd420534f2c29351c1bda6398 diff --git a/changelogs/4.18.md b/changelogs/4.18.md index b7b7509a6..4007b206c 100644 --- a/changelogs/4.18.md +++ b/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. \ No newline at end of file +- 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. \ No newline at end of file diff --git a/src/event/Event.php b/src/event/Event.php index 9d065be7e..8e1276c41 100644 --- a/src/event/Event.php +++ b/src/event/Event.php @@ -26,6 +26,7 @@ declare(strict_types=1); */ namespace pocketmine\event; +use pocketmine\timings\Timings; use function get_class; abstract class Event{ @@ -50,6 +51,9 @@ abstract class Event{ throw new \RuntimeException("Recursive event call detected (reached max depth of " . self::MAX_EVENT_CALL_DEPTH . " calls)"); } + $timings = Timings::getEventTimings($this); + $timings->startTiming(); + $handlerList = HandlerListManager::global()->getListFor(get_class($this)); ++self::$eventCallDepth; @@ -66,6 +70,7 @@ abstract class Event{ } }finally{ --self::$eventCallDepth; + $timings->stopTiming(); } } } diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 53293881b..c8434f753 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -582,9 +582,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( diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 43a0d2bb8..7b8776b9f 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -374,13 +374,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; @@ -398,11 +406,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(); @@ -412,6 +422,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 :( @@ -542,8 +555,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); diff --git a/src/network/mcpe/raklib/RakLibInterface.php b/src/network/mcpe/raklib/RakLibInterface.php index 6092ac42c..f63099e8a 100644 --- a/src/network/mcpe/raklib/RakLibInterface.php +++ b/src/network/mcpe/raklib/RakLibInterface.php @@ -199,6 +199,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){ @@ -209,6 +210,10 @@ class RakLibInterface implements ServerEventListener, AdvancedNetworkInterface{ $logger->debug(implode("\n", Utils::printableExceptionInfo($e))); $session->disconnectWithError(KnownTranslationFactory::pocketmine_disconnect_error_badPacket()); $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; } } } diff --git a/src/plugin/PluginManager.php b/src/plugin/PluginManager.php index 475533f7b..34a5d816a 100644 --- a/src/plugin/PluginManager.php +++ b/src/plugin/PluginManager.php @@ -655,7 +655,7 @@ class PluginManager{ } $prefix = $plugin->getDescription()->getSrcNamespacePrefix(); - if(str_starts_with($handlerName, $prefix)){ + if(str_starts_with($handlerName, $prefix) && $prefix !== ""){ $handlerName = substr($handlerName, strlen($prefix) + 1); } diff --git a/src/timings/Timings.php b/src/timings/Timings.php index e62066ea4..1d64e67df 100644 --- a/src/timings/Timings.php +++ b/src/timings/Timings.php @@ -25,9 +25,12 @@ namespace pocketmine\timings; use pocketmine\block\tile\Tile; use pocketmine\entity\Entity; +use pocketmine\event\Event; use pocketmine\network\mcpe\protocol\ClientboundPacket; use pocketmine\network\mcpe\protocol\ServerboundPacket; use pocketmine\scheduler\TaskHandler; +use function get_class; +use function str_starts_with; abstract class Timings{ public const INCLUDED_BY_OTHER_TIMINGS_PREFIX = "** "; @@ -111,6 +114,9 @@ abstract class Timings{ public static TimingsHandler $playerMove; + /** @var TimingsHandler[] */ + private static array $events = []; + public static function init() : void{ if(self::$initialized){ return; @@ -263,4 +269,18 @@ abstract class Timings{ return self::$commandTimingMap[$commandName] ??= new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Command - " . $commandName); } + + public static function getEventTimings(Event $event) : TimingsHandler{ + $eventClass = get_class($event); + if(!isset(self::$events[$eventClass])){ + if(str_starts_with($eventClass, "pocketmine\\event\\")){ + $name = (new \ReflectionClass($event))->getShortName(); + }else{ + $name = $eventClass; + } + self::$events[$eventClass] = new TimingsHandler($name, group: "Events"); + } + + return self::$events[$eventClass]; + } }