Clamp client-sent movement speed control #15721
Conversation
Results in the `movement_x` and `movement_y` fields of `player:get_player_control()` being safe to use (otherwise users would need to compute the length as `(x^2 + y^2)^0.5` and clamp that to 1 themselves).
appgurueu
added
Trivial
| *pkt >> player->control.movement_speed; | ||
| f32 movement_speed; | ||
| *pkt >> movement_speed; | ||
| player->control.movement_speed = std::clamp(movement_speed, 0.0f, 1.0f); |
There was a problem hiding this comment.
huh? shouldn't the direction be clamped while speed can be any value > 0?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What if the client sends NaN, -inf or inf?
There was a problem hiding this comment.
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.
Results in the
movement_xandmovement_yfields ofplayer:get_player_control()being safe to use (otherwise users would need to compute the length as(x^2 + y^2)^0.5and clamp that to 1 themselves).Prompted by #14348 (comment).