Tarantool development patches archive
 help / color / mirror / Atom feed
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


             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