Add compileString, load, and loadfile

[x4fx77x4f]

• Added compileString.
  • Has an environment parameter instead of a "HandleError" parameter as that is much more useful, and for compatibility with TSCM's Starfall (not that it matters given how much TSCM's Starfall differs from StarfallEx).
• Added load. Behaves like Lua 5.2/LuaJIT's load.
  • Doesn't support using a function for first parameter, but it's not like anyone uses that.
  • If identifier unspecified, defaults to =(load) instead of tostring-ing the environment.
  • Does not require but will handle a mode parameter, but will just ignore it.
• loadstring aliased to compileString for compatibility with older versions of Starfall.
  • Aliasing it to load like Lua 5.2 and LuaJIT do wouldn't break any of my chips, but I'm unsure how many other users will have had the same foresight.
• Added loadfile. Behaves like Lua 5.1's loadfile. A better alternative to something like loadstring(getScripts()[path], path).

Security considerations

• setfenv, unlike debug.setfenv, will throw if the first argument is anything but a function or a number, or if the second argument is anything but a table.
  • The code path that calls setfenv is only reachable if the value that will be the first argument is a function.
  • I cannot think of any way it could fail and still return the function.
  • A chip cannot set the environment of the function to any table they don't already have.
• I did not forget to whitelist the environment, but even if I had forgotten, that wouldn't be a security issue.
• I tostring the return value if it is not a function, so it will not cause a security issue even in the very unlikely event an unsafe non-string type is returned.
• A chip can only create new functions this way, not already existing functions, so there is no way it could be used to set the environment of external functions.
• The new code is not significantly more complex than the old code.
• A test was written and the code passes.

Test scripts

if player() ~= owner() then
    return
end
local code_bad = '$$$'
local retval = compileString(code_bad)
assert(retval == 'SF:'..tostring(_G)..':1: unexpected symbol near \'$\'')
local code = 'print("Hello, world!")'
retval = compileString(code)
assert(isfunction(retval))
assert(debug.getinfo(retval, 'S').source == '@SF:'..tostring(_G))
assert(getfenv(retval) == _G)
retval = compileString(code, '1')
assert(isfunction(retval))
assert(debug.getinfo(retval, 'S').source == '@SF:1')
assert(getfenv(retval) == _G)
local env = {}
retval = compileString(code, '2', env)
assert(isfunction(retval))
assert(debug.getinfo(retval, 'S').source == '@SF:2')
assert(getfenv(retval) == env)
assert(not pcall(compileString, nil))
assert(not pcall(compileString, code, false))
assert(not pcall(compileString, code, {}))
assert(not pcall(compileString, code, nil, false))
assert(not pcall(compileString, code, nil, ''))
assert(loadstring == compileString)
local func, err = load(code_bad)
assert(func == nil)
assert(err == 'SF:=(load):1: unexpected symbol near \'$\'')
func, err = load(code)
assert(isfunction(func))
assert(err == nil)
assert(debug.getinfo(func, 'S').source == '@SF:=(load)')
assert(getfenv(func) == _G)
func, err = load(code, '1')
assert(isfunction(func))
assert(err == nil)
assert(debug.getinfo(func, 'S').source == '@SF:1')
assert(getfenv(func) == _G)
func, err = load(code, '2', env)
assert(isfunction(func))
assert(err == nil)
assert(debug.getinfo(func, 'S').source == '@SF:2')
assert(getfenv(func) == env)
func, err = load(code, '3', 't')
assert(isfunction(func))
assert(err == nil)
assert(debug.getinfo(func, 'S').source == '@SF:3')
assert(getfenv(func) == _G)
func, err = load(code, '4', 't', env)
assert(isfunction(func))
assert(err == nil)
assert(debug.getinfo(func, 'S').source == '@SF:4')
assert(getfenv(func) == env)
assert(not pcall(load, nil))
assert(not pcall(load, code, false))
assert(not pcall(load, code, {}))
assert(not pcall(load, code, '5', false))
assert(not pcall(load, code, '6', 't', false))
assert(not pcall(load, code, '7', 't', ''))
print("ok")

--@include sfex_a882fea_included.lua
local path = 'sfex_a882fea_included.lua'
assert(isfunction(loadfile(path)))
assert(loadfile('$$$') == nil)
local t, j = {nil, 2, nil, 4, nil}, 5
local T = {loadfile(path)(unpack(t, 1, j))}
for i=1, j do
    assert(t[i] == T[i])
end
print("ok")

sfex_a882fea_included.lua:

return ...

[thegrb93]

What does adding these provide that you can't already do?

[x4fx77x4f]

