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

Disable JIT optimization for user code. #256

Closed
wants to merge 1 commit into from
Closed

Disable JIT optimization for user code. #256

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 14, 2016

That enables normal working of debug.sethook() even for loops. The drawback is that that code will run more slowly.

That enables normal working of debug.sethook() even for loops. The drawback is that that code will run more slowly.
@ghost
Copy link
Author

ghost commented Mar 14, 2016

Please check if this works. I think it may be the safest way to go. Otherwise I have prepared a patch along the lines discussed in #254 that inserts function calls in potential loops.

@Jeija
Copy link
Collaborator

Jeija commented Mar 14, 2016

Seems to work for me, I have tested it with a small set of Luacontrollers that are programmed with all kinds of loops / gotos / function calls that could cause DoS. This certainly needs a lot more testing, but if it actually works in the end, the drawback of having code in the luacontroller run minimally slower is by far outweighed by the benefit of

  • Having no more prohibited commands in the luacontroller
  • Finally no feature disparity between Lua / LuaJIT
  • Less code to maintain, which hopefully also means better security

Thank you so much for this patch! It is one of those easy solutions I didn't know existed, so I didn't even think of this!

@ghost
Copy link
Author

ghost commented Mar 15, 2016

Yeah, it's been frustrating for me too to come up with it after writing the patch :)

@Jeija
Copy link
Collaborator

Jeija commented Apr 2, 2016

Okay, so you and I (and anyone that came across this PR) haven't found any bugs so far. This code won't test itself, so I will have to merge it and see what happens. Worst case scenario is that some server using mesecons crashes and we have to revert this change... But I don't see that happening.

The only things I have changed from your original commit are some comments. I have also prepared a commit for the mesecons.net luacontroller documentation that I'll upload shortly.

@Jeija Jeija closed this Apr 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant