[Tarantool-patches] [PATCH luajit] Always exit after machine code page protection change fails.
Sergey Kaplun
skaplun at tarantool.org
Sun Sep 3 11:13:11 MSK 2023
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 at 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
More information about the Tarantool-patches
mailing list