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

[Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi!

Thanks for the patch!

Couple of nits, LGTM.

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)

> The first issue relates to the way modules loaded at runtime are built
					    ^ to be
> 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. 

> 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(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 3e36ff86..c793ad60 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -69,11 +69,46 @@ set(LUA_PATH
> )
> set(LUA_TEST_SUFFIX .test.lua)
> set(LUA_TEST_FLAGS --failures --shuffle)
> +set(LUA_TEST_ENV
> +  "LUA_PATH=\"${LUA_PATH}\;\;\""
> +  "LUA_CPATH=\"${LUA_CPATH}\;\;\""
> +)
> 
> if(CMAKE_VERBOSE_MAKEFILE)
>   list(APPEND LUA_TEST_FLAGS --verbose)
> endif()
> 
> +# XXX: 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.
> +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +  # XXX: Apple tries their best to "protect their users from
> +  # malware". As a result SIP (see the link[1] below) 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 (like /usr/bin/env used for preparing testing
> +  # environment). For more info, see the docs[2] below.
> +  #
> +  # 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.
> +  #
> +  # [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
> +  list(APPEND LUA_TEST_ENV TEST_DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +else()
> +  list(APPEND LUA_TEST_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +endif()
> +
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> # with dependecies are set in scope of BuildTestLib macro.
> add_custom_target(tarantool-tests
> @@ -83,9 +118,7 @@ add_custom_command(TARGET tarantool-tests
>   COMMENT "Running Tarantool tests"
>   COMMAND
>   env
> -    LUA_PATH="${LUA_PATH}\;\;"
> -    LUA_CPATH="${LUA_CPATH}\;\;"
> -    LD_LIBRARY_PATH="${LD_LIBRARY_PATH}"
> +    ${LUA_TEST_ENV}
>     ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
>       --exec '${LUAJIT_TEST_COMMAND}'
>       --ext ${LUA_TEST_SUFFIX}
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 64df5dbd..651dc3f4 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
> -- Disabled on *BSD due to #4819.
> utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> 
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
> utils.selfrun(arg, {
>   {
>     arg = {
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index edc5cf61..1ad4f832 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -3,6 +3,10 @@ local utils = require('utils')
> -- Disabled on *BSD due to #4819.
> utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> 
> +-- XXX: Tweak the process environment to get around SIP.
> +-- See the comment in suite CMakeLists.txt for more info.
> +utils.tweakenv(jit.os == 'OSX', 'DYLD_LIBRARY_PATH')
> +
> utils.selfrun(arg, {
>   {
>     arg = {
> 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.

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. 

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