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

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

Igor,

Thanks for the patch!

LGTM, except a few nitpicks below.

On 05.04.21, Igor Munkin wrote:
> This patch resolves the issue with running the tests with auxiliary
> dynamically loaded modules in case of out-of-source build.
> 
> The first issue relates to the way modules loaded at runtime are built
> 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
> 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.

This paragraph looks uninformative, looks like you can just use
"Moreover, " for the next sentence instead.

> 
> 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].

Sorry, but this paragraph is too wordy. I propose to reformulate it like
the following:

| Moreover, Apple tries their best to "protect their users from malicious
| software". As a result, SIP[1] has been designed and released. As a part
| of it, environment variables starting with DYLD_ and LD_ are unset
| for child process started by system programs[2].

Feel free to ignore.

> 
> That which does not kill us makes us stronger: fortunately, these

I suppose you can just drop this preamble.

| 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>.

> 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.

The last two sentences are redundant. Remove them please.

> 
> [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.

Same comments as for the commit message.

> +  #
> +  # [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);
> +]])

Side note: looks like we need to move `os.setenv()` fucntion from the
Tarantool's built-ins onboard as `misc.setenv()`.

> +

<skipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun