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

LuaC sandbox weakness #516

Open
numberZero opened this issue Jun 1, 2020 · 6 comments
Open

LuaC sandbox weakness #516

numberZero opened this issue Jun 1, 2020 · 6 comments

Comments

@numberZero
Copy link
Contributor

Take a look at this example:

x = "."
for k = 1,64 do
    x = x .. x
end

On my PC this takes 0.6 seconds before timing out. 0.6 seconds of busy doing nothing. My setup is Minetest 5.3.0-dev-51de4ae29 server with LuaJIT on Linux with Intel(R) Core(TM)2 Quad CPU Q9300 @ 2.50GHz (from /proc/cpuinfo) with plenty of RAM.

The problem is that the sandbox counts Lua instructions and not CPU or wall time. String concatenation counts as one (or some small fixed amount) instruction but takes significant time. With fine tuning, I was able to keep the server busy for 40 seconds — and the LuaC didn’t even overheat so was able to repeat... and the server was able to process things like node placement once per 40 seconds. @S-S-X that doesn’t sound like microseconds, what was your setup?

Related: #415

@S-S-X
Copy link

S-S-X commented Jun 1, 2020

My setup was just luacontroller without any connected componets and that exact code entered to form and hit execute button, result was timeout from lua controller side (default limits).

As timeout message seemed to be there immediately after hittin "execute" I then did simple test with minetest.get_us_time and it reports values varying between 40 to 160.

Ran test on Intel(R) Core(TM) i7-4710MQ CPU @ 2.50GHz (max 3.5GHz under load) laptop with 16GB RAM (which does not really matter here)

$ minetest --version
Minetest 5.2.0 (Linux)
Using Irrlicht 1.8.4
BUILD_TYPE=Release
RUN_IN_PLACE=0
USE_GETTEXT=1
USE_SOUND=1
USE_CURL=1
USE_FREETYPE=1
USE_LUAJIT=1

Also, mesecons I've used was this one: https://github.com/S-S-X/mesecons not too far behind, atow 1 commit behind and only 1 commit ahead added to quickly mitigate server crash caused by some other mod messing up metadata and luac not checking it.

@numberZero
Copy link
Contributor Author

I get OOM on that code sample, not timeout. So something must be radically different. Wondering what that could be...

@ssieb
Copy link

ssieb commented Jun 1, 2020

Are you using lua or luajit?

@numberZero
Copy link
Contributor Author

Tested on both (bundled Lua and LuaJIT 2.1.0-beta3), results are the same.

@TheEt1234
Copy link

TheEt1234 commented Apr 5, 2024

So, this can actually be "fixed"

  1. before the hook, save minetest.get_us_time() and collectgarbage('count') in variables, used for step 4

  2. Make a "imperfect memory limit" setting and a "time limit" setting

  3. Instead of running the hook every maxevents, run it every like.. i don't know... 10 instructions, and check the instruction limit that way

  4. check if time or memory exceeded the limits using the information from step 1

like this (replace https://github.com/minetest-mods/mesecons/blob/master/mesecons_luacontroller/init.lua#L605C1-L605C40 with this):

-- not tested at all but i've done something like this in one of my mods
local start_time = minetest.get_us_time()
local memory = collectgarbage("count")

local hook_time = 10 -- hardcoded
local max_micros = 10000 -- 10ms, hardcoded
local max_mem = 100 -- 100kb, so that outside things won't mess with it maybe

local events = 0
debug.sethook(
function()
   events = events+1
   if events>maxevents then timeout() end
   
   local current_mem = collectgarbage("count")-memory
   local current_time = minetest.get_us_time()-start_time
   
   if current_mem>max_mem then timeout() end
   if current-time>max_micros then timeout() end
end, hook_time
)

Sorry if i explained it badly, but this should work

@TheEt1234
Copy link

so will this be eventually fixed or?
alos now that i think about it the memory check isnt really nessesary

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

No branches or pull requests

4 participants