Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

WIP: MISC/mcl_engine_workarounds: Fix malformed client ready packet crashing server #278

Draft
cora wants to merge 1 commits from fix-malformed-clientready into master
pull from: fix-malformed-clientready
merge into: Mineclonia:master
Conversation 19 Commits 1 Files Changed 1 +23
cora commented 2022-02-15 04:10:49 +01:00
Member
Copy Link
Problem

This PR fixes #269

A malformed clientready packet can leave a connecting player indefinietly in a state where they're not fully connected but the player is returned by get_connected_players leading to all kinds of problems.

Solution

This PR sets a meta field in on_joinplayer and overwrites get_connected_players to check for that.

Testing Steps

1.Start a game in mcla master with host game
2. run the crash script on localhost:30000
3. Observe the game crash
4. Start a game on this pr
5. run the crash script on localhost:30000
6. Observe the game not crash

To do
  • Find out if any get_connected_players are run in other on_joinplayers and adapt those (if any exist).
##### Problem This PR fixes #269 A malformed clientready packet can leave a connecting player indefinietly in a state where they're not fully connected but the player is returned by get_connected_players leading to all kinds of problems. ##### Solution This PR sets a meta field in on_joinplayer and overwrites get_connected_players to check for that. ##### Testing Steps 1.Start a game in mcla master with host game 2. run the crash script on localhost:30000 3. Observe the game crash 4. Start a game on this pr 5. run the crash script on localhost:30000 6. Observe the game not crash ##### To do - [ ] Find out if any get_connected_players are run in other on_joinplayers and adapt those (if any exist).
cora added 1 commit 2022-02-15 04:10:50 +01:00
cora commented 2022-02-15 17:01:17 +01:00
Author
Member
Copy Link

Another way to do this would of course be just @erlehmann's original solution: cache all players in on_joinplayer and just return that table(nothing else needed in theory). I don't know if the references stay good throughout the entire connection though so I thought this way might be smarter.

The method of detection is that the "fake" clients do not cause on_joinplayer to be run in any case.

Another way to do this would of course be just @erlehmann's original solution: cache all players in on_joinplayer and just return that table(nothing else needed in theory). I don't know if the references stay good throughout the entire connection though so I thought this way might be smarter. The method of detection is that the "fake" clients do not cause on_joinplayer to be run in any case.
kay27 commented 2022-02-17 00:29:49 +01:00
First-time contributor
Copy Link

It seems it doesn't work.
What you fix - only players connected. But they still appear as dummy objects like this:
image
They take part in all interactions and crash the servers successfully because many places in the code aren't ready for that.

It seems it doesn't work. What you fix - only players connected. But they still appear as dummy objects like this: ![image](/attachments/08b176db-ed6e-43cf-8ae0-018096ac2940) They take part in all interactions and crash the servers successfully because many places in the code aren't ready for that.
image.png
258 KiB
cora commented 2022-02-17 03:12:20 +01:00
Author
Member
Copy Link

Well thats the least intrusive way i could think of that doesnt crash the game. If you don't want them you can easily compare original mgcp with the cool new one and kick them when theyre not in the latter ^^

Well thats the least intrusive way i could think of that doesnt crash the game. If you don't want them you can easily compare original mgcp with the cool new one and kick them when theyre not in the latter ^^
cora commented 2022-02-17 03:15:13 +01:00
Author
Member
Copy Link

They take part in all interactions and crash the servers successfully because many places in the code aren't ready for that.

Maybe you're right. I'm not sure where else the player object might come from though if not from mgcp or on_joinplayer.

But i suppose it's possible.

> They take part in all interactions and crash the servers successfully because many places in the code aren't ready for that. Maybe you're right. I'm not sure where else the player object might come from though if not from mgcp or on_joinplayer. But i suppose it's possible.
cora commented 2022-02-17 03:16:04 +01:00
Author
Member
Copy Link

Did it actually crash for you though ?

Did it actually crash for you though ?
kay27 commented 2022-02-17 03:22:33 +01:00
First-time contributor
Copy Link

