From 2548422973a4b6f4417f9fe9b0e02164991ad583 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 20 Apr 2025 16:44:23 +0100 Subject: [PATCH 1/4] AvailableEnchantmentRegistry: reject non-string tags fixes https://crash.pmmp.io/view/12627328 --- src/item/enchantment/AvailableEnchantmentRegistry.php | 3 +++ src/utils/Utils.php | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/item/enchantment/AvailableEnchantmentRegistry.php b/src/item/enchantment/AvailableEnchantmentRegistry.php index eed7bff52..2d8dafa4b 100644 --- a/src/item/enchantment/AvailableEnchantmentRegistry.php +++ b/src/item/enchantment/AvailableEnchantmentRegistry.php @@ -28,6 +28,7 @@ use pocketmine\item\enchantment\ItemEnchantmentTags as Tags; use pocketmine\item\enchantment\VanillaEnchantments as Enchantments; use pocketmine\item\Item; use pocketmine\utils\SingletonTrait; +use pocketmine\utils\Utils; use function array_filter; use function array_values; use function count; @@ -129,6 +130,7 @@ final class AvailableEnchantmentRegistry{ if(!$this->isRegistered($enchantment)){ throw new \LogicException("Cannot set primary item tags for non-registered enchantment"); } + Utils::validateArrayValueType($tags, fn(string $v) => 1); $this->primaryItemTags[spl_object_id($enchantment)] = array_values($tags); } @@ -152,6 +154,7 @@ final class AvailableEnchantmentRegistry{ if(!$this->isRegistered($enchantment)){ throw new \LogicException("Cannot set secondary item tags for non-registered enchantment"); } + Utils::validateArrayValueType($tags, fn(string $v) => 1); $this->secondaryItemTags[spl_object_id($enchantment)] = array_values($tags); } diff --git a/src/utils/Utils.php b/src/utils/Utils.php index 046296cf4..800bd0183 100644 --- a/src/utils/Utils.php +++ b/src/utils/Utils.php @@ -584,7 +584,7 @@ final class Utils{ /** * @phpstan-template TMemberType * @phpstan-param array $array - * @phpstan-param \Closure(TMemberType) : void $validator + * @phpstan-param \Closure(TMemberType) : mixed $validator */ public static function validateArrayValueType(array $array, \Closure $validator) : void{ foreach(Utils::promoteKeys($array) as $k => $v){ From 4a5c1e75407f9cf8b37795210fc0957f9a37059a Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 20 Apr 2025 16:57:44 +0100 Subject: [PATCH 2/4] Entity: truncate fire ticks instead of throwing exceptions as written in the comments, it's not reasonable to propagate this limitation, since it ultimately comes from a shortfall in the Mojang save format, not a limitation of PM's capability. It's also not obvious how this would be propagated to the likes of setOnFire(), as this would translate into a max time of 1638 seconds, a value no one is going to remember. There's a case to be made for truncating this on save rather than on initial set, but this is at least better than having Fire Aspect level 1000 cause crashes and whatever other gameplay logic that would have to work around this stupid limitation. --- src/entity/Entity.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/entity/Entity.php b/src/entity/Entity.php index e24c6067c..97cdc19de 100644 --- a/src/entity/Entity.php +++ b/src/entity/Entity.php @@ -60,6 +60,7 @@ use pocketmine\player\Player; use pocketmine\Server; use pocketmine\timings\Timings; use pocketmine\timings\TimingsHandler; +use pocketmine\utils\Limits; use pocketmine\utils\Utils; use pocketmine\VersionInfo; use pocketmine\world\format\Chunk; @@ -700,9 +701,16 @@ abstract class Entity{ * @throws \InvalidArgumentException */ public function setFireTicks(int $fireTicks) : void{ - if($fireTicks < 0 || $fireTicks > 0x7fff){ - throw new \InvalidArgumentException("Fire ticks must be in range 0 ... " . 0x7fff . ", got $fireTicks"); + if($fireTicks < 0){ + throw new \InvalidArgumentException("Fire ticks cannot be negative"); } + + //Since the max value is not externally obvious or intuitive, many plugins use this without being aware that + //reasonably large values are not accepted. We even have such usages within PM itself. It doesn't make sense + //to force all those calls to be aware of this limitation, as it's not a functional limit but a limitation of + //the Mojang save format. Truncating this to the max acceptable value is the next best thing we can do. + $fireTicks = min($fireTicks, Limits::INT16_MAX); + if(!$this->isFireProof()){ $this->fireTicks = $fireTicks; $this->networkPropertiesDirty = true; From 1ea5c060fdd23023857cdc7f874a6562d117a72d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 20 Apr 2025 18:16:54 +0100 Subject: [PATCH 3/4] bruh --- src/entity/Entity.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/entity/Entity.php b/src/entity/Entity.php index 97cdc19de..6681558ad 100644 --- a/src/entity/Entity.php +++ b/src/entity/Entity.php @@ -77,6 +77,7 @@ use function floatval; use function floor; use function fmod; use function get_class; +use function min; use function sin; use function spl_object_id; use const M_PI_2; From ad6f7dfedb31a299d6d9c30961a76acf1f888f65 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sun, 20 Apr 2025 19:48:28 +0100 Subject: [PATCH 4/4] World: verify saveability of blocks, entities and tiles at entry points I want to do the same for items, but items are going to be a pain in the ass. For items there are multiple possible entry points and all of them will need to be checked: - dropped items - inventory contents - lecterns - item frames I don't see a good way to deal with all these. We can't check for registration in the constructor because we need to fully construct the item in order to register it. Blocks are also a potential issue in other areas, but setBlock() is definitely the biggest offender. --- src/block/tile/TileFactory.php | 7 +++++++ .../convert/BlockObjectToStateSerializer.php | 4 ++++ src/entity/EntityFactory.php | 7 +++++++ src/world/World.php | 17 +++++++++++++++++ 4 files changed, 35 insertions(+) diff --git a/src/block/tile/TileFactory.php b/src/block/tile/TileFactory.php index 515dd8c63..26e0af6a5 100644 --- a/src/block/tile/TileFactory.php +++ b/src/block/tile/TileFactory.php @@ -114,6 +114,13 @@ final class TileFactory{ $this->saveNames[$className] = reset($saveNames); } + /** + * @phpstan-param class-string $class + */ + public function isRegistered(string $class) : bool{ + return isset($this->saveNames[$class]); + } + /** * @internal * @throws SavedDataLoadingException diff --git a/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php b/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php index 367d38449..45784d409 100644 --- a/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php +++ b/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php @@ -225,6 +225,10 @@ final class BlockObjectToStateSerializer implements BlockStateSerializer{ return $this->cache[$stateId] ??= $this->serializeBlock(RuntimeBlockStateRegistry::getInstance()->fromStateId($stateId)); } + public function isRegistered(Block $block) : bool{ + return isset($this->serializers[$block->getTypeId()]); + } + /** * @phpstan-template TBlockType of Block * @phpstan-param TBlockType $block diff --git a/src/entity/EntityFactory.php b/src/entity/EntityFactory.php index 03d9c03e6..970fd986f 100644 --- a/src/entity/EntityFactory.php +++ b/src/entity/EntityFactory.php @@ -219,6 +219,13 @@ final class EntityFactory{ $this->saveNames[$className] = reset($saveNames); } + /** + * @phpstan-param class-string $class + */ + public function isRegistered(string $class) : bool{ + return isset($this->saveNames[$class]); + } + /** * Creates an entity from data stored on a chunk. * diff --git a/src/world/World.php b/src/world/World.php index 3a7d0c538..afd01c628 100644 --- a/src/world/World.php +++ b/src/world/World.php @@ -2047,6 +2047,15 @@ class World implements ChunkManager{ throw new WorldException("Cannot set a block in un-generated terrain"); } + //TODO: this computes state ID twice (we do it again in writeStateToWorld()). Not great for performance :( + $stateId = $block->getStateId(); + if(!$this->blockStateRegistry->hasStateId($stateId)){ + throw new \LogicException("Block state ID not known to RuntimeBlockStateRegistry (probably not registered)"); + } + if(!GlobalBlockStateHandlers::getSerializer()->isRegistered($block)){ + throw new \LogicException("Block not registered with GlobalBlockStateHandlers serializer"); + } + $this->timings->setBlock->startTiming(); $this->unlockChunk($chunkX, $chunkZ, null); @@ -2769,6 +2778,11 @@ class World implements ChunkManager{ throw new AssumptionFailedError("Found two different entities sharing entity ID " . $entity->getId()); } } + if(!EntityFactory::getInstance()->isRegistered($entity::class)){ + //canSaveWithChunk is mutable, so that means it could be toggled after adding the entity and cause a crash + //later on. Better we just force all entities to have a save ID, even if it might not be needed. + throw new \LogicException("Entity " . $entity::class . " is not registered for a save ID in EntityFactory"); + } $pos = $entity->getPosition()->asVector3(); $this->entitiesByChunk[World::chunkHash($pos->getFloorX() >> Chunk::COORD_BIT_SIZE, $pos->getFloorZ() >> Chunk::COORD_BIT_SIZE)][$entity->getId()] = $entity; $this->entityLastKnownPositions[$entity->getId()] = $pos; @@ -2870,6 +2884,9 @@ class World implements ChunkManager{ if(!$this->isInWorld($pos->getFloorX(), $pos->getFloorY(), $pos->getFloorZ())){ throw new \InvalidArgumentException("Tile position is outside the world bounds"); } + if(!TileFactory::getInstance()->isRegistered($tile::class)){ + throw new \LogicException("Tile " . $tile::class . " is not registered for a save ID in TileFactory"); + } $chunkX = $pos->getFloorX() >> Chunk::COORD_BIT_SIZE; $chunkZ = $pos->getFloorZ() >> Chunk::COORD_BIT_SIZE;