Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergos,

Thanks for your review!

On 06.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Couple of nits, LGTM.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> regards,
> Sergos
> 
> 
> > On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > This patch resolves the issue with running the tests with auxiliary
> > dynamically loaded modules in case of out-of-source build.
> > 
> I wonder how it works for an in-source one… (just a comment)

There is no magic: the paths are adjusted within <utils.selfrun> to make
in-source testing (e.g. while developing) easier.

> 
> > The first issue relates to the way modules loaded at runtime are built
> 					    ^ to be

Thanks, fixed.

> > on MacOS. Since the auxiliary libraries are built as a dynamically
> > loaded modules on MacOS instead of shared libraries as it is done on
> > Linux and BSD, another environment variable should be used to guide
> > <ffi.load> while searching the extension. Hence the values collected in
> 
> This might overlap: ffi should use ‘LUA_CPATH’, while it is the dynamic 
> loader and dlopen() interface of it that relies on {DY}LD_ set of variables. 

No, it shouldn't. Despite the fact both native shared libraries and ones
implemented via Lua C API are loaded dynamically, they have different
loading principles. Hence, since FFI use the native libraries, it ought
to consider {DY}LD_ set of variables. LUA_CPATH on its turn provides the
paths to the Lua modules loaded via <require> and has another format and
ergo requirements for the libraries (e.g. an obligatory external
luaopen_<module> function).

