Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process SkinData asynchronously #6589

Open
wants to merge 8 commits into
base: stable
Choose a base branch
from
+129 −24
Open

Process SkinData asynchronously #6589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e45f989
process SkinData asynchronously
Blackjack200 Dec 28, 2024
832c9cd
check whether session is online
Blackjack200 Dec 28, 2024
2e69cf2
Fix: Prevent skin overwrite when multiple skin change tasks run concu…
Blackjack200 Dec 28, 2024
844cc8e
Fix: Correct disconnect message for invalid login UUID
Blackjack200 Dec 28, 2024
a8cddf2
fix codestyle...
Blackjack200 Dec 28, 2024
30ed339
onCompletion(): replace instanceof check with @var for consistency
Blackjack200 Dec 29, 2024
438d7ce
Fix missing handling for invalid base64 data case and adjust other code
Blackjack200 Dec 29, 2024
d3b54db
...
Blackjack200 Dec 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions src/network/mcpe/ProcessSkinTask.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

/*
*
* ____ _ _ __ __ _ __ __ ____
* | _ \ ___ ___| | _____| |_| \/ (_)_ __ ___ | \/ | _ \
* | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
* | __/ (_) | (__| < __/ |_| | | | | | | | __/_____| | | | __/
* |_| \___/ \___|_|\_\___|\__|_| |_|_|_| |_|\___| |_| |_|_|
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* @author PocketMine Team
* @link http://www.pocketmine.net/
*
*
*/

declare(strict_types=1);

namespace pocketmine\network\mcpe;

use pocketmine\entity\InvalidSkinException;
use pocketmine\entity\Skin;
use pocketmine\network\mcpe\convert\TypeConverter;
use pocketmine\network\mcpe\protocol\types\skin\SkinData;
use pocketmine\scheduler\AsyncTask;
use pocketmine\thread\NonThreadSafeValue;

class ProcessSkinTask extends AsyncTask{
private const TLS_KEY_ON_COMPLETION = "onCompletion";

private ?string $error = "Unknown";
/** @var NonThreadSafeValue<SkinData> */
private NonThreadSafeValue $skinData;

/**
* @param \Closure(?Skin $skin,?string $error) : void $callback
*/
public function __construct(
SkinData $skinData,
\Closure $callback,
){
$this->skinData = new NonThreadSafeValue($skinData);
$this->storeLocal(self::TLS_KEY_ON_COMPLETION, $callback);
}

public function onRun() : void{
try{
$skin = TypeConverter::getInstance()->getSkinAdapter()->fromSkinData($this->skinData->deserialize());
$this->setResult($skin);
$this->error = null;
}catch(\InvalidArgumentException|InvalidSkinException $e){
$this->error = $e->getMessage();
}
}

public function onCompletion() : void{
/** @var Skin|null $result */
$result = $this->getResult();
/** @var \Closure(?Skin $skin,?string $error) : void $callback */
$callback = $this->fetchLocal(self::TLS_KEY_ON_COMPLETION);
$callback($result, $this->error);
}
}
36 changes: 28 additions & 8 deletions src/network/mcpe/handler/InGamePacketHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
use pocketmine\block\utils\SignText;
use pocketmine\entity\animation\ConsumingItemAnimation;
use pocketmine\entity\Attribute;
use pocketmine\entity\InvalidSkinException;
use pocketmine\entity\Skin;
use pocketmine\event\player\PlayerEditBookEvent;
use pocketmine\inventory\transaction\action\DropItemAction;
use pocketmine\inventory\transaction\InventoryTransaction;
Expand All @@ -40,12 +40,14 @@
use pocketmine\item\WritableBook;
use pocketmine\item\WritableBookPage;
use pocketmine\item\WrittenBook;
use pocketmine\lang\KnownTranslationFactory;
use pocketmine\math\Facing;
use pocketmine\math\Vector3;
use pocketmine\nbt\tag\CompoundTag;
use pocketmine\nbt\tag\StringTag;
use pocketmine\network\mcpe\InventoryManager;
use pocketmine\network\mcpe\NetworkSession;
use pocketmine\network\mcpe\ProcessSkinTask;
use pocketmine\network\mcpe\protocol\ActorEventPacket;
use pocketmine\network\mcpe\protocol\ActorPickRequestPacket;
use pocketmine\network\mcpe\protocol\AnimatePacket;
Expand Down Expand Up @@ -840,15 +842,33 @@ public function handlePlayerSkin(PlayerSkinPacket $packet) : bool{
$this->session->getLogger()->debug("Refused duplicate skin change request");
return true;
}
$this->lastRequestedFullSkinId = $packet->skin->getFullSkinId();
$currentFullSkinId = $this->lastRequestedFullSkinId = $packet->skin->getFullSkinId();

