minetest-mods / mesecons

Mod for Minetest that adds digital circuitry [=Minecraft redstone]
https://mesecons.net
Other
211 stars 119 forks source link

LuaC sandbox weakness #516

Open numberZero opened 4 years ago

numberZero commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

ssieb commented 4 years ago

Are you using lua or luajit?

numberZero commented 4 years ago

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

TheEt1234 commented 7 months ago

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 commented 6 months ago

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