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

Jump to bottom

Luacontroller recursive table interrupt iid crash #473

Closed
kisah opened this issue Aug 15, 2019 · 8 comments · Fixed by #534
Closed

Luacontroller recursive table interrupt iid crash #473

kisah opened this issue Aug 15, 2019 · 8 comments · Fixed by #534
Labels
bug
Milestone
Mesecons 1.3

Comments

@kisah
Copy link

kisah commented Aug 15, 2019

Server crash POC:

hax = {}
hax.a = hax
interrupt(1, hax)

After browsing the source I've found that the iid is passed to add_action without serialization (init.lua)

local msg_ser = minetest.serialize(iid)
if #msg_ser <= mesecon.setting("luacontroller_interruptid_maxlen", 256) then
	mesecon.queue:add_action(pos, "lc_interrupt", {luac_id, iid}, time, iid, 1)
else
@sfan5 sfan5 added the bug label Aug 15, 2019
@numberZero
Copy link
Contributor

the iid is passed to add_action without serialization

That shouldn’t crash the server. Stack trace would be nice.

@numberZero
Copy link
Contributor 

2019-08-21 23:01:30: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod '' in callback node_on_receive_fields(): ...etest/development/bin/../mods/mesecons/mesecons/util.lua:190: stack overflow
2019-08-21 23:01:30: ERROR[Main]: stack traceback:
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:190: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...etest/development/bin/../mods/mesecons/mesecons/util.lua:195: in function 'tablecopy'
2019-08-21 23:01:30: ERROR[Main]: 	...evelopment/bin/../mods/mesecons/mesecons/actionqueue.lua:17: in function 'add_action'
2019-08-21 23:01:30: ERROR[Main]: 	...ent/bin/../mods/mesecons/mesecons_luacontroller/init.lua:285: in function 'v'
2019-08-21 23:01:30: ERROR[Main]: 	...ent/bin/../mods/mesecons/mesecons_luacontroller/init.lua:608: in function 'run_inner'
2019-08-21 23:01:30: ERROR[Main]: 	...ent/bin/../mods/mesecons/mesecons_luacontroller/init.lua:638: in function 'set_program'
2019-08-21 23:01:30: ERROR[Main]: 	...ent/bin/../mods/mesecons/mesecons_luacontroller/init.lua:722: in function <...ent/bin/../mods/mesecons/mesecons_luacontroller/init.lua:713>

@numberZero
Copy link
Contributor

So tablecopy doesn’t support recursive tables. Oh well, if it is copied anyway, there should be no reason to store unserialized IID.

@HybridDog
Copy link
Contributor

Maybe you could simply remove tablecopy and use minetest's table.copy instead.
https://github.com/minetest/minetest/blob/47da640d7763ee1e00badb7476ac5afc4f864367/builtin/common/misc_helpers.lua#L559

@numberZero numberZero mentioned this issue Sep 21, 2019
Merged
@sfan5
Copy link
Member

sfan5 commented Sep 23, 2019

Still crashes with the fix merged, just in a different way:

2019-09-23 19:47:39: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod 'mesecons_luacontroller' in callback node_on_receive_fields(): minetest/bin/../mods/mesecons-mp/mesecons/util.lua:197: stack overflow
2019-09-23 19:47:39: ERROR[Main]: stack traceback:
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:197: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	...
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	minetest/bin/../mods/mesecons-mp/mesecons/util.lua:201: in function 'cmpAny'
2019-09-23 19:47:39: ERROR[Main]: 	bin/../mods/mesecons-mp/mesecons/actionqueue.lua:27: in function 'add_action'
2019-09-23 19:47:39: ERROR[Main]: 	.../bin/../mods/mesecons-mp/mesecons_luacontroller/init.lua:298: in function 'v'
2019-09-23 19:47:39: ERROR[Main]: 	.../bin/../mods/mesecons-mp/mesecons_luacontroller/init.lua:621: in function 'run_inner'
2019-09-23 19:47:39: ERROR[Main]: 	.../bin/../mods/mesecons-mp/mesecons_luacontroller/init.lua:653: in function 'set_program'
2019-09-23 19:47:39: ERROR[Main]: 	.../bin/../mods/mesecons-mp/mesecons_luacontroller/init.lua:745: in function <.../bin/../mods/mesecons-mp/mesecons_luacontroller/init.lua:736>

@HybridDog
Copy link
Contributor

I think that the cmpAny function works incorrectly because it iterates only over the first table:
mesecon.cmpAny({1}, {1, 2}) returns true
mesecon.cmpAny({1, 2}, {1}) returns false

Testing if the same table keys are available could help:

function mesecon.cmpAny(t1, t2)
	if type(t1) ~= type(t2) then return false end
	if type(t1) ~= "table" and type(t2) ~= "table" then return t1 == t2 end

	for k in pairs(t2) do
		if t1[k] == nil then
			return false
		end
	end
	for i, e in pairs(t1) do
		if not mesecon.cmpAny(e, t2[i]) then return false end
	end

	return true
end

This function would still ignore metatable information and keys which are tables.

To fix the crash, maybe the seen parameter can be added (untested code):

function mesecon.cmpAny(t1, t2, seen, seen2)
	if type(t1) ~= type(t2) then
		return false
	end
	if type(t1) ~= "table" then
		return t1 == t2
	end

	-- Test if t2's keys are a subset of t1's keys
	for k in pairs(t2) do
		if t1[k] == nil then
			return false
		end
	end

	-- Test if all values in t1 are the same as those in t2
	seen = seen or {}
	seen2 = seen2 or {}
	for i, v in pairs(t1) do
		-- Do not crash with recursive tables
		if not seen[v] then
			if seen2[t2[i]] then
				-- t2[i] leads to a cycle but t[i] does not (yet)
				return false
			end
			seen[v] = true
			seen2[t2[i]] = true
			if not mesecon.cmpAny(v, t2[i], seen, seen2) then
				return false
			end
		elseif not seen2[t2[i]] then
			-- t[i] leads to a cycle but t2[i] does not
			return false
		end
	end

	return true
end

@numberZero
Copy link
Contributor

I just understood why serialization is not used. As iteration order is undetermined, serialization result is undetermined as well: e.g. {x=1, y=2} might be serialized like "{x=1, y=2}" or "{y=2, x=1}", depending on phase of Moon Lua version, table history and whatever else.

I think we should just forbid recursive tables here. Comparing them for equality is non-trivial; e.g. are {x, x} and {x, y} equal if x = {x, y} and y = {x, x} (or vice versa)?

@numberZero
Copy link
Contributor

In theory, serialization to some normal form should be possible. In non-recursive case it’s rather straightforward, and regarding performance, will be slower by itself but may (not guaranteed, though) speed up comparisons significantly. For the recursive case, the speed difference will be greater, but the normal form itself is to be figured out yet.

@numberZero numberZero added this to the Mesecons 1.3 milestone Jun 27, 2020
@numberZero numberZero mentioned this issue Sep 12, 2020
Merged
numberZero added a commit that referenced this issue Sep 21, 2020
@numberZero
Restrict Lua controller interrupt IDs (#534)
6921909 
* Deprecate non-string IIDs
* Restrict tabular IIDs to proper trees
Fixes crash on recursive interrupt ID (#473)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
Mesecons 1.3
Development

Successfully merging a pull request may close this issue.

Fix crash on recursive interrupt ID
4 participants
@sfan5 @HybridDog @kisah @numberZero