$this->session->getLogger()->debug("Processing skin change request");
try{
$skin = $this->session->getTypeConverter()->getSkinAdapter()->fromSkinData($packet->skin);
}catch(InvalidSkinException $e){
throw PacketHandlingException::wrap($e, "Invalid skin in PlayerSkinPacket");
}
return $this->player->changeSkin($skin, $packet->newSkinName, $packet->oldSkinName);
$this->player->getServer()->getAsyncPool()->submitTask(new ProcessSkinTask(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably make the server to freeze / crash if a malicious client floods tons of skin change requests per tick.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the existing $this->gamePacketLimiter be sufficient for skin change packets, or would it be better to implement a separate rate limiter specifically for PlayerSkinPacket to prevent potential flooding?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a separate rate limiter is needed, how should we set the values for INCOMING_SKIN_PACKET_PER_TICK = 1 and INCOMING_SKIN_PACKET_BUFFER_TICKS = 100?
Would these settings be appropriate, or should we adjust them further?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the existing $this->gamePacketLimiter be sufficient for skin change packets, or would it be better to implement a separate rate limiter specifically for PlayerSkinPacket to prevent potential flooding?

Probably not. It's supposed to limit all packets, so it has quite generous limits.

More so the issue here is memory overload if the client spams a lot of skin change packets (it could cause a large backlog in the worker pool & hog a lot of memory).

My thought was to limit the number of concurrent ProcessSkinTasks per session, but I'm not sure if this is the best way.

Copy link
Author

@Blackjack200 Blackjack200 Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting the rate of PlayerSkinPacket packets seems easier to implement and would have a similar effect to limiting the number of concurrent ProcessSkinTask.

In my preliminary testing, setting INCOMING_SKIN_PACKET_PER_TICK = 0.1 and INCOMING_SKIN_PACKET_BUFFER_TICKS = 20 seems promising (though it still needs sufficient testing). However, this would require changing the $averagePerTick in PacketLimiter from an int to a float or both. Would that be acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. The number of packets isn't the only factor we need to consider. We also have to be aware of how long each packet takes to process.

Under normal conditions the packets might take 10 ms each to process, but an attacker could craft packets that take 10-100x longer to process and still be subject to the same packets-per-tick limits (getting more lag per packet). That's why I said we probably need to limit the number of tasks, rather than the number of packets.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. The number of packets isn't the only factor we need to consider. We also have to be aware of how long each packet takes to process.

Under normal conditions the packets might take 10 ms each to process, but an attacker could craft packets that take 10-100x longer to process and still be subject to the same packets-per-tick limits (getting more lag per packet). That's why I said we probably need to limit the number of tasks, rather than the number of packets.

Aside from the better methods for now, we could consider handling these potential time-bomb data by spawning new processes, similar to ConsoleReaderChildProcess. This would allow the main thread to control their runtime, using OS-level process killing (with proper abstraction for generalization—I’ve already implemented this in my private project, leveraging opis/closure). However, this approach does come with challenging compatibility issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds overengineered to me.

$packet->skin,
function(?Skin $skin, ?string $error) use ($currentFullSkinId, $packet) : void{
if(!$this->session->isConnected()){
Blackjack200 marked this conversation as resolved.
Show resolved Hide resolved
return;
}
if($error !== null){
$this->session->disconnectWithError(
reason: "Invalid skin: " . $error,
disconnectScreenMessage: KnownTranslationFactory::disconnectionScreen_invalidSkin()
);
return;
}
if($skin !== null){
if($currentFullSkinId !== $this->lastRequestedFullSkinId){
$this->session->getLogger()->debug("Skin change request ignored due to newer skin change");
return;
}
$this->player->changeSkin($skin, $packet->newSkinName, $packet->oldSkinName);
$this->session->getLogger()->debug("Skin change request processed");
}
}
));
return true;
}

