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

Clamp client-sent movement speed control #15721

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clamp client-sent movement speed control #15721

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cae9805
Clamp client-sent movement speed control
appgurueu Jan 27, 2025
f945111
Handle NaN
appgurueu Jan 27, 2025
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
8 changes: 7 additions & 1 deletion src/network/serverpackethandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include "util/srp.h"
#include "clientdynamicinfo.h"

#include <algorithm>

void Server::handleCommand_Deprecated(NetworkPacket* pkt)
{
infostream << "Server: " << toServerCommandTable[pkt->getCommand()].name
Expand Down Expand Up @@ -468,7 +470,11 @@ void Server::process_PlayerPos(RemotePlayer *player, PlayerSAO *playersao,
*pkt >> bits;

if (pkt->getRemainingBytes() >= 8) {
*pkt >> player->control.movement_speed;
f32 movement_speed;
*pkt >> movement_speed;
if (movement_speed != movement_speed) // NaN
movement_speed = 0.0f;
player->control.movement_speed = std::clamp(movement_speed, 0.0f, 1.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

huh? shouldn't the direction be clamped while speed can be any value > 0?

Copy link
Member

@grorp grorp Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the direction (radians) should wrap around, so any value should be valid.

at least the virtual joystick of the touch controls can be dragged past a speed of 1 (used by virtual_joystick_triggers_aux1), however this is already clamped on the client iirc, so adding server-side validation makes sense.

consideration: might it be a better idea to send the unclamped speed value to the server and document that? since you can always clamp yourself, but you can't undo the clamping.

Copy link
Member

Choose a reason for hiding this comment

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

What if the client sends NaN, -inf or inf?

Copy link
Contributor Author

@appgurueu appgurueu Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Infinities are fine with clamping. NaN is UB indeed, good call. I had expected something saner (I would have been fine with it being clamped to either boundary).

We should probably also forbid NaN or +-Inf directions.

*pkt >> player->control.movement_direction;
} else {
player->control.movement_speed = 0.0f;
Expand Down
Loading