Clean up Skin error handling, close #2926

Skin->__construct() now does all the validation.

src/pocketmine/Player.php

@@ -820,10 +820,6 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	 * @return bool
 	 */
 	public function changeSkin(Skin $skin, string $newSkinName, string $oldSkinName) : bool{
-		if(!$skin->isValid()){
-			return false;
-		}
-
 		$ev = new PlayerChangeSkinEvent($this, $this->getSkin(), $skin);
 		$ev->call();
 

src/pocketmine/entity/Human.php

@@ -105,30 +105,18 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 			if($skinTag === null){
 				throw new \InvalidStateException((new \ReflectionClass($this))->getShortName() . " must have a valid skin set");
 			}
-			$this->skin = self::deserializeSkinNBT($skinTag); //this throws if the skin is invalid
+			$this->skin = new Skin( //this throws if the skin is invalid
+				$skinTag->getString("Name"),
+				$skinTag->hasTag("Data", StringTag::class) ? $skinTag->getString("Data") : $skinTag->getByteArray("Data"), //old data (this used to be saved as a StringTag in older versions of PM)
+				$skinTag->getByteArray("CapeData", ""),
+				$skinTag->getString("GeometryName", ""),
+				$skinTag->getByteArray("GeometryData", "")
+			);
 		}
 
 		parent::__construct($world, $nbt);
 	}
 
-	/**
-	 * @param CompoundTag $skinTag
-	 *
-	 * @return Skin
-	 * @throws \InvalidArgumentException
-	 */
-	protected static function deserializeSkinNBT(CompoundTag $skinTag) : Skin{
-		$skin = new Skin(
-			$skinTag->getString("Name"),
-			$skinTag->hasTag("Data", StringTag::class) ? $skinTag->getString("Data") : $skinTag->getByteArray("Data"), //old data (this used to be saved as a StringTag in older versions of PM)
-			$skinTag->getByteArray("CapeData", ""),
-			$skinTag->getString("GeometryName", ""),
-			$skinTag->getByteArray("GeometryData", "")
-		);
-		$skin->validate();
-		return $skin;
-	}
-
 	/**
 	 * @deprecated
 	 *
@@ -171,7 +159,6 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 	 * @param Skin $skin
 	 */
 	public function setSkin(Skin $skin) : void{
-		$skin->validate();
 		$this->skin = $skin;
 		$this->skin->debloatGeometryData();
 	}
