[Tarantool-patches] [PATCH luajit] core: fix cur_L restoration on error throw
Sergey Kaplun
skaplun at tarantool.org
Mon Aug 16 13:19:49 MSK 2021
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
More information about the Tarantool-patches
mailing list