I can't disconnect/cick them and they crashed the server on interactions, last stuff I tried to fix has been logged when they magneted xp orbs

I added some info to the minetest issue but still think we should better consider them in main loops, it looks simpler

I can't disconnect/cick them and they crashed the server on interactions, last stuff I tried to fix has been logged when they magneted xp orbs I added some info to the minetest issue but still think we should better consider them in main loops, it looks simpler
cora commented 2022-02-17 03:25:08 +01:00
Author
Member
Copy Link

I can't disconnect/cick them and they crashed the server on interactions, last stuff I tried to fix has been logged when they magneted xp orbs

I added some info to the minetest issue but still think we should better consider them in main loops, it looks simpler

Oh yeah i see. I'll try to figure it out.

> I can't disconnect/cick them and they crashed the server on interactions, last stuff I tried to fix has been logged when they magneted xp orbs > > I added some info to the minetest issue but still think we should better consider them in main loops, it looks simpler Oh yeah i see. I'll try to figure it out.
kay27 commented 2022-02-17 03:27:22 +01:00
First-time contributor
Copy Link

To be honest, my problem is - I can't run the crashing script and go to sleep, though it could help to locate and fix many places of code. Locally, I can but almost nothing is happening locally, so I think, wow, everything is fixed, run crash.py for real server, it works 5-10 minutes and wow - we've got something new

To be honest, my problem is - I can't run the crashing script and go to sleep, though it could help to locate and fix many places of code. Locally, I can but almost nothing is happening locally, so I think, wow, everything is fixed, run crash.py for real server, it works 5-10 minutes and wow - we've got something new
cora commented 2022-02-17 03:29:20 +01:00
Author
Member
Copy Link

No I think if they actually cause widespread weirdness we can't let them connect.

Also i'm dumb ... yeah portals would also make it crash possibly cause they search for objects in the area right?

I think it might be good to detect and rewrite the is_player method of the fake player. That should take care of the rest. They aren't a problem bc theyre objects but bc they're players right ?

No I think if they actually cause widespread weirdness we can't let them connect. Also i'm dumb ... yeah portals would also make it crash possibly cause they search for objects in the area right? I think it might be good to detect and rewrite the is_player method of the fake player. That should take care of the rest. They aren't a problem bc theyre objects but bc they're players right ?
kay27 commented 2022-02-17 03:34:57 +01:00
First-time contributor
Copy Link

The only problem, I think, we didn't initialize them because they didn't go through the callback where we used to initialize player stuff. Some oldschool practice says, deal only player names, don't deal player objects, but when you need player meta, or inventory, or texture, entities, effects - it is not enough to process just names. So yes, we have to create player object somehow (which is more likely impossible for Lua) or ignore them in all loops.

mcl_util.is_player in testing branch of mcl5 right now:


local possible_hackers = {}

function mcl_util.is_player(obj)
	if not obj then return end
	if not obj.is_player then return end
	if not obj:is_player() then return end
	local name = obj:get_player_name()
	if not name then return end
	if possible_hackers[name] then return end
	return true
end

minetest.register_on_authplayer(function(name, ip, is_success)
	if not is_success then return end
	possible_hackers[name] = true
end)

minetest.register_on_joinplayer(function(player)
	possible_hackers[player:get_player_name()] = nil
end)

I just replace if dropper:is_player() to if mcl_util and mcl_util.is_player(dropper), etc.

Why mcl_util and - because we have wrong dependencies currently which is another issue

The only problem, I think, we didn't initialize them because they didn't go through the callback where we used to initialize player stuff. Some oldschool practice says, deal only player names, don't deal player objects, but when you need player meta, or inventory, or texture, entities, effects - it is not enough to process just names. So yes, we have to create player object somehow (which is more likely impossible for Lua) or ignore them in all loops. mcl_util.is_player in testing branch of mcl5 right now: ```lua local possible_hackers = {} function mcl_util.is_player(obj) if not obj then return end if not obj.is_player then return end if not obj:is_player() then return end local name = obj:get_player_name() if not name then return end if possible_hackers[name] then return end return true end minetest.register_on_authplayer(function(name, ip, is_success) if not is_success then return end possible_hackers[name] = true end) minetest.register_on_joinplayer(function(player) possible_hackers[player:get_player_name()] = nil end) ``` I just replace `if dropper:is_player()` to `if mcl_util and mcl_util.is_player(dropper)`, etc. Why `mcl_util and` - because we have wrong dependencies currently which is another issue
cora commented 2022-02-17 03:41:48 +01:00
Author
Member
Copy Link

