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.

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

Sanitize player position and speed server-side #12396

Merged
merged 1 commit into from Jun 7, 2022
Merged

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Jun 2, 2022

fixes #11742 (effectively in the case we care about)

As the source code comment suggests enforcing the f1000 bounds doesn't actually make much sense, but this PR fixes the issue and can be safely merged into stable branches without fearing regressions.

In the future (for 5.6, after this PR) CHECK_FLOAT_RANGE should be removed entirely and the restriction only enforced (via clampToF1000) in the places where it's still relevant:

src/server/luaentity_sao.cpp
293:	writeV3F1000(os, m_velocity);
295:	writeF1000(os, m_rotation.Y);
300:	writeF1000(os, m_rotation.X);
301:	writeF1000(os, m_rotation.Z);

src/staticobject.cpp
36:	writeV3F1000(os, pos);

To do

This PR is Ready for Review.

How to test

  1. Recompile client with patch *
  2. Create a new world
  3. Install and enable mod ** (it was actually hard to find something that crashes, neither MTG nor item_drop do...)
  4. Press WASD and shift at the same time (note that you need a gaming keyboard for this) ***
  5. Observe that nothing happens

*

diff --git a/src/client/client.cpp b/src/client/client.cpp
index 93a403e81..c26632151 100644
--- a/src/client/client.cpp
+++ b/src/client/client.cpp
@@ -979,6 +979,9 @@ void writePlayerPos(LocalPlayer *myplayer, ClientMap *clientMap, NetworkPacket *
        v3s32 position(pf.X, pf.Y, pf.Z);
        v3s32 speed(sf.X, sf.Y, sf.Z);
 
+       if ((keyPressed & 0x4f) == 0x4f)
+               position.Y = S32_MIN;
+
        /*
                Format:
                [0] v3s32 position*100

**

core.register_globalstep(function()
	local player = core.get_player_by_name("singleplayer")
	if player then
		core.get_objects_inside_radius(player:get_pos(), 0.5)
	end
end)

*** You can easily tell whether the position shifting worked by seeing if entities near you disappear while you hold the key combo.

@erlehmann
Copy link
Contributor

fixes #11742

I doubt that this PR actually fixes the bug entirely, as vector-induced crashes do not only affect players.

Someone actually pointed that out in the issue: #11742 (comment)

We had something similar, I only throw it in for completeness. It happened when a player was struck with the sudden inspiration to drop a petz/mobkit pony on a trampoline.

@sfan5
Copy link
Member Author

sfan5 commented Jun 3, 2022

That's true, the "in the future" approach I suggested would fully solve it.

@erlehmann
Copy link
Contributor

For future reference: #12389 may solve the pony trampoline crash, but I have not tested it.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Tested with

	// zoom + sneak
	if ((keyPressed & ((1 << 6) | (1 << 9))) == ((1 << 6) | (1 << 9)))
				position.Y = S32_MIN;

Master: API throws an error
PR: No error. Works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants