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

Enable safe Lua os.* functions #316

Merged
merged 4 commits into from Sep 6, 2018
Merged

Conversation

Dezash
Copy link
Contributor

@Dezash Dezash commented Aug 8, 2018

Loads Lua Os library and disables the following functions:

  • os.execute
  • os.rename
  • os.remove
  • os.exit
  • os.getenv
  • os.tmpname

The following functions are enabled:

  • os.clock
  • os.date
  • os.difftime
  • os.setlocale
  • os.time

These functions are much faster than MTA's substitute functions.
Here's how os.clock compares with getTickCount:

local startTime = getTickCount()
for i = 1, 1000000 do
    local a = getTickCount()
end
print("getTickCount", getTickCount() - startTime)

startTime = getTickCount()
for i = 1, 1000000 do
    local a = os.clock()
end
print("os.clock", getTickCount() - startTime)

Result:
INFO: getTickCount 3926
INFO: os.clock 418

Almost 10 times faster than getTickCount. Considering that functions such as getTickCount or getRealTime are often used onClientRender, this may allow scripters to slightly increase script performance.

This would also help accommodate people who have knowledge of Lua, but are new to MTA.

qaisjp
qaisjp previously requested changes Aug 8, 2018
Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Please move to InitSecurity and set as a disabled function

@CrosRoad95
Copy link
Contributor

You should enable os.tmpname
Generate a name that can be used for a temporary file. This only generates a name, it does not open a file.

http://lua-users.org/wiki/OsLibraryTutorial
could be useful 🙄

@Dezash
Copy link
Contributor Author

Dezash commented Aug 8, 2018

@CrosRoad95 os.tmpname returns a string with the full directory. It might be a security concern.

@Pirulax
Copy link
Contributor

Pirulax commented Aug 8, 2018

os.clock returns seconds, while getTickCount() returns ms

@Dezash
Copy link
Contributor Author

Dezash commented Aug 8, 2018

@Pirulax it returns a float of 0.001 precision in seconds format, which is the same as milliseconds.

@Pirulax
Copy link
Contributor

Pirulax commented Aug 8, 2018

I dont know.
Tried to run os.clock() on repl.it, but it's wired a little bit there.
I dont have lua interpreter installed, so I cant test that.
But I remember that I've read something about os.clock() returning seconds only, and if I remembered it correctly, I've read that at math.randomseed() function on lua-users

@Dezash
Copy link
Contributor Author

Dezash commented Aug 8, 2018

@Pirulax it does return seconds, but in 0.001 precision. Read the example from Lua documentation: http://lua-users.org/wiki/OsLibraryTutorial

@patrikjuvonen patrikjuvonen added the enhancement New feature or request label Aug 9, 2018
@AlexTMjugador
Copy link
Member

AlexTMjugador commented Aug 15, 2018

I'm just going to give my two cents here to @Dezash: wouldn't it be better to push the CLuaUtilDefs::DisabledFunction method to the Lua stack, instead of a nil value, to disable these unsafe OS library functions? That method logs an error and pushes false to the Lua stack, which states more clearly MTA's intent to disable those functions to the scripter. Just replacing them with a nil value, on the other hand, is a bit less clear at first hand.

I think (but I'm not sure, I'm not an active C++ developer who works with Lua) you can use lua_pushcfunction(m_luaVM, CLuaUtilDefs::DisabledFunction) to accomplish this, similarly to other disabled functions that follow.

@qaisjp
Copy link
Contributor

qaisjp commented Aug 15, 2018

@AlexTMjugador agreed. I did mention this in my review but I don't think he noticed 🙂

@Dezash
Copy link
Contributor Author

Dezash commented Aug 16, 2018

Good idea, I missed qaisjp's note somehow.

@qaisjp qaisjp dismissed their stale review August 19, 2018 17:41

changes done

@patrikjuvonen patrikjuvonen changed the title Enable safe Os functions Enable safe Lua os.* functions Sep 4, 2018
@qaisjp qaisjp changed the base branch from master to next September 6, 2018 07:37
@qaisjp qaisjp changed the base branch from next to master September 6, 2018 07:38
@qaisjp qaisjp merged commit 94bd9c5 into multitheftauto:master Sep 6, 2018
@qaisjp qaisjp added this to the 1.5.6 milestone Sep 6, 2018
qaisjp added a commit that referenced this pull request Sep 6, 2018
This reverts commit 94bd9c5, reversing
changes made to 98e8454.
qaisjp added a commit that referenced this pull request Sep 6, 2018
@jushar
Copy link
Contributor

jushar commented Sep 6, 2018

We might want to disable os.setlocale as well as it changes the locale globally for the entire application. This could result in unwanted side effects.

@qaisjp qaisjp modified the milestones: 1.5.6, 1.5.7 Sep 6, 2018
qaisjp added a commit that referenced this pull request Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants