From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4296840F3AE for ; Thu, 18 Jun 2020 00:07:43 +0300 (MSK) From: Alexander Turenko Date: Thu, 18 Jun 2020 00:06:50 +0300 Message-Id: In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v2 2/3] merger: clean fiber-local Lua stack after next() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , Igor Munkin Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko merge_source_next() implementations for 'buffer', 'table' and 'tuple' sources use a temporary Lua state to process data passed from Lua. It may use the fiber-local Lua state when it is present. In this case the state is not freed when all operations are done and may be reused later. We should ensure that merge_source_next() code do not leave extra values on the stack, because otherwise the stack may be overflowed. Now those functions may be called only from Lua and if the fiber-local Lua state is present it is the same as one that is passed to a Lua/C function. After a Lua/C call a Lua interpreter automatically removes everything below returned values. So the stack will not accumulate any garbage. However the merge source API is desined in the way to allow to use any merge source from C. If we'll use it this way in a future, we can meet the described problem. The merge_source_next() implementations do not leave any garbage on a Lua stack at success path, but may left something when an error occurs (say, when a Lua iterator generator returns more then two values). I would not bother with finding and fixing all such cases, considering that it would be valid for usual Lua/C code. However it seems reasonable to ensure that a stack is even when we releasing a temporary Lua state. Reported-by: Vladislav Shpilevoy Follows up #4954 --- src/box/lua/merger.c | 47 ++-- test/CMakeLists.txt | 1 + test/box-tap/CMakeLists.txt | 4 + test/box-tap/check_merge_source.c | 108 +++++++++ test/box-tap/gh-4954-merger-via-c.test.lua | 251 +++++++++++++++++++++ 5 files changed, 392 insertions(+), 19 deletions(-) create mode 100644 test/box-tap/CMakeLists.txt create mode 100644 test/box-tap/check_merge_source.c create mode 100755 test/box-tap/gh-4954-merger-via-c.test.lua diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c index cc5626cbc..3e97969c8 100644 --- a/src/box/lua/merger.c +++ b/src/box/lua/merger.c @@ -164,18 +164,27 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format) * wrong stack slot when it will be scheduled for execution after * yield. * - * Return a Lua state on success and set @a coro_ref. This - * reference should be passed to `luaT_release_temp_luastate()`, - * when the state is not needed anymore. + * Return a Lua state on success and set @a coro_ref and @a top. + * These values should be passed to + * `luaT_release_temp_luastate()`, when the state is not needed + * anymore. * * Return NULL and set a diag at failure. */ static struct lua_State * -luaT_temp_luastate(int *coro_ref) +luaT_temp_luastate(int *coro_ref, int *top) { if (fiber()->storage.lua.stack != NULL) { + /* + * Reuse existing stack. In the releasing function + * we should drop a stack top to its initial + * value to don't exhaust available slots by + * many requests in row. + */ + struct lua_State *L = fiber()->storage.lua.stack; *coro_ref = LUA_REFNIL; - return fiber()->storage.lua.stack; + *top = lua_gettop(L); + return L; } /* @@ -197,6 +206,7 @@ luaT_temp_luastate(int *coro_ref) * registry while it is in use. */ *coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); + *top = -1; return L; } @@ -206,14 +216,10 @@ luaT_temp_luastate(int *coro_ref) * It is the other half of `luaT_temp_luastate()`. */ static void -luaT_release_temp_luastate(int coro_ref) +luaT_release_temp_luastate(struct lua_State *L, int coro_ref, int top) { - /* - * FIXME: The reusable fiber-local Lua state is not - * unreferenced here (coro_ref == LUA_REFNIL), but - * it must be truncated to its past top to prevent - * stack overflow. - */ + if (top >= 0) + lua_settop(L, top); luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); } @@ -521,11 +527,12 @@ static int luaL_merge_source_buffer_fetch(struct merge_source_buffer *source) { int coro_ref = LUA_REFNIL; - struct lua_State *L = luaT_temp_luastate(&coro_ref); + int top = -1; + struct lua_State *L = luaT_temp_luastate(&coro_ref, &top); if (L == NULL) return -1; int rc = luaL_merge_source_buffer_fetch_impl(L, source); - luaT_release_temp_luastate(coro_ref); + luaT_release_temp_luastate(L, coro_ref, top); return rc; } @@ -799,11 +806,12 @@ luaL_merge_source_table_next(struct merge_source *base, struct tuple **out) { int coro_ref = LUA_REFNIL; - struct lua_State *L = luaT_temp_luastate(&coro_ref); + int top = -1; + struct lua_State *L = luaT_temp_luastate(&coro_ref, &top); if (L == NULL) return -1; int rc = luaL_merge_source_table_next_impl(L, base, format, out); - luaT_release_temp_luastate(coro_ref); + luaT_release_temp_luastate(L, coro_ref, top); return rc; } @@ -898,7 +906,7 @@ luaL_merge_source_tuple_fetch(struct lua_State *L, /* Handle incorrect results count. */ if (nresult != 2) { - diag_set(IllegalParams, "Expected , got %d " + diag_set(IllegalParams, "Expected , , got %d " "return values", nresult); return -1; } @@ -973,11 +981,12 @@ luaL_merge_source_tuple_next(struct merge_source *base, struct tuple **out) { int coro_ref = LUA_REFNIL; - struct lua_State *L = luaT_temp_luastate(&coro_ref); + int top = -1; + struct lua_State *L = luaT_temp_luastate(&coro_ref, &top); if (L == NULL) return -1; int rc = luaL_merge_source_tuple_next_impl(L, base, format, out); - luaT_release_temp_luastate(coro_ref); + luaT_release_temp_luastate(L, coro_ref, top); return rc; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 697d1b21d..63e5b5015 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -65,6 +65,7 @@ add_custom_target(test-force add_subdirectory(app) add_subdirectory(app-tap) add_subdirectory(box) +add_subdirectory(box-tap) add_subdirectory(unit) add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test/gh-4427-ffi-sandwich ${PROJECT_BINARY_DIR}/third_party/luajit/test/gh-4427-ffi-sandwich) add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test/lj-flush-on-trace ${PROJECT_BINARY_DIR}/third_party/luajit/test/lj-flush-on-trace) diff --git a/test/box-tap/CMakeLists.txt b/test/box-tap/CMakeLists.txt new file mode 100644 index 000000000..a39898bd8 --- /dev/null +++ b/test/box-tap/CMakeLists.txt @@ -0,0 +1,4 @@ +# fiber.h requires tarantool_ev.h from third_party directory. +include_directories(${CMAKE_SOURCE_DIR}/third_party) + +build_module(check_merge_source check_merge_source.c) diff --git a/test/box-tap/check_merge_source.c b/test/box-tap/check_merge_source.c new file mode 100644 index 000000000..8247f9df3 --- /dev/null +++ b/test/box-tap/check_merge_source.c @@ -0,0 +1,108 @@ +#include /* lua_*() */ +#include /* struct luaL_Reg */ +#include "lib/core/diag.h" /* struct error, diag_*() */ +#include "fiber.h" /* fiber_self() */ +#include "lua/utils.h" /* luaL_checkcdata() */ +#include "box/merger.h" /* struct merge_source, + * merge_source_next(), + * struct tuple (opaque) + */ + +/** + * Verify whether a temporary fiber-local Lua state has the same + * amount of stack slots before and after merge_source_next() + * call. + * + * A merge source is designed to be used from plain C code without + * passing any Lua state explicitly. There are merge sources + * ('table', 'buffer', 'tuple') that require temporary Lua stack + * to fetch next tuple and they use fiber-local Lua stack when it + * is available. + * + * Such calls should not left garbage on the fiber-local Lua + * stack, because many of them in row may overflow the stack. + * + * The module built as a separate dynamic library, but it uses + * internal tarantool functions. So it is not a 'real' external + * module, but the stub that imitates usage of a merge source from + * tarantool code. + */ + +/** + * Extract a merge source from the Lua stack. + */ +static struct merge_source * +luaT_check_merge_source(struct lua_State *L, int idx) +{ + uint32_t cdata_type; + struct merge_source **source_ptr = luaL_checkcdata(L, idx, &cdata_type); + assert(source_ptr != NULL); + return *source_ptr; +} + +/** + * Call merge_source_next() virtual method of a merge source. + * + * The purpose of this function is to verify whether the + * fiber-local Lua stack is properly cleaned after + * merge_source_next() call on the passed merge source. + * + * The function is to be called from Lua. Lua API is the + * following: + * + * Parameters: + * + * - merge_source A merge source object to call + * merge_source_next() on it. + * + * Return values: + * + * - is_next_ok Whether the call is successful. + * - err_msg Error message from the call or nil. + * - is_stack_even Whether the fiber-local Lua stack is + * even after the call. + */ +static int +lbox_check_merge_source_call_next(struct lua_State *L) +{ + assert(lua_gettop(L) == 1); + + /* + * Ensure that there is reusable temporary Lua stack. + * + * Note: It may be the same as L (and usually do). + */ + struct lua_State *temporary_L = fiber_self()->storage.lua.stack; + assert(temporary_L != NULL); + + struct tuple *tuple; + struct merge_source *source = luaT_check_merge_source(L, 1); + + int top = lua_gettop(temporary_L); + int rc = merge_source_next(source, NULL, &tuple); + (void) tuple; + bool is_stack_even = lua_gettop(temporary_L) == top; + struct error *e = diag_last_error(diag_get()); + + lua_pushboolean(L, rc == 0); + if (rc == 0) + lua_pushnil(L); + else + lua_pushstring(L, e->errmsg); + lua_pushboolean(L, is_stack_even); + return 3; +} + +/** + * Register the module. + */ +LUA_API int +luaopen_check_merge_source(struct lua_State *L) +{ + static const struct luaL_Reg meta[] = { + {"call_next", lbox_check_merge_source_call_next}, + {NULL, NULL} + }; + luaL_register(L, "merge_source", meta); + return 1; +} diff --git a/test/box-tap/gh-4954-merger-via-c.test.lua b/test/box-tap/gh-4954-merger-via-c.test.lua new file mode 100755 index 000000000..9fbb0919a --- /dev/null +++ b/test/box-tap/gh-4954-merger-via-c.test.lua @@ -0,0 +1,251 @@ +#!/usr/bin/env tarantool + +-- +-- gh-4954: The fiber-local Lua stack should be even after +-- merge_source_next() call from C code. +-- +-- See test/box-tap/check_merge_source.c for more information. +-- + +local fio = require('fio') + +-- Use BUILDDIR passed from test-run or cwd when run w/o +-- test-run to find test/box-tap/check_merge_source.{so,dylib}. +local build_path = os.getenv('BUILDDIR') or '.' +package.cpath = fio.pathjoin(build_path, 'test/box-tap/?.so' ) .. ';' .. + fio.pathjoin(build_path, 'test/box-tap/?.dylib') .. ';' .. + package.cpath + +local buffer = require('buffer') +local msgpack = require('msgpack') +local merger = require('merger') +local tap = require('tap') +local check_merge_source = require('check_merge_source') + +-- {{{ Lua iterator generator functions + +local function triplet() + return 1, 2, 3 +end + +local function wrong_type() + return 1, 2 +end + +local function no_chunks() + return nil +end + +local function bad_buffer() + local buf = buffer.ibuf() + msgpack.encode({foo = 'bar'}, buf) + return 1, buf +end + +-- FIXME: Enable after gh-5048: ('non-array tuple in a buffer +-- leads to assertion fail'). +--[[ +local function bad_tuple_in_buffer() + local tuple = 1 + local buf = buffer.ibuf() + msgpack.encode({tuple}, buf) + return 1, buf +end +]]-- + +local function empty_buffer(_, state) + if state ~= nil then + return nil + end + local buf = buffer.ibuf() + return 1, buf +end + +local function no_tuples_buffer(_, state) + if state ~= nil then + return nil + end + local buf = buffer.ibuf() + msgpack.encode({}, buf) + return 1, buf +end + +local function good_buffer(_, state) + if state ~= nil then + return nil + end + local buf = buffer.ibuf() + local tuple = {1, 2, 3} + msgpack.encode({tuple}, buf) + return 1, buf +end + +local function bad_tuple_in_table() + local tuple = 1 + return 1, {tuple} +end + +local function empty_table(_, state) + if state ~= nil then + return nil + end + return 1, {} +end + +local function good_table(_, state) + if state ~= nil then + return nil + end + local tuple = {1, 2, 3} + return 1, {tuple} +end + +local function bad_tuple() + local tuple = 1 + return 1, tuple +end + +local function good_tuple(_, state) + if state ~= nil then + return nil + end + local tuple = {1, 2, 3} + return 1, tuple +end + +-- }}} + +local cases = { + { + 'buffer source, bad gen function', + source_new = merger.new_buffer_source, + source_gen = triplet, + exp_err = '^Expected , , got 3 return values$', + }, + { + 'buffer source, bad gen result', + source_new = merger.new_buffer_source, + source_gen = wrong_type, + exp_err = '^Expected , $', + }, + { + 'buffer source, bad buffer', + source_new = merger.new_buffer_source, + source_gen = bad_buffer, + exp_err = '^Invalid merge source 0x[0-9a-f]+$', + }, + -- FIXME: Enable after gh-5048: ('non-array tuple in a buffer + -- leads to assertion fail'). + --[[ + { + 'buffer source, bad tuple in buffer', + source_new = merger.new_buffer_source, + source_gen = bad_tuple_in_buffer, + exp_err = '^A tuple must be an array$', + }, + ]]-- + { + 'buffer source, no buffers', + source_new = merger.new_buffer_source, + source_gen = no_chunks, + }, + { + 'buffer source, empty buffer', + source_new = merger.new_buffer_source, + source_gen = empty_buffer, + }, + { + 'buffer source, no tuples buffer', + source_new = merger.new_buffer_source, + source_gen = no_tuples_buffer, + }, + { + 'buffer source, good buffer', + source_new = merger.new_buffer_source, + source_gen = good_buffer, + }, + { + 'table source, bad gen function', + source_new = merger.new_table_source, + source_gen = triplet, + exp_err = '^Expected , , got 3 return values$', + }, + { + 'table source, bad gen result', + source_new = merger.new_table_source, + source_gen = wrong_type, + exp_err = '^Expected ,
$', + }, + { + 'table source, bad tuple in table', + source_new = merger.new_table_source, + source_gen = bad_tuple_in_table, + exp_err = '^A tuple or a table expected, got number$', + }, + { + 'buffer source, no tables', + source_new = merger.new_table_source, + source_gen = no_chunks, + }, + { + 'table source, empty table', + source_new = merger.new_table_source, + source_gen = empty_table, + }, + { + 'table source, good table', + source_new = merger.new_table_source, + source_gen = good_table, + }, + { + 'tuple source, bad gen function', + source_new = merger.new_tuple_source, + source_gen = triplet, + exp_err = '^Expected , , got 3 return values$', + }, + { + 'tuple source, bad gen result', + source_new = merger.new_tuple_source, + source_gen = wrong_type, + exp_err = '^A tuple or a table expected, got number$', + }, + { + 'tuple source, bad tuple', + source_new = merger.new_tuple_source, + source_gen = bad_tuple, + exp_err = '^A tuple or a table expected, got number$', + }, + { + 'tuple source, no tuples', + source_new = merger.new_tuple_source, + source_gen = no_chunks, + }, + { + 'tuple source, good tuple', + source_new = merger.new_tuple_source, + source_gen = good_tuple, + }, +} + +local test = tap.test('gh-4954-merger-via-c') +test:plan(#cases) + +for _, case in ipairs(cases) do + test:test(case[1], function(test) + test:plan(3) + local source = case.source_new(case.source_gen) + local is_next_ok, err_msg, is_stack_even = + check_merge_source.call_next(source) + if case.exp_err == nil then + test:ok(is_next_ok, 'merge_source_next() should succeed') + test:ok(err_msg == nil, 'no error message') + else + test:ok(not is_next_ok, 'merge_source_next() should fail') + test:ok(string.match(err_msg, case.exp_err), 'verify error message', + {err_msg = err_msg, exp_err = case.exp_err}) + end + test:ok(is_stack_even, 'fiber-local Lua stack should be even') + end) +end + +os.exit(test:check() and 0 or 1) -- 2.25.0