@@ -841,8 +828,6 @@ class Human extends Creature implements ProjectileSource, InventoryHolder{
 	}
 
 	protected function sendSpawnPacket(Player $player) : void{
-		$this->skin->validate();
-
 		if(!($this instanceof Player)){
 			/* we don't use Server->updatePlayerListData() because that uses batches, which could cause race conditions in async compression mode */
 			$pk = new PlayerListPacket();

src/pocketmine/entity/Skin.php

@@ -48,6 +48,18 @@ class Skin{
 	private $geometryData;
 
 	public function __construct(string $skinId, string $skinData, string $capeData = "", string $geometryName = "", string $geometryData = ""){
+		if($skinId === ""){
+			throw new \InvalidArgumentException("Skin ID must not be empty");
+		}
+		$len = strlen($skinData);
+		if(!in_array($len, self::ACCEPTED_SKIN_SIZES, true)){
+			throw new \InvalidArgumentException("Invalid skin data size $len bytes (allowed sizes: " . implode(", ", self::ACCEPTED_SKIN_SIZES) . ")");
+		}
+		if($capeData !== "" and strlen($capeData) !== 8192){
+			throw new \InvalidArgumentException("Invalid cape data size " . strlen($capeData) . " bytes (must be exactly 8192 bytes)");
+		}
+		//TODO: validate geometry
+
 		$this->skinId = $skinId;
 		$this->skinData = $skinData;
 		$this->capeData = $capeData;
@@ -55,36 +67,6 @@ class Skin{
 		$this->geometryData = $geometryData;
 	}
 
-	/**
-	 * @deprecated
-	 * @return bool
-	 */
-	public function isValid() : bool{
-		try{
-			$this->validate();
-			return true;
-		}catch(\InvalidArgumentException $e){
-			return false;
-		}
-	}
-
-	/**
-	 * @throws \InvalidArgumentException
-	 */
-	public function validate() : void{
-		if($this->skinId === ""){
-			throw new \InvalidArgumentException("Skin ID must not be empty");
-		}
-		$len = strlen($this->skinData);
-		if(!in_array($len, self::ACCEPTED_SKIN_SIZES, true)){
-			throw new \InvalidArgumentException("Invalid skin data size $len bytes (allowed sizes: " . implode(", ", self::ACCEPTED_SKIN_SIZES) . ")");
-		}
-		if($this->capeData !== "" and strlen($this->capeData) !== 8192){
-			throw new \InvalidArgumentException("Invalid cape data size " . strlen($this->capeData) . " bytes (must be exactly 8192 bytes)");
-		}
-		//TODO: validate geometry
-	}
-
 	/**
 	 * @return string
 	 */

src/pocketmine/event/player/PlayerChangeSkinEvent.php

@@ -70,7 +70,6 @@ class PlayerChangeSkinEvent extends PlayerEvent implements Cancellable{
 	 * @throws \InvalidArgumentException if the specified skin is not valid
 	 */
 	public function setNewSkin(Skin $skin) : void{
-		$skin->validate();
 		$this->newSkin = $skin;
 	}
 }

src/pocketmine/network/mcpe/handler/LoginSessionHandler.php

@@ -75,14 +75,16 @@ class LoginSessionHandler extends SessionHandler{
 			return true;
 		}
 
-		$skin = new Skin(
+		try{
-			$packet->clientData[LoginPacket::I_SKIN_ID],
+			$skin = new Skin(
-			base64_decode($packet->clientData[LoginPacket::I_SKIN_DATA]),
+				$packet->clientData[LoginPacket::I_SKIN_ID],
-			base64_decode($packet->clientData[LoginPacket::I_CAPE_DATA]),
+				base64_decode($packet->clientData[LoginPacket::I_SKIN_DATA]),
-			$packet->clientData[LoginPacket::I_GEOMETRY_NAME],
+				base64_decode($packet->clientData[LoginPacket::I_CAPE_DATA]),
-			base64_decode($packet->clientData[LoginPacket::I_GEOMETRY_DATA])
+				$packet->clientData[LoginPacket::I_GEOMETRY_NAME],
-		);
+				base64_decode($packet->clientData[LoginPacket::I_GEOMETRY_DATA])
-		if(!$skin->isValid()){
+			);
+		}catch(\InvalidArgumentException $e){
+			$this->session->getLogger()->debug("Invalid skin: " . $e->getMessage());
 			$this->session->disconnect("disconnectionScreen.invalidSkin");
 
 			return true;

src/pocketmine/network/mcpe/protocol/PlayerListPacket.php

@@ -27,6 +27,7 @@ namespace pocketmine\network\mcpe\protocol;
 
 
 use pocketmine\entity\Skin;
+use pocketmine\network\BadPacketException;
 use pocketmine\network\mcpe\handler\SessionHandler;
 use pocketmine\network\mcpe\protocol\types\PlayerListEntry;
 use function count;
@@ -59,13 +60,17 @@ class PlayerListPacket extends DataPacket implements ClientboundPacket{
 				$geometryName = $this->getString();
 				$geometryData = $this->getString();
 
-				$entry->skin = new Skin(
+				try{
-					$skinId,
+					$entry->skin = new Skin(
-					$skinData,
+						$skinId,
-					$capeData,
+						$skinData,
-					$geometryName,
+						$capeData,
-					$geometryData
+						$geometryName,
-				);
+						$geometryData
+					);
+				}catch(\InvalidArgumentException $e){
+					throw new BadPacketException($e->getMessage(), 0, $e);
+				}
 				$entry->xboxUserId = $this->getString();
 				$entry->platformChatId = $this->getString();
 			}else{

src/pocketmine/network/mcpe/protocol/PlayerSkinPacket.php

@@ -26,6 +26,7 @@ namespace pocketmine\network\mcpe\protocol;
 #include <rules/DataPacket.h>
 
 use pocketmine\entity\Skin;
+use pocketmine\network\BadPacketException;
 use pocketmine\network\mcpe\handler\SessionHandler;
 use pocketmine\utils\UUID;
 
@@ -54,7 +55,11 @@ class PlayerSkinPacket extends DataPacket implements ClientboundPacket, Serverbo
 		$geometryModel = $this->getString();
 		$geometryData = $this->getString();
 
-		$this->skin = new Skin($skinId, $skinData, $capeData, $geometryModel, $geometryData);
+		try{
+			$this->skin = new Skin($skinId, $skinData, $capeData, $geometryModel, $geometryData);
+		}catch(\InvalidArgumentException $e){
+			throw new BadPacketException($e->getMessage(), 0, $e);
+		}
 
 		$this->premiumSkin = $this->getBool();
 	}