public function handleSubClientLogin(SubClientLoginPacket $packet) : bool{
Expand Down
49 changes: 33 additions & 16 deletions src/network/mcpe/handler/LoginPacketHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,15 @@

namespace pocketmine\network\mcpe\handler;

use pocketmine\entity\InvalidSkinException;
use pocketmine\entity\Skin;
use pocketmine\event\player\PlayerPreLoginEvent;
use pocketmine\lang\KnownTranslationFactory;
use pocketmine\lang\Translatable;
use pocketmine\network\mcpe\auth\ProcessLoginTask;
use pocketmine\network\mcpe\JwtException;
use pocketmine\network\mcpe\JwtUtils;
use pocketmine\network\mcpe\NetworkSession;
use pocketmine\network\mcpe\ProcessSkinTask;
use pocketmine\network\mcpe\protocol\LoginPacket;
use pocketmine\network\mcpe\protocol\types\login\AuthenticationData;
use pocketmine\network\mcpe\protocol\types\login\ClientData;
Expand All @@ -41,6 +42,7 @@
use pocketmine\player\PlayerInfo;
use pocketmine\player\XboxLivePlayerInfo;
use pocketmine\Server;
use pocketmine\utils\AssumptionFailedError;
use Ramsey\Uuid\Uuid;
use function is_array;

Expand Down Expand Up @@ -68,22 +70,39 @@ public function handleLogin(LoginPacket $packet) : bool{
return true;
}

$clientData = $this->parseClientData($packet->clientDataJwt);
if(!Uuid::isValid($extraData->identity)){
throw new PacketHandlingException("Invalid login UUID");
}

$clientData = $this->parseClientData($packet->clientDataJwt);
try{
$skin = $this->session->getTypeConverter()->getSkinAdapter()->fromSkinData(ClientDataToSkinDataHelper::fromClientData($clientData));
}catch(\InvalidArgumentException | InvalidSkinException $e){
$this->session->disconnectWithError(
reason: "Invalid skin: " . $e->getMessage(),
disconnectScreenMessage: KnownTranslationFactory::disconnectionScreen_invalidSkin()
);

return true;
$skinData = ClientDataToSkinDataHelper::fromClientData($clientData);
}catch(\InvalidArgumentException $e){
throw PacketHandlingException::wrap($e);
}
$this->server->getAsyncPool()->submitTask(new ProcessSkinTask(
$skinData,
function(?Skin $skin, ?string $error) use ($packet, $clientData, $extraData){
Blackjack200 marked this conversation as resolved.
Show resolved Hide resolved
if(!$this->session->isConnected()){
return;
}
if($error !== null){
$this->session->disconnectWithError(
reason: "Invalid skin: " . $error,
disconnectScreenMessage: KnownTranslationFactory::disconnectionScreen_invalidSkin()
);
return;
}
if($skin === null){
throw new AssumptionFailedError("This should never happen...");
}
$this->onSkinDataProcessed($extraData, $clientData, $skin, $packet);
},
));
return true;
}

if(!Uuid::isValid($extraData->identity)){
throw new PacketHandlingException("Invalid login UUID");
}
protected function onSkinDataProcessed(AuthenticationData $extraData, ClientData $clientData, Skin $skin, LoginPacket $packet) : void{
$uuid = Uuid::fromString($extraData->identity);
$arrClientData = (array) $clientData;
$arrClientData["TitleID"] = $extraData->titleId;
Expand Down Expand Up @@ -136,12 +155,10 @@ public function handleLogin(LoginPacket $packet) : bool{
$ev->call();
if(!$ev->isAllowed()){
$this->session->disconnect($ev->getFinalDisconnectReason(), $ev->getFinalDisconnectScreenMessage());
return true;
return;
}

$this->processLogin($packet, $ev->isAuthRequired());

return true;
}

/**
Expand Down
Loading