Skip to content

Commit

Permalink
core: fix cur_L restoration on error throw
Browse files Browse the repository at this point in the history
This change is a kind of revertion of commits
ed412cd ('Update cur_L on exceptional
path') and 97699d9 ('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
  • Loading branch information
Buristan committed Aug 15, 2021
1 parent 3a2e484 commit 12e7226
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 4 deletions.
6 changes: 6 additions & 0 deletions src/lj_err.c
Expand Up @@ -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);
Expand Down
2 changes: 0 additions & 2 deletions src/vm_x64.dasc
Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 0 additions & 2 deletions src/vm_x86.dasc
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions test/tarantool-tests/CMakeLists.txt
Expand Up @@ -57,6 +57,7 @@ macro(BuildTestCLib lib sources)
endmacro()

add_subdirectory(gh-4427-ffi-sandwich)
add_subdirectory(gh-6189-curL)
add_subdirectory(lj-flush-on-trace)
add_subdirectory(misclib-getmetrics-capi)

Expand Down
25 changes: 25 additions & 0 deletions test/tarantool-tests/gh-6189-curL.test.lua
@@ -0,0 +1,25 @@
local libcurl = require('libcurL')
local tap = require('tap')

local test = tap.test('gh-6189-curL')
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(libcurl.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)
1 change: 1 addition & 0 deletions test/tarantool-tests/gh-6189-curL/CMakeLists.txt
@@ -0,0 +1 @@
BuildTestCLib(libcurL libcurL.c)
36 changes: 36 additions & 0 deletions test/tarantool-tests/gh-6189-curL/libcurL.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 libcurL[] = {
{"error_from_other_thread", error_from_other_thread},
{NULL, NULL}
};

LUA_API int luaopen_libcurL(lua_State *L)
{
luaL_register(L, "libcurL", libcurL);
return 1;
}

0 comments on commit 12e7226

Please sign in to comment.