sbrl / Minetest-WorldEditAdditions

Extra tools and commands to extend WorldEdit for Minetest
https://worldeditadditions.mooncarrot.space/
Mozilla Public License 2.0
16 stars 3 forks source link

Unprotected call in `wea_core` causes crash #115

Closed VorTechnix closed 1 month ago

VorTechnix commented 1 month ago

https://github.com/sbrl/Minetest-WorldEditAdditions/blob/269a73be48ddec8fc9946cbef847e6ccef138a9e/worldeditadditions_core/core/run_command.lua#L115

If the options.parse function of a chat command blows up when given a certain input the game crashes. This could take down a server for no reason, with no warning and no chance to save current state. Everything in wea_core should fail safe so

local parse_result = { options.parse(paramtext) } 

should be changed to

local parse_result = { pcall(options.parse, paramtext) } 

TODO: Look for other instances in WEA core where calls should be protected.

sbrl commented 1 month ago

Interesting, I wasn't aware pcall was allowed in the Minetest sandbox. Yeah, we should probably do this, though we'd also need to pay special attention to error handling to make sure we don't obscure errors.

Traditionally I've tried to avoid all crashes in the command code themselves, but I do agree a level of protection here -- if implemented correctly -- would significantly improve WEA's stability.

I'll take a look at this, but the next few days are extremely busy so it may have to wait until next week.

VorTechnix commented 1 month ago

Interesting, I wasn't aware pcall was allowed in the Minetest sandbox.

I've used it in a few places and minetest seems fine with it. I temporarily patched the crash on my branch by locally implementing the change I suggested. So pcall seems to work in minetest today. 🤷‍♂️

sbrl commented 1 month ago

Nice. What happens if there is a crash though in the parsing function? What is the effect of that?

VorTechnix commented 1 month ago

image

A nice trace-back message in the player chat

sbrl commented 1 month ago

hmmmmmm error message is quite opaque tho probably worth implementing some extra logic there

okay, I can take care of that - it shouldn't be complicated

Thanks for looking into it!

VorTechnix commented 1 month ago

Make sure to protect the main chat command function calls as well... just in case.

sbrl commented 1 month ago

Fixed in #123