Nothing, but it does make some things easier. loadstring currently behaves incorrectly, which makes me uncomfortable using it without also adding support for the possibility of a breaking change, or using the code in a non-Starfall environment. What could be just

local func, err = load(code, name)
if func == nil then
	error(err)
end
func()

becomes

local func, err = loadstring(code, name)
if func ~= nil and type(func) ~= 'function' then
	func, err = nil, func
end
if func == nil then
	error(err)
end
func()

. loadfile is a convenience feature for passing values between files in the same project. Compare this: init.lua:

--@include ./ext.lua
local this = {}
dofile('./ext.lua')(this)
print(this.foo)

ext.lua:

return function(this)
	this.foo = "bar"
end

With this: init.lua:

--@include ./ext.lua
local this = {}
loadfile('./ext.lua')(this)
print(this.foo)

ext.lua:

local this = ...
this.foo = "bar"

[anormaltwig]

local func = loadstring(code)
assert(isfunction(func), func)

func()

I don't see why you'd need another function for cleanliness reasons.

[x4fx77x4f]

I don't see why you'd need another function for cleanliness reasons.

Having to just know that loadstring doesn't do what it does in vanilla Lua is a footgun. Breaking the assert(loadstring(...)) idiom is a footgun. Having load makes it easier to write portable code. Having loadfile is a convenience that can mean one doesn't have to know what directory the code is in or dig through the return value of getScripts.

[thegrb93]

I wouldn't mind fixing the error behavior of those rather than adding new functions.

[x4fx77x4f]

I could alias loadstring to load. I just didn't like the idea of breaking compatibility. I could also remove compileString if you think it's redundant.

[thegrb93]

It's a bug if the error behavior isn't matching standard lua so I don't think compatibility is an issue

[anormaltwig]

Shouldn't load and loadstring behave like Lua 5.1's version and not 5.2 since that's the version GMod uses?

[x4fx77x4f]

GMod uses LuaJIT, which backports a lot of things from Lua 5.2 when they don't break compatibility. I linked to a list of deviations from Lua 5.1 in LuaJIT earlier.

[anormaltwig]

Alright, makes sense, but there is a completely unused mode argument in load.

[x4fx77x4f]

That is intentional and mentioned in the OP. It is to make sure that code expecting load or loadstring to have a third parameter will work as is, since like I mentioned before, it is a footgun to deviate from the original function's behavior but keep the same name. It has just occurred to me the same argument could be made for loadfile, since I omitted the mode and environment parameters, and it returns the same, precompiled function instead of a new one each time. I could rename it to getFunction or something, and perhaps add getFunctions and getScript for completeness. Also, I have yet to test these changes.

[thegrb93]

I think these changes are more confusing. Especially the 'require' documentation changes. Why is require different? I also disagree with renaming loadstring to load.

[x4fx77x4f]

require does not take a path in vanilla Lua, and trying to use a path anyway does not work in vanilla Lua.

[thegrb93]

Then change to 'Works pretty much like standard Lua require() except needs the file extension'

[x4fx77x4f]

Vanilla require also uses periods instead of slashes as directory separators, and does a variety of things with package. IMO, StarfallEx's require is not at all like vanilla require.

[thegrb93]

I think I've used slashes in vanilla lua without issue. Maybe you're mistaking it with java or something?

[x4fx77x4f]

Just because you can doesn't mean you're supposed to. I could change the documentation to something like "The path must be an actual path, including the file extension and using slashes for directory separators instead of periods.", but I'm not going to say it's like vanilla require when it isn't. Also, should I change the default identifier for loadstring to be back to tostring-ing the instance environment instead of using =(load)? This change would make no difference for security since chips can still use whatever identifier they want instead of the default. I have updated the test scripts in the OP and verified that the code works.

[thegrb93]

All right that looks fine

[x4fx77x4f]

I think this is fine to merge as is. I did, however, notice getScripts does not check if the instance owner is superuser, so superusers can be denied access to read the source of other chips. This was the case before I rewrote the check as well. I am unsure if this was intentional.

[thegrb93]

It looks like the mode parameter should be removed? I'm not sure what it's doing

[x4fx77x4f]

I have addressed this before, so I will just repeat what I said.

That is intentional and mentioned in the OP. It is to make sure that code expecting load or loadstring to have a third parameter will work as is, since like I mentioned before, it is a footgun to deviate from the original function's behavior but keep the same name.

It is not documented because it does not do anything because loading of bytecode is not and never will be allowed. These three lines will ensure both code using the function as documented and code expecting a mode parameter to exist will both work:

if not isstring(mode) then
	mode, env = nil, mode
end

[thegrb93]

Ok, thanks. I think it looks good then