> 
> > scope of <BuildTestCLib> macro need to be set to DYLD_LIBRARY_PATH
> > variable instead of LD_LIBRARY_PATH on MacOS.
> > 
> > Unfortunately, this rather small change doesn't resolve the problem at
> > all and the root cause lies much deeper than it seems at the beginning.
> > 
> > Apple tries their best to "protect their users from malicious software".
> > As a result SIP[1] has been designed and released. Now, Apple developers
> > are *so protected*, that they can load nothing being not installed in
> > the system, since some programs sanitize the environment before they
> > start child processes. Specifically, environment variables starting with
> > DYLD_ and LD_ are unset for child process started by system programs[2].
> > 
> > That which does not kill us makes us stronger: fortunately, these
> > environment variables are used by FFI machinery to find the proper
> > shared library, hence we can still tweak testing environment before
> > calling <ffi.load>. However, the value can't be passed via the standard
> > environment variable, so we prepend TEST_ prefix to its name to get
> > around SIP magic tricks. Finally, to set the variable required by FFI
> > machinery the introduced <utils.tweakenv> routine is used. PROFIT!
> > Your move, Cupertino geniuses.
> > 
> > [1]: https://support.apple.com/en-us/HT204899
> > [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
> > 
> > Resolves tarantool/tarantool#5959
> > Follows up tarantool/tarantool#4862
> > 
> > Co-authored-by: Sergey Ostanevich <sergos@tarantool.org>
> > Co-authored-by: Mons Anderson <mons@cpan.org>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > test/tarantool-tests/CMakeLists.txt           | 39 +++++++++++++++++--
> > .../gh-4427-ffi-sandwich.test.lua             |  4 ++
> > .../lj-flush-on-trace.test.lua                |  4 ++
> > test/tarantool-tests/utils.lua                | 39 ++++++++++++++++---
> > 4 files changed, 77 insertions(+), 9 deletions(-)
> > 

<snipped>

> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index d2dd71b0..9bdb71ec 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
> > @@ -1,7 +1,12 @@
> > local M = {}
> > 
> > +local ffi = require('ffi')
> > local tap = require('tap')
> > 
> > +ffi.cdef([[
> > +  int setenv(const char *name, const char *value, int overwrite);
> > +]])
> > +
> 
> Why? os.setenv() didn’t work for some reason? Comment it and I’m fine.

Oh, yeah! Sorry, but I am so glad you have faced this by yourself and
produce this precedent! I finally have a good argument for pushing
#4577[1]. BTW, now I believe we need the similar ticket for Lua
interfaces too.

Back to the business: so, you are asking why I didn't use <os.setenv> in
LuaJIT tests? The answer is quite simple: because there is no such
function in Lua! But those guys, who decided to spoil the standard Lua
namespace, believed it is a good idea to store their new functions
alongside with the ones provided by Lua standard library. This is why I
am against using the Lua namespaces for platform-defined functions and
we have introduced <misc> for the new extensions.

So, unfortunately, this can't be done in other way without more efforts.

> 
> tarantool> os.setenv("a","b")
> ---
> ...
> 
> tarantool> os.getenv("a")
> ---
> - b
> ...
> 
> tarantool> os.execute('echo $a')
> b
> ---
> 
> > local function luacmd(args)
> >   -- arg[-1] is guaranteed to be not nil.
> >   local idx = -2
> > @@ -13,6 +18,12 @@ local function luacmd(args)
> >   return table.concat(args, ' ', idx + 1, -1)
> > end
> > 
> > +local function unshiftenv(variable, value, sep)
> > +  local envvar = os.getenv(variable)
> > +  return ('%s="%s%s"'):format(variable, value,
> > +                              envvar and ('%s%s'):format(sep, envvar) or '')
> > +end
> > +
> > function M.selfrun(arg, checks)
> >   -- If TEST_SELFRUN is set, just execute the test payload below
> >   -- <selfrun> call, ...
> > @@ -24,18 +35,22 @@ function M.selfrun(arg, checks)
> > 
> >   test:plan(#checks)
> > 
> > +  local libext = package.cpath:match('?.(%a+);')
> >   local vars = {
> >     LUABIN = luacmd(arg),
> >     SCRIPT = arg[0],
> >     PATH   = arg[0]:gsub('%.test%.lua', ''),
> > -    SUFFIX = package.cpath:match('?.(%a+);'),
> > +    SUFFIX = libext,
> > +    ENV = table.concat({
> > +      unshiftenv('LUA_PATH', '<PATH>/?.lua', ';'),
> > +      unshiftenv('LUA_CPATH', '<PATH>/?.<SUFFIX>', ';'),
> > +      unshiftenv((libext == 'dylib' and 'DYLD' or 'LD') .. '_LIBRARY_PATH',
> > +                 '<PATH>', ':'),
> > +      'TEST_SELFRUN=1',
> > +    }, ' '),
> >   }
> > 
> > -  local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
> > -                          'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
> > -                          'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> > -                          'TEST_SELFRUN=1' ..
> > -                          '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> > +  local cmd = string.gsub('<ENV> <LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> > 
> >   for _, ch in pairs(checks) do
> >     local testf = test[ch.test]
> > @@ -58,4 +73,16 @@ function M.skipcond(condition, message)
> >   os.exit(test:check() and 0 or 1)
> > end
> > 
> > +function M.tweakenv(condition, variable)
> > +  if not condition or os.getenv(variable) then return end
> > +  local testvar = assert(os.getenv('TEST_' .. variable),
> > +                         ('Neither %s nor auxiliary TEST_%s variables are set')
> > +                         :format(variable, variable))
> > +  -- XXX: The third argument of setenv(3) is set to zero to forbid
> > +  -- overwriting the <variable>. Since there is the check above
> > +  -- whether this <variable> is set in the process environment, it
> > +  -- just makes this solution foolproof.
> 
> So, if you check the presence there’s no need in third argument and os.setenv()
> can be used? This can be done instead of comment above - and I’m fine. 

No, this doesn't block me from using <os.setenv>, but rather allows to
tweak the process environment via setenv(3) in a more safe way.

> 
> > +  ffi.C.setenv(variable, testvar, 0)
> > +end
> > +
> > return M
> > -- 
> > 2.25.0
> > 
> 

[1]: https://github.com/tarantool/tarantool/issues/4577

-- 
Best regards,
IM