Re: [Tarantool-patches] [PATCH luajit] Always exit after machine code page protection change fails.

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

Hi, Igor!
Thanks for the patch!
Please, consider my comments below.

On 01.09.23, Igor Munkin wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> (cherry picked from commit c50232eb320d56d526ba5e6cb5bda8cf5a848a55)
> 
> Unfortunately, <exit> call had been missing for a long time for the case
> when <mprotect> fails within <mcode_protect>. Though the patch per se is
> quite trivial, the test is not at all. It exploits the fact, that

Typo: s/fact,/fact/

> <mprotect> is used only for protecting area for mcode or callback

Typo: s/area/the area/

> function pointers. Hence, if the test doesn't use FFI at all, it is
> guaranteed that the only <mprotect> called in LuaJIT runtime locates in
> <mcode_protect> (that is not true for Tarantool, so the test is disabled
> for integration testing routine). Furthermore, overloading <mprotect> on

Typo: s/for/for the/

> macOS occurs to be not an easy ride either, so running the test on macOS

s/occurs to be/is/
Feel free to ignore.

> is disabled, since this is the common part for all platforms and

Typo: s/disabled,/disabled/

> everything can be checked on Linux in a much more easier way.

Typo: s/more//

> 
> Igor Munkin:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/imun/lj-802-panic-at-mcode-protfail
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9077
> Related issues:
> * https://github.com/tarantool/tarantool/issues/8825
> * https://github.com/LuaJIT/LuaJIT/issues/802
> 
> Regarding the macOS and <mprotect> overriding: I tried several flags to
> make this crap work, but I've finally failed to override <mprotect> via
> the alternative dylib. I add the related changes right below, so anyone
> curious can try my approach.
> 
> ================================================================================
> 
> diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
> index 83a9ae2e..3f28e420 100644
> --- a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
> +++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
> @@ -24,7 +24,11 @@ test:plan(3)
>  -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
>  -- with the given environment, launch options and CLI arguments.
>  local script = require('utils').exec.makecmd(arg, {
> -  env = { LD_PRELOAD = 'mymprotect.so' },
> +  env = {
> +    DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH'),
> +    DYLD_INSERT_LIBRARIES = 'mymprotect.dylib',
> +    LD_PRELOAD = 'mymprotect.so',
> +  },
>    redirect = '2>&1',
>  })
> 
> ================================================================================
> 
> This link[1] claims, that there is no way to override syscalls this way
> on macOS, but I've even failed to override <malloc> or rather <memcpy>.
> DYLD_FORCE_FLATE_NAMESPACE didn't change the result, all DYLD_*
> debugging envvars yield some info, that looks like a gibberish to me
> (comparing to LD_DEBUG=1). I'm giving up (at least for now), but I'm
> open to discuss if someone is interested in debugging this.
> 
> [1]: https://stackoverflow.com/questions/929893/how-can-i-override-malloc-calloc-free-etc-under-os-x

Thanks for the clarification! I suppose that skipcond for macOS is
fine.

> 
>  src/lj_mcode.c                                |  3 +-
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  .../lj-802-panic-at-mcode-protfail.test.lua   | 41 +++++++++++++++++++
>  .../CMakeLists.txt                            |  1 +
>  .../mymprotect.c                              |  6 +++
>  .../lj-802-panic-at-mcode-protfail/script.lua | 12 ++++++
>  6 files changed, 63 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
>  create mode 100644 test/tarantool-tests/lj-802-panic-at-mcode-protfail/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-802-panic-at-mcode-protfail/mymprotect.c
>  create mode 100644 test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
> new file mode 100644
> index 00000000..83a9ae2e
> --- /dev/null
> +++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua

The test doesn't fail before the patch

Neither if run it from command line:

| LUA_PATH="src/?.lua;test/tarantool-tests/?.lua;test/tarantool-tests/?/init.lua;" LD_LIBRARY_PATH="test/tarantool-tests/lj-802-panic-at-mcode-protfail/"  src/luajit test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua
| TAP version 13
| 1..3
| ok - Panic occurred as a result of <mprotect> failure
| ok - LuaJIT exited as a result of the panic (error check)
| ok - LuaJIT exited as a result of the panic (poison check)

nor as as part of `make tarantool-tests` (checked with or wo GC64).

Compiled as the following:
| cmake . -DLUAJIT_ENABLE_WARNINGS=OFF -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=ON && make -j

Tested with the following diff:
===================================================================
diff --git a/src/lj_mcode.c b/src/lj_mcode.c
index a88d16bd..9b59053a 100644
--- a/src/lj_mcode.c
+++ b/src/lj_mcode.c
@@ -180,7 +180,7 @@ static void mcode_protect(jit_State *J, int prot)
 #define MCPROT_RUN      MCPROT_RX
 
 /* Protection twiddling failed. Probably due to kernel security. */
-static LJ_NORET LJ_NOINLINE void mcode_protfail(jit_State *J)
+static LJ_NOINLINE void mcode_protfail(jit_State *J)
 {
   lua_CFunction panic = J2G(J)->panic;
   if (panic) {
@@ -188,7 +188,7 @@ static LJ_NORET LJ_NOINLINE void mcode_protfail(jit_State *J)
     setstrV(L, L->top++, lj_err_str(L, LJ_ERR_JITPROT));
     panic(L);
   }
-  exit(EXIT_FAILURE);
+  // exit(EXIT_FAILURE);
 }
 
 /* Change protection of MCode area. */
===================================================================

WDIDW?

> @@ -0,0 +1,41 @@
> +local tap = require('tap')
> +local test = tap.test('lj-flush-on-trace'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +  -- XXX: This test has to check the particular patch for
> +  -- <mcode_protfail> and <mprotect> is overloaded for this

Typo: s/<mcode_protfail>/<mcode_protfail>,/

> +  -- purpose. However, <mprotect> is used widely in Tarantool

Typo: s/is used widely/is widely used/

> +  -- to play with fiber stacks, so overriding <mprotect> is not
> +  -- suitable to test this feature in Tarantool.
> +  -- luacheck: no global
> +  ['<mprotect> overriding can break Tarantool'] = _TARANTOOL,
> +  -- XXX: Unfortunately, it's too hard to overload (or even
> +  -- impossible, who knows, since Cupertino fellows do not
> +  -- provide any information about their system) something from
> +  -- libsystem_kernel.dylib (the library providing <mprotect>).
> +  -- All in all, this test checks the part, that is common for all

Typo: s/part,/part/

> +  -- platforms, so it's not vital to run this test on macOS, since

Typo: s/macOS,/macOS/

> +  -- everything can be checked on Linux in a much more easier way.

Typo: s/more//

> +  ['<mprotect> cannot be overridden on macOS'] = jit.os == 'OSX',
> +})
> +
> +test:plan(3)
> +

<snipped>

> diff --git a/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua
> new file mode 100644
> index 00000000..661099fa
> --- /dev/null
> +++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua
> @@ -0,0 +1,12 @@
> +jit.opt.start('hotloop=1')
> +
> +-- Run a simple loop that triggers <mprotect> on trace assembling.
> +local a = 0
> +for i = 1, 3 do

Minor: Should it be 4 here to actually *run* a loop and be sure that we
don't execute this part of the code?

> +  a = a + i
> +end
> +
> +-- XXX: Just a simple contract output in case neither panic at

Typo: s/panic/the panic/

> +-- <mprotect>, nor crash occurs (see for LUAJIT_UNPROTECT_MCODE in
> +-- lj_mcode.c for more info).
> +io.write(arg[1])
> -- 
> 2.30.2
> 

-- 
Best regards,
Sergey Kaplun