Well i meant make it just return false. If we detect one of these. Then we can probably even let them stay because i suppose if theyre not seen as players by mcl they won't be a problem.

I haven't figured out a great method to detect them though other than running mgcp in a globalstep and checking that.

Another method i thought about is having a timer start in prejoinplayer and if joinplayer doesnt happen within say 2 seconds we rewrite the is_player function.

Are you sure they can't be kicked though? kick takes a name as arg right ?

Well i meant make it just return false. If we detect one of these. Then we can probably even let them stay because i suppose if theyre not seen as players by mcl they won't be a problem. I haven't figured out a great method to detect them though other than running mgcp in a globalstep and checking that. Another method i thought about is having a timer start in prejoinplayer and if joinplayer doesnt happen within say 2 seconds we rewrite the is_player function. Are you sure they can't be kicked though? kick takes a name as arg right ?
kay27 commented 2022-02-17 03:45:48 +01:00
First-time contributor
Copy Link

yea, have no idea why, tried lot of times, my latest attempt to make it work is a mess but you will see what i tried to do from it

diff --git a/mods/CORE/mcl_init/init.lua b/mods/CORE/mcl_init/init.lua
index f66cbf2d9..966967053 100644
--- a/mods/CORE/mcl_init/init.lua
+++ b/mods/CORE/mcl_init/init.lua
@@ -36,28 +36,29 @@ math.randomseed(os.time())
 local player_list = {}
 local player_hash = {}
 local is_player_list_outdated = true
