[Tarantool-patches] [PATCH luajit 3/3] test: fix dynamic modules loading on MacOS
Igor Munkin
imun at tarantool.org
Tue Apr 6 21:05:59 MSK 2021
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 at tarantool.org>
>
> regards,
> Sergos
>
>
> > On 5 Apr 2021, at 20:11, Igor Munkin <imun at 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 at tarantool.org>
> > Co-authored-by: Mons Anderson <mons at cpan.org>
> > Signed-off-by: Igor Munkin <imun at 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
More information about the Tarantool-patches
mailing list