Add support for require method in lua

[bugobliterator]

Also move the global library initialisation out of sandbox method. This way we initialise the lua libs once and then set the reference inside the sandbox, this is similar to what we do with our generated lua bindings.

Split from following PR as requested:

#13660

Current master

AP: Lua: Running ./scripts/ahrs-print-home-and-origin.lua
AP: Home - Lat:-353632608.0 Long:1491652352.0 Alt:58409.0
AP: Origin - Lat:-353632608.0 Long:1491652352.0 Alt:58409.0
AP: Lua: Time: 0 Mem: 50777 + 886
AP: Lua: Running ./scripts/ahrs-print-angle-and-rates.lua
AP: Ang R:0.1 P:0.1 Y:-5.9 Rate R:0.0 P:0.0 Y:0.1
AP: Lua: Time: 0 Mem: 50629 + 738

With package module loaded

AP: Lua: Running ./scripts/ahrs-print-home-and-origin.lua
AP: Home - Lat:-353632608.0 Long:1491652352.0 Alt:58409.0
AP: Origin - Lat:-353632608.0 Long:1491652352.0 Alt:58409.0
AP: Lua: Time: 0 Mem: 52301 + 886
AP: Lua: Running ./scripts/ahrs-print-angle-and-rates.lua
AP: Ang R:0.1 P:0.0 Y:-4.2 Rate R:0.0 P:0.0 Y:0.4
AP: Lua: Time: 0 Mem: 52153 + 738

[tridge]

@bugobliterator can you add an example script using require, with an examples/modules/ script?

[tridge]

@bugobliterator I tested with an aerobatics schedule, and min heap needed in SITL went from 270000 to 276000, which is a bit more than I expected.
I think it is well worth it for the feature, but I do worry that some production script will get a memory error where it didn't before. I wish we had a better way to warn users that they are close to running out of memory

[WickedShell]

I'm not sure if this does everything desired correctly. I'd need to check, but sharing global tables means you break the sandbox and one script that overrides something can take out the other scripts global access to something.

[bugobliterator]

I'm not sure if this does everything desired correctly. I'd need to check, but sharing global tables means you break the sandbox and one script that overrides something can take out the other scripts global access to something.

Ok, I see the vulnerability, to illustrate @WickedShell 's point here's an example:

math.abs = function (x)
    if x < 0 then
        gcs:send_text(6, "x is negative")
        return -x
    else
        gcs:send_text(6, "x is positive")
        return x
    end
end
function update()
    gcs:send_text(6, "called from test1.lua")
    gcs:send_text(6, math.abs(-1))
    return update, 1000
end

return update, 1000

-- test2.lua
function update()
    gcs:send_text(6, "called from test2.lua")
    gcs:send_text(6, math.abs(-1))
    return update, 1000
end

return update, 1000

if we are to run above script with my change, the result is going to be foloowing:

AP: called from test2.lua
AP: x is negative
AP: 1
AP: called from test1.lua
AP: x is negative
AP: 1

Which breaks the sandbox, as pointed out by @WickedShell .

whereas before it was:

AP: called from test1.lua
AP: x is negative
AP: 1
AP: called from test2.lua
AP: 1

Also to point out this vulnerability is not for generated bindings where methods are hidden behind closures, out of reach from global modification.

Another thing to know, sandbox has already been broken through some of the global manual implementations, like millis, if a script overrides millis it will change things for all running scripts. This PR introduced this #20668 , these methods were sandboxed before. @IamPete1 @tridge @WickedShell

[bugobliterator]

@WickedShell @tridge @IamPete1 I have updated the require library to follow the sandboxing rules. Now when you require a module, the underlying script inherits the _ENV from caller script.

[IamPete1]

I have had a play about, loading some pre-existing lua libraries ect. All seems to work as expected. Library's get full set of methods built in and custom.

It all seems to work, it would be nice to find another way to do get the environment and remove the lua_get_current_ref stuff. But ultimately this works and I'm not sure of any other way. We can always rework if we find such a way in the future.

[tridge]

I'd like to retest memory/cpu impact for aerobatics

[tridge]

@andypnz we need to test this with aerobatics in one of our Thursday sessions

[andypnz]

@andypnz we need to test this with aerobatics in one of our Thursday sessions

Let’s do that :-) Shall we start up again tomorrow, or Thursday next week?

[tridge]

@bugobliterator tested with aerobatics on top of this branch and it uses slightly less memory! About 1k less heap needed with this PR than without (275k vs 276k)

[DaveHowie]

Hello @bugobliterator,

Is support for the Lua require limited to Lua scripts embedded in the firmware?

Or

Is support for the Lua require expected to work with scripts upload to an SD card?

[IamPete1]

Hello @bugobliterator,

Is support for the Lua require limited to Lua scripts embedded in the firmware?

Or

Is support for the Lua require expected to work with scripts upload to an SD card?

Should work for both and combinations. ROMFS was broken for a while but should work now (#26157)

[DaveHowie]

Thank you for the response Peter.

Prior to my earlier post I had attempted with scripting running on simulation copter. However, it was failing as not finding the referenced Lua script.

After reading your response, I went back and tried it again, and realized that I needed to include the scripts path (even though I thought I had tried that earlier):

local adl = require("/scripts/ADLogger")

In summary, it is working for me.

Again, thank you for the response.