Hi, Igor! Thanks for the patch and the fixes! LGTM   -- Best regards, Maxim Kokryashkin     >Вторник, 5 сентября 2023, 14:49 +03:00 от Igor Munkin : >  >Sergey, > >Thanks for your review! I've replied to your comments inline. > >On 03.09.23, Sergey Kaplun wrote: >> Hi, Igor! >> Thanks for the patch! >> Please, consider my comments below. >> >> On 01.09.23, Igor Munkin wrote: >> > From: Mike Pall >> > >> > Reported by Sergey Kaplun. >> > >> > (cherry picked from commit c50232eb320d56d526ba5e6cb5bda8cf5a848a55) >> > >> > Unfortunately, call had been missing for a long time for the case >> > when fails within . Though the patch per se is >> > quite trivial, the test is not at all. It exploits the fact, that >> >> Typo: s/fact,/fact/ >> >> > 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 called in LuaJIT runtime locates in >> > (that is not true for Tarantool, so the test is disabled >> > for integration testing routine). Furthermore, overloading 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// >> >> > > >Thanks for proofreading the commit message; the new one with the fixes >can be found below: > >================================================================================ > >Always exit after machine code page protection change fails. > >Reported by Sergey Kaplun. > >(cherry picked from commit c50232eb320d56d526ba5e6cb5bda8cf5a848a55) > >Unfortunately, call had been missing for a long time for the case >when fails within . Though the patch per se is >quite trivial, the test is not at all. It exploits the fact that > is used only for protecting the area for mcode or callback >function pointers. Hence, if the test doesn't use FFI at all, it is >guaranteed that the only called in LuaJIT runtime locates in > (that is not true for Tarantool, so the test is disabled >for the integration testing routine). Furthermore, attempts to overload > on macOS occur to be not an easy ride either, so running the >test on macOS is disabled since this is the common part for all >platforms and everything can be checked on Linux in a much easier way. > >Igor Munkin: >* added the description and the test for the problem > >Part of tarantool/tarantool#8825 > >================================================================================ > >> > 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 >> > > > > >> > 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 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? > >Building LuaJIT with the internal assertions, LOL. Anyway, thanks for >noticing this. I've added additional check to the test (see the >incremental diff below): > >================================================================================ > >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..f4dc4e1c 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 >@@ -19,7 +19,7 @@ local test = tap.test('lj-flush-on-trace'):skipcond({ >   [' cannot be overridden on macOS'] = jit.os == 'OSX', > }) >  >-test:plan(3) >+test:plan(4) >  > -- runs %testname%/script.lua by > -- with the given environment, launch options and CLI arguments. >@@ -35,6 +35,8 @@ test:like(output, 'runtime code generation failed, restricted kernel%?', >           'Panic occurred as a result of failure') > test:unlike(output, 'Segmentation fault', >             'LuaJIT exited as a result of the panic (error check)') >+test:unlike(output, 'Aborted', >+ 'LuaJIT exited as a result of the panic (assertion check)') > test:unlike(output, poison, >             'LuaJIT exited as a result of the panic (poison check)') >  > >================================================================================ > >> >> > @@ -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 >> > + -- and is overloaded for this >> >> Typo: s//,/ >> >> > + -- purpose. However, is used widely in Tarantool >> >> Typo: s/is used widely/is widely used/ >> >> > + -- to play with fiber stacks, so overriding is not >> > + -- suitable to test this feature in Tarantool. >> > + -- luacheck: no global >> > + [' 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 ). >> > + -- 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// >> >> > + [' cannot be overridden on macOS'] = jit.os == 'OSX', >> > +}) >> > + >> > +test:plan(3) >> > + > >All the typos in the test code are fixed; the diff is below: > >================================================================================ > >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 f4dc4e1c..94f4314f 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 >@@ -3,8 +3,8 @@ 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 >- -- and is overloaded for this >- -- purpose. However, is used widely in Tarantool >+ -- , and is overloaded for this >+ -- purpose. However, is widely used in Tarantool >   -- to play with fiber stacks, so overriding is not >   -- suitable to test this feature in Tarantool. >   -- luacheck: no global >@@ -13,9 +13,9 @@ local test = tap.test('lj-flush-on-trace'):skipcond({ >   -- impossible, who knows, since Cupertino fellows do not >   -- provide any information about their system) something from >   -- libsystem_kernel.dylib (the library providing ). >- -- All in all, this test checks the part, that is common for all >- -- platforms, so it's not vital to run this test on macOS, since >- -- everything can be checked on Linux in a much more easier way. >+ -- All in all, this test checks the part that is common for all >+ -- platforms, so it's not vital to run this test on macOS since >+ -- everything can be checked on Linux in a much easier way. >   [' cannot be overridden on macOS'] = jit.os == 'OSX', > }) >  > >================================================================================ > >> > 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 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? > >But 3 is this dedicated iteration. Here are some details regarding the >loop recording: >* The first iteration is executed until the corresponding loop bytecode >  (BC_FORL, IIRC) is reached. >* Starting from the first occurrence of the aforementioned bytecode, the >  compiler starts recording the loop. This is the second iteration. >* To successfully finalize the compilation of the loop, the jump at that >  bytecode should be done back to the beginning of the loop body (this >  is the heuristic of JIT compiler, since the loop is not considered >  "hot" otherwise and there is no reason to finalize this trace). If the >  jump target is valid in the sense described above, the trace is >  finalized and, since the jump targets back to the loop body, the next >  loop iteration is run. >* Since the trace is already compiled, the third and the last iteration >  is run via the mcode instead of VM interpreting the bytecode. At this >  point the execution of the trace is finished with the corresponding >  guard checking the "iterator" value. > >So, if you want one "full" iteration and one "to-be-exited" iteration, I >can increment the right boundary of the loop, but I doubt that it's >strongly required here (since we check that the compilation is failed at >the assembling phase). > >> >> > + a = a + i >> > +end >> > + >> > +-- XXX: Just a simple contract output in case neither panic at >> >> Typo: s/panic/the panic/ >> > >All the typos in the script code are also fixed; the diff is below: > >================================================================================ > >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 >index 661099fa..201a8ff2 100644 >--- a/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua >+++ b/test/tarantool-tests/lj-802-panic-at-mcode-protfail/script.lua >@@ -6,7 +6,7 @@ for i = 1, 3 do >   a = a + i > end >  >--- XXX: Just a simple contract output in case neither panic at >+-- XXX: Just a simple contract output in case neither the panic at > -- , nor crash occurs (see for LUAJIT_UNPROTECT_MCODE in > -- lj_mcode.c for more info). > io.write(arg[1]) > >================================================================================ > >> > +-- , 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 > >-- >Best regards, >IM