-minetest.register_on_joinplayer(function(player)
-	player_hash[player:get_player_name()] = player
+minetest.register_on_joinplayer(function(player_object)
+	player_hash[player_object:get_player_name()] = player_object
 	is_player_list_outdated = true
 end)
-minetest.register_on_leaveplayer(function(player)
-	player_hash[player:get_player_name()] = nil
+minetest.register_on_leaveplayer(function(player_object)
+	player_hash[player_object:get_player_name()] = nil
 	is_player_list_outdated = true
 end)
 function minetest.get_connected_players()
 	if is_player_list_outdated then
 		player_list = {}
-		for _, value in pairs(player_hash) do
-			player_list[#player_list + 1] = value
+		for _, player_object in pairs(player_hash) do
+			player_list[#player_list + 1] = player_object
 		end
 		is_player_list_outdated = false
 	end
 	return player_list
 end
-minetest.register_globalstep(function()
-	for _, pl in pairs(core.get_connected_players()) do
-		if not player_hash[pl:get_player_name()] then
-			minetest.disconnect_player(pl:get_player_name(), "It was all a dream.")
+minetest.register_on_authplayer(function(name, ip, is_success)
+	minetest.after(0.01, function(name)
+		if not player_hash[name] then
+			minetest.chat_send_all("Disconnecting " .. tostring(ip) .. " for server crashing attempt")
+			minetest.kick_player(name, "It was all a dream.")
 		end
-	end
+	end)
 end)
yea, have no idea why, tried lot of times, my latest attempt to make it work is a mess but you will see what i tried to do from it ```diff diff --git a/mods/CORE/mcl_init/init.lua b/mods/CORE/mcl_init/init.lua index f66cbf2d9..966967053 100644 --- a/mods/CORE/mcl_init/init.lua +++ b/mods/CORE/mcl_init/init.lua @@ -36,28 +36,29 @@ math.randomseed(os.time()) local player_list = {} local player_hash = {} local is_player_list_outdated = true -minetest.register_on_joinplayer(function(player) - player_hash[player:get_player_name()] = player +minetest.register_on_joinplayer(function(player_object) + player_hash[player_object:get_player_name()] = player_object is_player_list_outdated = true end) -minetest.register_on_leaveplayer(function(player) - player_hash[player:get_player_name()] = nil +minetest.register_on_leaveplayer(function(player_object) + player_hash[player_object:get_player_name()] = nil is_player_list_outdated = true end) function minetest.get_connected_players() if is_player_list_outdated then player_list = {} - for _, value in pairs(player_hash) do - player_list[#player_list + 1] = value + for _, player_object in pairs(player_hash) do + player_list[#player_list + 1] = player_object end is_player_list_outdated = false end return player_list end -minetest.register_globalstep(function() - for _, pl in pairs(core.get_connected_players()) do - if not player_hash[pl:get_player_name()] then - minetest.disconnect_player(pl:get_player_name(), "It was all a dream.") +minetest.register_on_authplayer(function(name, ip, is_success) + minetest.after(0.01, function(name) + if not player_hash[name] then + minetest.chat_send_all("Disconnecting " .. tostring(ip) .. " for server crashing attempt") + minetest.kick_player(name, "It was all a dream.") end - end + end) end) ```
cora commented 2022-02-17 03:54:04 +01:00
Author
Member
Copy Link

mmmh i wonder .. can we get the object through get_player_by_name ?

mmmh i wonder .. can we get the object through get_player_by_name ?
cora commented 2022-02-17 04:11:55 +01:00
Author
Member
Copy Link

mmmh this seems to work for me (p similar to yours ig)

local sus={}
minetest.register_on_prejoinplayer(function(name) 
    sus[name]=true
    minetest.after(1,function()
	if not sus[name] then return end
        minetest.chat_send_all(name.." is sus")
	    minetest.kick_player(name)
    end)
end)
minetest.register_on_joinplayer(function(p)
    local name=p:get_player_name()
    sus[name]=nil
end)
mmmh this seems to work for me (p similar to yours ig) ```lua local sus={} minetest.register_on_prejoinplayer(function(name) sus[name]=true minetest.after(1,function() if not sus[name] then return end minetest.chat_send_all(name.." is sus") minetest.kick_player(name) end) end) minetest.register_on_joinplayer(function(p) local name=p:get_player_name() sus[name]=nil end) ```
cora commented 2022-02-17 04:17:56 +01:00
Author
Member
Copy Link

The problem is that media dl might well take longer i suppose

The problem is that media dl might well take longer i suppose
kay27 commented 2022-02-17 04:18:04 +01:00
First-time contributor
Copy Link

Well, on auth they are NIL:
image

Well, on auth they are NIL: ![image](/attachments/21df2e77-0840-438c-bcce-ea786df8b06f)
image.png
653 KiB
kay27 commented 2022-02-17 04:21:40 +01:00
First-time contributor
Copy Link

after 2 seconds of being idle - still nil

after 2 seconds of being idle - still `nil`
kay27 commented 2022-02-17 04:26:04 +01:00
First-time contributor
Copy Link

This doesn't work too, predictably

minetest.register_on_authplayer(function(name, ip, is_success)
	minetest.after(2, function()
		local connected_players = minetest.get_connected_players()
		for _, player in pairs(connected_players) do
			local player_name = player:get_player_name()
			if name == player_name then
				minetest.chat_send_all("Found an object: "..tostring(player))
			end
		end
	end)
end)
This doesn't work too, predictably ```lua minetest.register_on_authplayer(function(name, ip, is_success) minetest.after(2, function() local connected_players = minetest.get_connected_players() for _, player in pairs(connected_players) do local player_name = player:get_player_name() if name == player_name then minetest.chat_send_all("Found an object: "..tostring(player)) end end end) end) ```
cora commented 2022-02-17 11:17:57 +01:00
Author
Member
Copy Link

the thing that seems to work is that they do not trigger on_joinplayer.

the thing that seems to work is that they do not trigger on_joinplayer.
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mineclonia/Mineclonia#278
No description provided.
Delete Branch "fix-malformed-clientready"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?