From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org>, Kirill Yukhin <kyukhin@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: [Tarantool-patches] [PATCH luajit] core: fix cur_L restoration on error throw Date: Mon, 16 Aug 2021 13:19:49 +0300 [thread overview] Message-ID: <20210816101949.25035-1-skaplun@tarantool.org> (raw) This change is a kind of revertion of commits ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 ('Update cur_L on exceptional path') and 97699d9ee2467389b6aea21a098e38aff3469b5f ('Fix cur_L tracking on exceptional path'). When an error is thrown on the coroutine that is not the one being currently executed, `cur_L` is not set up. Hence, when the running trace exits at assertion guard right after the error is caught, Lua state is restored from the incorrect `cur_L`. As a result the resulting stack is inconsistent and the crash occurs. Aforementioned patches fix the behaviour only for x86/x64 architectures. This patch updates the `cur_L` within `lj_err_throw()` to the given lua_State, where the error is raised, since this is the only coroutine that can proceed later. Also, it removes unnecessary restorations of `cur_L` at C and FF exception handlers in the VM. Nevertheless, throwing an error at non-currently executed coroutine is a violation of Lua/C API. So, in the nearest possible future this patch should be replaced within the corresponding assert. Resolves tarantool/tarantool#6189 Relates to tarantool/tarantool#6323 Follows up tarantool/tarantool#1516 --- Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6189-curL Issues: * https://github.com/tarantool/tarantool/issues/6189 * https://github.com/tarantool/tarantool/issues/6323 * https://github.com/tarantool/tarantool/issues/1516 Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6189-curL CI is red on FreeBSD, so the patch is probably not ready to merge. I've tried to reproduce it on sh8, but I've failed. sh4 is still not working and is used for running our CI jobs. Stay tuned! src/lj_err.c | 6 ++++ src/vm_x64.dasc | 2 -- src/vm_x86.dasc | 2 -- test/tarantool-tests/CMakeLists.txt | 1 + test/tarantool-tests/gh-6189-cur_L.test.lua | 25 +++++++++++++ .../gh-6189-cur_L/CMakeLists.txt | 1 + test/tarantool-tests/gh-6189-cur_L/libcur_L.c | 36 +++++++++++++++++++ 7 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 test/tarantool-tests/gh-6189-cur_L.test.lua create mode 100644 test/tarantool-tests/gh-6189-cur_L/CMakeLists.txt create mode 100644 test/tarantool-tests/gh-6189-cur_L/libcur_L.c diff --git a/src/lj_err.c b/src/lj_err.c index b6be357e..df9755a2 100644 --- a/src/lj_err.c +++ b/src/lj_err.c @@ -509,6 +509,12 @@ LJ_NOINLINE void LJ_FASTCALL lj_err_throw(lua_State *L, int errcode) global_State *g = G(L); lj_trace_abort(g); setmref(g->jit_base, NULL); + /* + ** FIXME: Throwing error not on the currently executing coroutine is + ** a violation of Lua/C API. The next line should be changed to assert. + ** Set the running lua_State as the one where the error can be handled. + */ + setgcref(g->cur_L, obj2gco(L)); L->status = LUA_OK; #if LJ_UNWIND_EXT err_raise_ext(errcode); diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc index 974047d3..c65e5625 100644 --- a/src/vm_x64.dasc +++ b/src/vm_x64.dasc @@ -513,7 +513,6 @@ static void build_subroutines(BuildCtx *ctx) |->vm_unwind_c_eh: // Landing pad for external unwinder. | mov L:DISPATCH, SAVE_L | mov GL:RB, L:DISPATCH->glref - | mov GL:RB->cur_L, L:DISPATCH | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC | mov DISPATCH, L:DISPATCH->glref // Setup pointer to dispatch table. | add DISPATCH, GG_G2DISP @@ -539,7 +538,6 @@ static void build_subroutines(BuildCtx *ctx) | add DISPATCH, GG_G2DISP | mov PC, [BASE-8] // Fetch PC of previous frame. | mov_false RA - | mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB | mov RB, [BASE] | mov [BASE-16], RA // Prepend false to error message. | mov [BASE-8], RB diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc index ab8e6f27..bdd5c95e 100644 --- a/src/vm_x86.dasc +++ b/src/vm_x86.dasc @@ -654,7 +654,6 @@ static void build_subroutines(BuildCtx *ctx) |->vm_unwind_c_eh: // Landing pad for external unwinder. | mov L:DISPATCH, SAVE_L | mov GL:RB, L:DISPATCH->glref - | mov dword GL:RB->cur_L, L:DISPATCH | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC | mov DISPATCH, L:DISPATCH->glref // Setup pointer to dispatch table. | add DISPATCH, GG_G2DISP @@ -690,7 +689,6 @@ static void build_subroutines(BuildCtx *ctx) | add DISPATCH, GG_G2DISP | mov PC, [BASE-4] // Fetch PC of previous frame. | mov dword [BASE-4], LJ_TFALSE // Prepend false to error message. - | mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB | // INTERP until jump to BC_RET* or return to C. | set_vmstate INTERP | jmp ->vm_returnc // Increments RD/MULTRES and returns. diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt index 2fdb4d1f..df74a277 100644 --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -57,6 +57,7 @@ macro(BuildTestCLib lib sources) endmacro() add_subdirectory(gh-4427-ffi-sandwich) +add_subdirectory(gh-6189-cur_L) add_subdirectory(lj-flush-on-trace) add_subdirectory(misclib-getmetrics-capi) diff --git a/test/tarantool-tests/gh-6189-cur_L.test.lua b/test/tarantool-tests/gh-6189-cur_L.test.lua new file mode 100644 index 00000000..8521af9a --- /dev/null +++ b/test/tarantool-tests/gh-6189-cur_L.test.lua @@ -0,0 +1,25 @@ +local libcur_L = require('libcur_L') +local tap = require('tap') + +local test = tap.test('gh-6189-cur_L') +test:plan(1) + +local function cbool(cond) + if cond then + return 1 + else + return 0 + end +end + +-- Compile function to trace with snapshot. +jit.opt.start('hotloop=1') +cbool(true) +cbool(true) + +pcall(libcur_L.error_from_other_thread) +-- Call with restoration from a snapshot with wrong cur_L. +cbool(false) + +test:ok(true) +os.exit(test:check() and 0 or 1) diff --git a/test/tarantool-tests/gh-6189-cur_L/CMakeLists.txt b/test/tarantool-tests/gh-6189-cur_L/CMakeLists.txt new file mode 100644 index 00000000..1e58e560 --- /dev/null +++ b/test/tarantool-tests/gh-6189-cur_L/CMakeLists.txt @@ -0,0 +1 @@ +BuildTestCLib(libcur_L libcur_L.c) diff --git a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c new file mode 100644 index 00000000..2d58d2e7 --- /dev/null +++ b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c @@ -0,0 +1,36 @@ +#include <lua.h> +#include <lauxlib.h> + +static lua_State *old_L = NULL; + +int throw_error_at_old_thread(lua_State *cur_L) +{ + lua_error(old_L); + /* Unreachable. */ + return 0; +} + +static int error_from_other_thread(lua_State *L) +{ + lua_State *next_cur_L = lua_newthread(L); + old_L = L; + /* Remove thread. */ + lua_pop(L, 1); + /* Do not show frame slot as return result after error. */ + lua_pushnil(L); + lua_pushcfunction(next_cur_L, throw_error_at_old_thread); + lua_call(next_cur_L, 0, 0); + /* Unreachable. */ + return 0; +} + +static const struct luaL_Reg libcur_L[] = { + {"error_from_other_thread", error_from_other_thread}, + {NULL, NULL} +}; + +LUA_API int luaopen_libcur_L(lua_State *L) +{ + luaL_register(L, "libcur_L", libcur_L); + return 1; +} -- 2.31.0
next reply other threads:[~2021-08-16 10:21 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-16 10:19 Sergey Kaplun via Tarantool-patches [this message] 2021-08-18 8:49 ` [Tarantool-patches] [PATCH luajit v2] " Sergey Kaplun via Tarantool-patches 2021-08-18 16:57 ` Igor Munkin via Tarantool-patches 2021-08-18 20:03 ` Sergey Kaplun via Tarantool-patches 2021-08-18 20:26 ` Igor Munkin via Tarantool-patches 2021-08-19 8:23 ` Igor Munkin via Tarantool-patches 2021-08-19 7:42 ` [Tarantool-patches] [PATCH luajit] " Kirill Yukhin via Tarantool-patches 2021-08-19 7:56 ` Vitaliia Ioffe via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210816101949.25035-1-skaplun@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=kyukhin@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit] core: fix cur_L restoration on error throw' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox