diff --git a/changelogs/4.18.md b/changelogs/4.18.md index 890f00d6d..940a855bd 100644 --- a/changelogs/4.18.md +++ b/changelogs/4.18.md @@ -109,3 +109,15 @@ Released 5th April 2023. - 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. + +# 4.18.4 +Released 10th April 2023. + +## Fixes +- Fixed movement becoming broken when the player moves at high speed (e.g. due to high levels of the Speed effect). +- Updated dependencies to get fixes in `pocketmine/nbt` and `pocketmine/bedrock-protocol`. + +## Internals +### Network +- Game packets are now rate-limited in a similar manner to packet batches. This helps to more effectively mitigate certain types of DoS attacks. +- Added a new class `PacketRateLimiter`, implementing functionality previously baked directly into `NetworkSession` in a more generic way to allow reuse. diff --git a/composer.lock b/composer.lock index 17a18e11e..9dacda0ef 100644 --- a/composer.lock +++ b/composer.lock @@ -199,16 +199,16 @@ }, { "name": "netresearch/jsonmapper", - "version": "v4.1.0", + "version": "v4.2.0", "source": { "type": "git", "url": "https://github.com/cweiske/jsonmapper.git", - "reference": "cfa81ea1d35294d64adb9c68aa4cb9e92400e53f" + "reference": "f60565f8c0566a31acf06884cdaa591867ecc956" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/cweiske/jsonmapper/zipball/cfa81ea1d35294d64adb9c68aa4cb9e92400e53f", - "reference": "cfa81ea1d35294d64adb9c68aa4cb9e92400e53f", + "url": "https://api.github.com/repos/cweiske/jsonmapper/zipball/f60565f8c0566a31acf06884cdaa591867ecc956", + "reference": "f60565f8c0566a31acf06884cdaa591867ecc956", "shasum": "" }, "require": { @@ -244,9 +244,9 @@ "support": { "email": "cweiske@cweiske.de", "issues": "https://github.com/cweiske/jsonmapper/issues", - "source": "https://github.com/cweiske/jsonmapper/tree/v4.1.0" + "source": "https://github.com/cweiske/jsonmapper/tree/v4.2.0" }, - "time": "2022-12-08T20:46:14+00:00" + "time": "2023-04-09T17:37:40+00:00" }, { "name": "pocketmine/bedrock-block-upgrade-schema", @@ -328,16 +328,16 @@ }, { "name": "pocketmine/bedrock-protocol", - "version": "20.1.1+bedrock-1.19.70", + "version": "20.1.2+bedrock-1.19.70", "source": { "type": "git", "url": "https://github.com/pmmp/BedrockProtocol.git", - "reference": "455dbad93d29b4489b60910a41e38b8007b26563" + "reference": "2787c531039b3d92fa3ec92f28b95158dc24b915" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/455dbad93d29b4489b60910a41e38b8007b26563", - "reference": "455dbad93d29b4489b60910a41e38b8007b26563", + "url": "https://api.github.com/repos/pmmp/BedrockProtocol/zipball/2787c531039b3d92fa3ec92f28b95158dc24b915", + "reference": "2787c531039b3d92fa3ec92f28b95158dc24b915", "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.1+bedrock-1.19.70" + "source": "https://github.com/pmmp/BedrockProtocol/tree/20.1.2+bedrock-1.19.70" }, - "time": "2023-03-29T22:38:17+00:00" + "time": "2023-04-10T11:40:32+00:00" }, { "name": "pocketmine/binaryutils", @@ -512,23 +512,23 @@ }, { "name": "pocketmine/color", - "version": "0.3.0", + "version": "0.3.1", "source": { "type": "git", "url": "https://github.com/pmmp/Color.git", - "reference": "8cb346d0a21ad3287cc8d7175e4b643416607249" + "reference": "a0421f1e9e0b0c619300fb92d593283378f6a5e1" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/Color/zipball/8cb346d0a21ad3287cc8d7175e4b643416607249", - "reference": "8cb346d0a21ad3287cc8d7175e4b643416607249", + "url": "https://api.github.com/repos/pmmp/Color/zipball/a0421f1e9e0b0c619300fb92d593283378f6a5e1", + "reference": "a0421f1e9e0b0c619300fb92d593283378f6a5e1", "shasum": "" }, "require": { "php": "^8.0" }, "require-dev": { - "phpstan/phpstan": "1.9.4", + "phpstan/phpstan": "1.10.3", "phpstan/phpstan-strict-rules": "^1.2.0" }, "type": "library", @@ -544,9 +544,9 @@ "description": "Color handling library used by PocketMine-MP and related projects", "support": { "issues": "https://github.com/pmmp/Color/issues", - "source": "https://github.com/pmmp/Color/tree/0.3.0" + "source": "https://github.com/pmmp/Color/tree/0.3.1" }, - "time": "2022-12-18T19:49:21+00:00" + "time": "2023-04-10T11:38:05+00:00" }, { "name": "pocketmine/errorhandler", @@ -738,16 +738,16 @@ }, { "name": "pocketmine/nbt", - "version": "0.3.3", + "version": "0.3.4", "source": { "type": "git", "url": "https://github.com/pmmp/NBT.git", - "reference": "f4321be50df1a18b9f4e94d428a2e68a6e2ac2b4" + "reference": "62c02464c6708b2467c1e1a2af01af09d5114eda" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/pmmp/NBT/zipball/f4321be50df1a18b9f4e94d428a2e68a6e2ac2b4", - "reference": "f4321be50df1a18b9f4e94d428a2e68a6e2ac2b4", + "url": "https://api.github.com/repos/pmmp/NBT/zipball/62c02464c6708b2467c1e1a2af01af09d5114eda", + "reference": "62c02464c6708b2467c1e1a2af01af09d5114eda", "shasum": "" }, "require": { @@ -757,7 +757,7 @@ }, "require-dev": { "phpstan/extension-installer": "^1.0", - "phpstan/phpstan": "1.7.7", + "phpstan/phpstan": "1.10.3", "phpstan/phpstan-strict-rules": "^1.0", "phpunit/phpunit": "^9.5" }, @@ -774,9 +774,9 @@ "description": "PHP library for working with Named Binary Tags", "support": { "issues": "https://github.com/pmmp/NBT/issues", - "source": "https://github.com/pmmp/NBT/tree/0.3.3" + "source": "https://github.com/pmmp/NBT/tree/0.3.4" }, - "time": "2022-07-06T14:13:26+00:00" + "time": "2023-04-10T11:31:20+00:00" }, { "name": "pocketmine/raklib", diff --git a/src/VersionInfo.php b/src/VersionInfo.php index 61ec22aec..723613b39 100644 --- a/src/VersionInfo.php +++ b/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.4"; + public const BASE_VERSION = "4.18.5"; public const IS_DEVELOPMENT_BUILD = true; public const BUILD_CHANNEL = "stable"; diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php index 92b916326..7f07fbd77 100644 --- a/src/network/mcpe/NetworkSession.php +++ b/src/network/mcpe/NetworkSession.php @@ -114,11 +114,8 @@ use function base64_encode; use function bin2hex; use function count; use function get_class; -use function hrtime; use function in_array; -use function intdiv; use function json_encode; -use function min; use function strcasecmp; use function strlen; use function strtolower; @@ -128,19 +125,14 @@ use function ucfirst; use const JSON_THROW_ON_ERROR; class NetworkSession{ - private const INCOMING_PACKET_BATCH_PER_TICK = 2; //usually max 1 per tick, but transactions may arrive separately - private const INCOMING_PACKET_BATCH_MAX_BUDGET = 100 * self::INCOMING_PACKET_BATCH_PER_TICK; //enough to account for a 5-second lag spike + private const INCOMING_PACKET_BATCH_PER_TICK = 2; //usually max 1 per tick, but transactions arrive separately + private const INCOMING_PACKET_BATCH_BUFFER_TICKS = 100; //enough to account for a 5-second lag spike - /** - * At most this many more packets can be received. If this reaches zero, any additional packets received will cause - * the player to be kicked from the server. - * This number is increased every tick up to a maximum limit. - * - * @see self::INCOMING_PACKET_BATCH_PER_TICK - * @see self::INCOMING_PACKET_BATCH_MAX_BUDGET - */ - private int $incomingPacketBatchBudget = self::INCOMING_PACKET_BATCH_MAX_BUDGET; - private int $lastPacketBudgetUpdateTimeNs; + private const INCOMING_GAME_PACKETS_PER_TICK = 2; + private const INCOMING_GAME_PACKETS_BUFFER_TICKS = 100; + + private PacketRateLimiter $packetBatchLimiter; + private PacketRateLimiter $gamePacketLimiter; private \PrefixedLogger $logger; private ?Player $player = null; @@ -196,7 +188,8 @@ class NetworkSession{ $this->disposeHooks = new ObjectSet(); $this->connectTime = time(); - $this->lastPacketBudgetUpdateTimeNs = hrtime(true); + $this->packetBatchLimiter = new PacketRateLimiter("Packet Batches", self::INCOMING_PACKET_BATCH_PER_TICK, self::INCOMING_PACKET_BATCH_BUFFER_TICKS); + $this->gamePacketLimiter = new PacketRateLimiter("Game Packets", self::INCOMING_GAME_PACKETS_PER_TICK, self::INCOMING_GAME_PACKETS_BUFFER_TICKS); $this->setHandler(new SessionStartPacketHandler( $this->server, @@ -339,13 +332,7 @@ class NetworkSession{ Timings::$playerNetworkReceive->startTiming(); try{ - if($this->incomingPacketBatchBudget <= 0){ - $this->updatePacketBudget(); - if($this->incomingPacketBatchBudget <= 0){ - throw new PacketHandlingException("Receiving packets too fast"); - } - } - $this->incomingPacketBatchBudget--; + $this->packetBatchLimiter->decrement(); if($this->cipher !== null){ Timings::$playerNetworkReceiveDecrypt->startTiming(); @@ -377,6 +364,7 @@ class NetworkSession{ $stream = new BinaryStream($decompressed); $count = 0; foreach(PacketBatch::decodeRaw($stream) as $buffer){ + $this->gamePacketLimiter->decrement(); if(++$count > 100){ throw new PacketHandlingException("Too many packets in batch"); } @@ -1105,23 +1093,6 @@ class NetworkSession{ $this->sendDataPacket(ToastRequestPacket::create($title, $body)); } - private function updatePacketBudget() : void{ - $nowNs = hrtime(true); - $timeSinceLastUpdateNs = $nowNs - $this->lastPacketBudgetUpdateTimeNs; - if($timeSinceLastUpdateNs > 50_000_000){ - $ticksSinceLastUpdate = intdiv($timeSinceLastUpdateNs, 50_000_000); - /* - * If the server takes an abnormally long time to process a tick, add the budget for time difference to - * compensate. This extra budget may be very large, but it will disappear the next time a normal update - * occurs. This ensures that backlogs during a large lag spike don't cause everyone to get kicked. - * As long as all the backlogged packets are processed before the next tick, everything should be OK for - * clients behaving normally. - */ - $this->incomingPacketBatchBudget = min($this->incomingPacketBatchBudget, self::INCOMING_PACKET_BATCH_MAX_BUDGET) + (self::INCOMING_PACKET_BATCH_PER_TICK * 2 * $ticksSinceLastUpdate); - $this->lastPacketBudgetUpdateTimeNs = $nowNs; - } - } - public function tick() : void{ if(!$this->isConnected()){ $this->dispose(); diff --git a/src/network/mcpe/PacketRateLimiter.php b/src/network/mcpe/PacketRateLimiter.php new file mode 100644 index 000000000..3f0fbf768 --- /dev/null +++ b/src/network/mcpe/PacketRateLimiter.php @@ -0,0 +1,81 @@ +maxBudget = $this->averagePerTick * $maxBufferTicks; + $this->budget = $this->maxBudget; + $this->lastUpdateTimeNs = hrtime(true); + } + + /** + * @throws PacketHandlingException if the rate limit has been exceeded + */ + public function decrement(int $amount = 1) : void{ + if($this->budget <= 0){ + $this->update(); + if($this->budget <= 0){ + throw new PacketHandlingException("Exceeded rate limit for \"$this->name\""); + } + } + $this->budget -= $amount; + } + + public function update() : void{ + $nowNs = hrtime(true); + $timeSinceLastUpdateNs = $nowNs - $this->lastUpdateTimeNs; + if($timeSinceLastUpdateNs > $this->updateFrequencyNs){ + $ticksSinceLastUpdate = intdiv($timeSinceLastUpdateNs, $this->updateFrequencyNs); + /* + * If the server takes an abnormally long time to process a tick, add the budget for time difference to + * compensate. This extra budget may be very large, but it will disappear the next time a normal update + * occurs. This ensures that backlogs during a large lag spike don't cause everyone to get kicked. + * As long as all the backlogged packets are processed before the next tick, everything should be OK for + * clients behaving normally. + */ + $this->budget = min($this->budget, $this->maxBudget) + ($this->averagePerTick * 2 * $ticksSinceLastUpdate); + $this->lastUpdateTimeNs = $nowNs; + } + } +} diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 5ee45347e..37f23e158 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -214,7 +214,7 @@ class InGamePacketHandler extends PacketHandler{ if($newPos->distanceSquared($curPos) > 1){ //Tolerate up to 1 block to avoid problems with client-sided physics when spawning in blocks $this->session->getLogger()->debug("Got outdated pre-teleport movement, received " . $newPos . ", expected " . $curPos); //Still getting movements from before teleport, ignore them - return false; + return true; } // Once we get a movement within a reasonable distance, treat it as a teleport ACK and remove position lock diff --git a/src/player/Player.php b/src/player/Player.php index 52eabac17..00f85b9ff 100644 --- a/src/player/Player.php +++ b/src/player/Player.php @@ -1231,7 +1231,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ $revert = false; - if($distanceSquared > 100){ + if($distanceSquared > 225){ //15 blocks //TODO: this is probably too big if we process every movement /* !!! BEWARE YE WHO ENTER HERE !!! * @@ -1243,7 +1243,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ * If you must tamper with this code, be aware that this can cause very nasty results. Do not waste our time * asking for help if you suffer the consequences of messing with this. */ - $this->logger->debug("Moved too fast, reverting movement"); + $this->logger->debug("Moved too fast (" . sqrt($distanceSquared) . " blocks in 1 movement), reverting movement"); $this->logger->debug("Old position: " . $oldPos->asVector3() . ", new position: " . $newPos); $revert = true; }elseif(!$this->getWorld()->isInLoadedTerrain($newPos)){