-
Notifications
You must be signed in to change notification settings - Fork 53
Move Logic Rewrite
To see why this has to be rewritten, we'll first look at how vanilla handles CPacketPlayer packets line by line. Here is NetHandlerPlayServer.processPlayer:
public void processPlayer(CPacketPlayer packet) {
PacketThreadUtil.checkThreadAndEnqueue(packet, this, player.getServerWorld());First, the server checks that the move packet is valid (the doubles are finite and in the range +/-30 000 000), and disconnects the player if not.
if (isMovePlayerPacketInvalid(packet)) {
disconnect(new TextComponentTranslation("multiplayer.disconnect.invalid_player_movement"));
return;
}Next, the server gets the world from player.dimension. Firstly, this can be done much later, since it is not necessary right now, and isn't necessary if some checks fail (queuedEndExit, sleeping, riding, moved wrongly).
But the only time when server.getWorld(player.dimension) is not equal to player.getServerWorld() is when player.world is null. However, if player.world is null, then a null pointer exception would occur before world is even used. Therefore, we can set world to player.getServerWorld(), at the start of the method, and use that every time the world is needed.
WorldServer world = server.getWorld(player.dimension);Now we check if the player has an end exit queued. The player.queuedEndExit field is set to true by EntityPlayerMP.changeDimension when both the current and target dimension are 1, meaning that the player entered an end portal in the end. That causes the player to be removed from the world, but still not be transferred to the new world until the client sends a CPacketClientStatus PERFORM_RESPAWN packet (after the player has seen the credits).
if (player.queuedEndExit) return;Normally, NetHandlerPlayServer.update calls captureCurrentPosition every network tick. However, it is possible for a move packet to be received before the first tick. In that case, we capture the current position to avoid movement checks with the incorrect old position of (0, 0, 0).
if (networkTickCount == 0) {
captureCurrentPosition();
}After a client position sync (such as after a teleportation), targetPos is set to the player's new position. Until the player sends a CPacketConfirmTeleport packet, the server will ignore move packets received. If the player is still sending move packets after one 20 ticks (1 second), the server sets the position back to targetPos (discarding any moves caused by the server, such as falling, water, or explosions).
This is a problem for two reasons. Firstly, the valid moves caused by the server are being reverted, so the player will appear to jump back into the position every second, which looks weird and hacky to other players. In fact, a player may intentionally abuse this to stay immune to falling after a teleport. Secondly, if the ping is higher than 1 second, a the client will never catch up with the server. By the time they have confirmed one teleport, that one will have become invalid because the player sent another one, causing jump-backs until the player stops moving for whatever their ping is.
The best solution for this would be to simply ignore move packets until the player confirms the teleport, without changing the player's position or resending any packets. The logic behind resending the teleport packet might have been that the client may have "missed" the first packet. But this is impossible with TCP, and is never done for any other kind of packets such as respawn packets. Processing the packet and letting the "moved too quickly" check resync the player would not be an option, as those get sent even more frequently than 1 second.
if (targetPos != null) {
if (networkTickCount - lastPositionUpdate > 20) {
lastPositionUpdate = networkTickCount;
setPlayerLocation(targetPos.x, targetPos.y, targetPos.z, player.rotationYaw, player.rotationPitch);
}
return;
}
lastPositionUpdate = networkTickCount;Next, the server checks if the player is riding an entity (such as a minecart). In that case, the server accepts the yaw and pitch but refuses the new position. Note that player.setPositionAndRotation, unlike setPlayerLocation, only syncs the player without sending a teleport packet. serverUpdateMovingPlayer is used to sync the player's chunk map.
if (player.isRiding()) {
player.setPositionAndRotation(player.posX, player.posY, player.posZ, packet.getYaw(player.rotationYaw), packet.getPitch(player.rotationPitch));
server.getPlayerList().serverUpdateMovingPlayer(player);
return;
}Next, we store the x, y, and z coordinates before the player has been moved, and the packet's x, y, z, pitch and yaw, coordinates (defaulting to the current position if the packet is not a move packet).
double prevX = player.posX;
double prevY = player.posY;
double prevZ = player.posZ;
double packetX = packet.getX(player.posX);
double packetY = packet.getY(player.posY);
double packetZ = packet.getZ(player.posZ);
float packetYaw = packet.getYaw(player.rotationYaw);
float packetPitch = packet.getPitch(player.rotationPitch);The difference between the packet's position and the last tick is calculated (firstGood_ is set by calling captureCurrentPosition , which is done every tick by NetHandlerPlayServer.update, or after a block collision causes a player teleport). The move speed of the player is also calculated.
double xDiff = packetX - firstGoodX;
double yDiff = packetY - firstGoodY;
double zDiff = packetZ - firstGoodZ;
double motionSq = player.motionX * player.motionX + player.motionY * player.motionY + player.motionZ * player.motionZ;
double diffSq = xDiff * xDiff + yDiff * yDiff + zDiff * zDiff;If the player is sleeping, move packets are ignored, and if the packet has a distance of more than 1 since the last accepted position, the server resyncs the client. This check is correctly done after the riding check, allowing things such as "bed minecarts" to be implemented.
But if the client sent a move packet while sleeping, it's probably because it somehow forgot it was sleeping, so it would be better to make the server wake the player up, and process the packet as normal.
if (player.isPlayerSleeping()) {
if (diffSq > 1.0D) {
setPlayerLocation(player.posX, player.posY, player.posZ, packet.getYaw(player.rotationYaw), packet.getPitch(player.rotationPitch));
}
return;
}The number of move packets since the last tick is calculated, for use by movement speed checking. If there are too many, the counter is set to 1 to avoid cheating by spamming move packets. But this is wrong, as the max distance per tick should not even be based on the number of packets!
++movePacketCounter;
int movePacketsSinceLastTick = movePacketCounter - lastMovePacketCounter;
if (movePacketsSinceLastTick > 5) {
LOGGER.debug("{} is sending move packets too frequently ({} packets since last tick)", player.getName(), movePacketsSinceLastTick);
movePacketsSinceLastTick = 1;
}The packet is ignored and the client is resynced if the movement speed is too big. However, this logic is completely broken.
Firstly, with 5 move packets per tick (maximum accepted by Minecraft), the player can move at most 500 blocks per second, or 1500 blocks per second with an elytra. The player's maximum difference from the last packet should instead depend on the time (not tick count, the server may be lagging!) since the last packet, with a hardcoded maximum difference (such that the player doesn't seem to be jumping around though having a correct average move speed). The maximum speed per second should depend on the player's walking speed to not trigger this check when it is increased by a command or potion.
Secondly, the velocity squared of the player should not simply be added to the maximum speed, the direction of the velocity should also be considered (if the player is moving quickly backwards, a large move packet forwards is invalid!).
Note that the !player.isInvulnerableDimensionChange() check is unnecessary here, since isInvulnerableDimensionChange (for which a better name would be blockCollisionMovedPlayer) is only true between a block collision (handled later in this method through Entity.move) and the confirmation of the teleport by the client (until then, this line is never reached because of targetPos being non-null).
if (!player.isInvulnerableDimensionChange() && !(player.getServerWorld().getGameRules().getBoolean("disableElytraMovementCheck") && player.isElytraFlying())) {
float maxDiffPerPacket = player.isElytraFlying() ? 300.0F : 100.0F;
if (diffSq - motionSq > maxDiffPerPacket * movePacketsSinceLastTick && (!server.isSinglePlayer() || !server.getServerOwner().equals(player.getName()))) {
LOGGER.warn("{} moved too quickly! {},{},{}", player.getName(), xDiff, yDiff, zDiff);
setPlayerLocation(player.posX, player.posY, player.posZ, player.rotationYaw, player.rotationPitch);
return;
}
}Now, the server handles the actual movement. The difference since the last packet is calculated, the player jumps if the packet.isOnGround is false and the packet's move is upwards, and the player is moved in that direction. The move method makes sure that the player doesn't pass through blocks, climb up stairs which are too high, or sneak off edges of blocks.
The EntityPlayerMP.invulnerableDimensionChange field is set to true by blocks which cause a teleportation on collision, for which a better name being blockCollisionMovedPlayer). It would probably be better to use targetPos != null to check if the block caused a teleportation instead, to reduce redundancy and have one less thing that blocks doing teleportation have to worry about. If that field is true (or better, if targetPos is not null), the method should capture the new position and stop right after the move instead of checking if the packet is valid.
Although Entity.move checks that the entity did not pass through any blocks, it triggers block collision events only for the final position after the move, meaning that a player could cheat by passing through an end portal without being teleported. Instead, the move method should trigger the collision method of for every block in the path, and stop once a block collision causes a teleport.
boolean wasNotInBlock = world.getCollisionBoxes(player, player.getEntityBoundingBox().shrink(0.0625D)).isEmpty();
xDiff = packetX - lastGoodX;
yDiff = packetY - lastGoodY;
zDiff = packetZ - lastGoodZ;
if (player.onGround && !packet.isOnGround() && yDiff > 0.0D) {
player.jump();
}
player.move(MoverType.PLAYER, xDiff, yDiff, zDiff);For some reason, the server trusts the packet's onGround status... This is bad, especially since it is likely incorrect after a teleport caused by a block collision.
player.onGround = packet.isOnGround();The difference between the result of move and the packet's position is calculated. If it is too large, the player is considered to have moved wrongly. The y difference is accepted no matter how large it is, due to an obvious bug (yDiff > -0.5D || yDiff < 0.5D is always true). It was probably intended to accept it only if it between -0.5 and 0.5 (but why?).
xDiff = packetX - player.posX;
yDiff = packetY - player.posY;
zDiff = packetZ - player.posZ;
double yDiffA = yDiff > -0.5D || yDiff < 0.5D ? 0.0D : yDiff;
diffSq = xDiff * xDiff + yDiffA * yDiffA + zDiff * zDiff;
boolean movedWrongly = false;
if (!player.isInvulnerableDimensionChange() && diffSq > 0.0625D && !player.isPlayerSleeping() && !player.interactionManager.isCreative() && player.interactionManager.getGameType() != GameType.SPECTATOR) {
movedWrongly = true;
LOGGER.warn("{} moved wrongly!", player.getName());
}The player is first moved to the packet's position and movement stats are updated. Then, if the player was not noclipping or in a block, and the player moved into a block or moved wrongly, the position is reverted to the original position before the move. This is wrong for many reasons.
Firstly, the player's position is set to the packet position after a teleport, and set to the correct position only after the client confirms the teleport (which may never happen!). It's unclear whether this is intentional, but it causes many problems, such as MC-98153, Players could even intentionally not send the teleport confirmation packet and then disconnecting to hack by teleporting into/out of the nether without changing their position.
Secondly, if the move was invalid, the position is reverted to prevX/Y/Z, which reverts any teleportation but not world change, causing MC-123364.
Thirdly, movement stats get added even on an invalid move.
Furthermore, the !player.isPlayerSleeping() is useless as this was already checked before.
player.setPositionAndRotation(packetX, packetY, packetZ, packetYaw, packetPitch);
player.addMovementStat(player.posX - prevX, player.posY - prevY, player.posZ - prevZ);
if (!player.noClip && !player.isPlayerSleeping()) {
boolean notInBlock = world.getCollisionBoxes(player, player.getEntityBoundingBox().shrink(0.0625D)).isEmpty();
if (wasNotInBlock && (movedWrongly || !notInBlock)) {
setPlayerLocation(prevX, prevY, prevZ, packetYaw, packetPitch);
return;
}
}The server updates the floating field (to check that the player isn't "floating too long"), sets player.onGround to packet.isOnGround() (again trusting the packet!), and handles fall logic. A better way of doing this would be to simply not allow floating for any time, and correct the position if the packet is in the air but the player isn't jumping.
floating = yDiff >= -0.03125D;
floating &= !server.isFlightAllowed() && !player.capabilities.allowFlying;
floating &= !player.isPotionActive(MobEffects.LEVITATION) && !player.isElytraFlying() && !world.checkBlockCollision(player.getEntityBoundingBox().grow(0.0625D).expand(0.0D, -0.55D, 0.0D));
player.onGround = packet.isOnGround();
server.getPlayerList().serverUpdateMovingPlayer(player);
player.handleFalling(player.posY - prevY, packet.isOnGround());The new player's position is saved in lastGoodX/Y/Z.
lastGoodX = player.posX;
lastGoodY = player.posY;
lastGoodZ = player.posZ;
}A rewritten processPlayer method can be found here. It fixes all the vanilla bugs described in the previous section and implements a better anti-cheat system for servers.