From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH v3 6/6] box: introduce Lua persistent functions
Date: Tue, 18 Jun 2019 19:23:09 +0300 [thread overview]
Message-ID: <20190618162309.agkhc7lp6i4c2wsx@esperanza> (raw)
In-Reply-To: <0b5ebd5a39cfe1e2d964c1e29ee4c9f09fe1a751.1560433806.git.kshcherbatov@tarantool.org>
On Thu, Jun 13, 2019 at 05:08:26PM +0300, Kirill Shcherbatov wrote:
> This patch changes a format of the _func system space
> to support persistent Lua functions. The format of the
> system space is
>
> [<id> UINT, <owner> UINT, <name> STR, <setuid> UINT,
> <language> STR, <body> STR, <is_deterministic> BOOL,
> <is_sandboxed>, <opts> MAP]
I guess this part could be dropped as it's covered by the docbot request
right below.
>
> Closes #4182
> Needed for #1260
>
> @TarantoolBot document
> Title: Persistent Lua functions
>
> Now Tarantool supports 'persistent' Lua functions.
> Such functions are stored in snapshoot and are available after
snapshot
> restart.
> To create a persistent Lua function, specify a function body
> in box.schema.func.create call:
> e.g. body = "function(a, b) return a + b end"
>
> A Lua persistent function may be 'sandboxed'. The 'sandboxed'
> function is executed in isolated environment:
> a. only limited set of Lua functions and modules are available:
> -assert -error -pairs -ipairs -next -pcall -xpcall -type
> -print -select -string -tonumber -tostring -unpack -math -utf8;
> b. global variables are forbidden
>
> Finally, the new 'is_deterministic' flag allows to mark a
> registered function as deterministic, i.e. the function that
> can produce only one result for a given list of parameters.
>
> The new box.schema.func.create interface is:
> box.schema.func.create('funcname', <setuid = true|FALSE>,
> <if_not_exists = true|FALSE>, <language = LUA|c>,
> <body = string ('')>, <is_deterministic = true|FALSE>,
> <is_sandboxed = true|FALSE>)
Why is FALSE written in CAPS? :)
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 081786820..5232a56c1 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2569,6 +2587,17 @@ func_def_new_from_tuple(struct tuple *tuple)
> /* Lua is the default. */
> def->language = FUNC_LANGUAGE_LUA;
> }
> + if (field_count > BOX_FUNC_FIELD_BODY) {
> + assert(field_count > BOX_FUNC_FIELD_OPTS);
One can try to insert a tuple that has 'body', but doesn't have 'opts'
field. We shouldn't crash in this case, but rather fail gracefully. So
please remove this assertion.
> + def->is_deterministic =
> + tuple_field_bool_xc(tuple,
> + BOX_FUNC_FIELD_IS_DETERMINISTIC);
> + def->is_sandboxed =
> + tuple_field_bool_xc(tuple,
> + BOX_FUNC_FIELD_IS_SANDBOXED);
> + } else {
> + def->is_deterministic = false;
> + }
> def_guard.is_active = false;
> return def;
> }
> diff --git a/src/box/func_def.h b/src/box/func_def.h
> index 7df6678d8..4886d5440 100644
> --- a/src/box/func_def.h
> +++ b/src/box/func_def.h
> @@ -58,11 +58,24 @@ struct func_def {
> uint32_t fid;
> /** Owner of the function. */
> uint32_t uid;
> + /** Definition of the persistent function. */
> + char *body;
> /**
> * True if the function requires change of user id before
> * invocation.
> */
> bool setuid;
> + /**
> + * Whether this function is deterministic (can produce
> + * only one result for a given list of parameters).
> + */
> + bool is_deterministic;
> + /**
> + * Whether the routine mast be initialized with isolated
s/mast/must
> + * sandbox where only a limited number if functions is
> + * available.
> + */
> + bool is_sandboxed;
> /**
> * The language of the stored function.
> */
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index 90c28a160..43445373f 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -573,22 +573,142 @@ box_lua_eval(const char *expr, uint32_t expr_len,
> struct func_lua {
> /** Function object base class. */
> struct func base;
> + /**
> + * The reference index of Lua function object.
> + * Is equal to LUA_REFNIL when undefined.
IMO better say:
For a persistent function: a reference to the function body.
Otherwise LUA_REFNIL.
> + */
> + int lua_ref;
> };
>
> static struct func_vtab func_lua_vtab;
> +static struct func_vtab func_persistent_lua_vtab;
> +
> +static const char *default_sandbox_exports[] = {
> + "assert", "error", "ipairs", "math", "next", "pairs", "pcall", "print",
> + "select", "string", "table", "tonumber", "tostring", "type", "unpack",
> + "xpcall", "utf8",
> +};
> +
> +/**
> + * Assemble a new sandbox with given exports table on top of the
> + * Lua stack. All modules in exports list are copying deeply
> + * to ensure the immutablility of this system object.
> + */
> +static int
> +luaT_prepare_sandbox(struct lua_State *L, const char *exports[],
Let's call this function simply 'prepare_lua_sandbox'.
> + uint32_t exports_count)
Should be named 'export_count'. Also, please use 'int'.
> +{
> + int count, rc = -1;
> + const char *deepcopy = "table.deepcopy";
> + int luaL_deepcopy_func_ref = LUA_REFNIL;
> + if (luaT_func_find(L, deepcopy, deepcopy + strlen(deepcopy),
> + &count) != 0)
> + goto end;
> + luaL_deepcopy_func_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> + assert(luaL_deepcopy_func_ref != LUA_REFNIL);
> + lua_createtable(L, exports_count, 0);
> + for (unsigned i = 0; i < exports_count; i++) {
> + int count;
> + uint32_t name_len = strlen(exports[i]);
> + if (luaT_func_find(L, exports[i], exports[i] + name_len,
> + &count) != 0)
> + goto end;
> + switch (lua_type(L, -1)) {
> + case LUA_TTABLE:
> + lua_rawgeti(L, LUA_REGISTRYINDEX,
> + luaL_deepcopy_func_ref);
> + lua_insert(L, -2);
> + lua_call(L, 1, LUA_MULTRET);
> + FALLTHROUGH;
You don't really need to FALLTHROUGH here. Let's please use 'break'.
> + case LUA_TFUNCTION:
> + break;
> + default:
> + unreachable();
> + }
> + lua_setfield(L, -2, exports[i]);
> + }
> + rc = 0;
> +end:
> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, luaL_deepcopy_func_ref);
> + return rc;
> +}
> +
> +static int
> +func_persistent_lua_load(struct lua_State *L)
A comment would be welcome.
> +{
> + struct func_lua *func = (struct func_lua *) lua_topointer(L, 1);
> + assert(func->base.def->body != NULL);
> + lua_settop(L, 0);
> +
> + struct region *region = &fiber()->gc;
> + size_t region_svp = region_used(region);
> + const char *load_pref = "return ";
> + uint32_t load_str_sz =
> + strlen(load_pref) + strlen(func->base.def->body) + 1;
> + char *load_str = region_alloc(region, load_str_sz);
> + if (load_str == NULL) {
> + diag_set(OutOfMemory, load_str_sz, "region", "load_str");
> + goto error;
> + }
> + sprintf(load_str, "%s%s", load_pref, func->base.def->body);
> + if (luaL_loadstring(L, load_str) != 0 ||
> + luaT_call(L, 0, 1) != 0 || !lua_isfunction(L, -1)) {
> + diag_set(ClientError, ER_LOAD_FUNCTION, func->base.def->name,
> + func->base.def->body);
Should be an error message here, not a function body. I think we should
pass diag_last_error()->errmsg.
> + goto error;
> + }
> + if (func->base.def->is_sandboxed) {
> + if (luaT_prepare_sandbox(L, default_sandbox_exports,
> + nelem(default_sandbox_exports)) != 0) {
> + diag_set(ClientError, ER_LOAD_FUNCTION,
> + func->base.def->name, func->base.def->body);
Ditto.
> + goto error;
> + }
> + lua_setfenv(L, -2);
> + }
> + func->lua_ref = luaL_ref(L, LUA_REGISTRYINDEX);
> + region_truncate(region, region_svp);
> + return lua_gettop(L);
> +error:
> + region_truncate(region, region_svp);
> + return luaT_error(L);
> +}
>
> struct func *
> func_lua_new(struct func_def *def)
> {
> - (void) def;
> assert(def->language == FUNC_LANGUAGE_LUA);
> + if (def->is_sandboxed && def->body == NULL) {
> + diag_set(ClientError, ER_CREATE_FUNCTION, def->name,
> + "is_sandboxed option may be set only for persistent "
> + "Lua function (when body option is set)");
> + return NULL;
> + }
> struct func_lua *func =
> (struct func_lua *) malloc(sizeof(struct func_lua));
> if (func == NULL) {
> diag_set(OutOfMemory, sizeof(*func), "malloc", "func");
> return NULL;
> }
> - func->base.vtab = &func_lua_vtab;
> + if (def->body != NULL) {
> + /** Load persistent Lua function. */
> + struct lua_State *L = lua_newthread(tarantool_L);
> + int coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
> + lua_pushcfunction(L, func_persistent_lua_load);
> + func->base.def = def;
> + lua_pushlightuserdata(L, func);
> + int rc = luaT_call(L, 1, LUA_MULTRET);
Is this really necessary? Can't you call func_persistent_lua_load
directly? AFAICS it doesn't throw any exceptions.
> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref);
> + if (rc != 0) {
> + free(func);
> + return NULL;
> + }
> + assert(func->lua_ref != LUA_REFNIL);
> + func->base.vtab = &func_persistent_lua_vtab;
> + } else {
> + func->lua_ref = LUA_REFNIL;
> + func->base.vtab = &func_lua_vtab;
> + }
> return &func->base;
> }
>
> @@ -614,6 +734,53 @@ static struct func_vtab func_lua_vtab = {
> .destroy = func_lua_unload,
> };
>
> +static void
> +func_persistent_lua_unload(struct func *base)
func_persistent_lua_load takes lua_State while this function takes
struct func. Looks inconsistent.
> +{
> + assert(base != NULL && base->def->language == FUNC_LANGUAGE_LUA &&
> + base->def->body != NULL);
> + assert(base->vtab == &func_persistent_lua_vtab);
> + struct func_lua *func = (struct func_lua *) base;
> + luaL_unref(tarantool_L, LUA_REGISTRYINDEX, func->lua_ref);
> + func->lua_ref = LUA_REFNIL;
> +}
> +
> +static int
> +func_persistent_lua_call_cb(lua_State *L)
I'd try to make this function more generic: name it
execute_lua_call_by_ref and pass lua_ref to it instead of
func_lua. We could probably reuse the same context we use
for executing CALL/EVAL (lua_exec_ctx).
> +{
> + struct port *in_port = (struct port *) lua_topointer(L, 1);
> + struct func_lua *func = (struct func_lua *) lua_topointer(L, 2);
> + lua_settop(L, 0); /* clear the stack to simplify the logic below */
> +
> + assert(func->lua_ref != LUA_REFNIL);
> + lua_rawgeti(L, LUA_REGISTRYINDEX, func->lua_ref);
> +
> + /* Push the rest of args (a tuple). */
> + int top = lua_gettop(L);
> + port_dump_lua(in_port, L, true);
> + int arg_count = lua_gettop(L) - top;
> +
> + lua_call(L, arg_count, LUA_MULTRET);
> + return lua_gettop(L);
> +}
> +
> +static inline int
> +func_persistent_lua_call(struct func *base, struct port *in_port,
> + struct port *out_port)
> +{
> + assert(base != NULL && base->def->language == FUNC_LANGUAGE_LUA &&
> + base->def->body != NULL);
> + assert(base->vtab == &func_persistent_lua_vtab);
> + return box_process_lua(func_persistent_lua_call_cb, base,
> + in_port, out_port);
> +
> +}
> +
> +static struct func_vtab func_persistent_lua_vtab = {
> + .call = func_persistent_lua_call,
> + .destroy = func_persistent_lua_unload,
> +};
> +
> static int
> lbox_module_reload(lua_State *L)
> {
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 9c3ee063c..9d8df54dc 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -820,10 +821,32 @@ local function create_vcollation_space()
> box.space[box.schema.VCOLLATION_ID]:format(format)
> end
>
> +local function upgrade_func_to_2_2_1()
> + log.info("Update _func format")
> + local _func = box.space[box.schema.FUNC_ID]
> + local format = {}
> + format[1] = {name='id', type='unsigned'}
> + format[2] = {name='owner', type='unsigned'}
> + format[3] = {name='name', type='string'}
> + format[4] = {name='setuid', type='unsigned'}
> + format[5] = {name='language', type='string'}
> + format[6] = {name='body', type='string'}
> + format[7] = {name='is_deterministic', type='boolean'}
> + format[8] = {name='is_sandboxed', type='boolean'}
> + format[9] = {name='opts', type='map'}
> + for _, v in box.space._func:pairs() do
> + _ = box.space._func:replace({v.id, v.owner, v.name, v.setuid,
> + v[5] or 'LUA', '', false, false,
Why not v.language?
> + setmap({})})
> + end
> + _func:format(format)
> +end
> +
> local function upgrade_to_2_2_1()
> upgrade_sequence_to_2_2_1()
> upgrade_ck_constraint_to_2_2_1()
> create_vcollation_space()
> + upgrade_func_to_2_2_1()
> end
>
> --------------------------------------------------------------------------------
> diff --git a/test/box/function1.result b/test/box/function1.result
> index 331bd466a..172f56082 100644
> --- a/test/box/function1.result
> +++ b/test/box/function1.result
> @@ -504,3 +510,120 @@ box.schema.func.drop('secret_leak')
> box.schema.func.drop('secret')
> ---
> ...
> +--
> +-- gh-4182: Introduce persistent Lua functions.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +body = [[function(tuple)
> + if type(tuple.address) ~= 'string' then
> + return nil, 'Invalid field type'
> + end
> + local t = tuple.address:upper():split()
> + for k,v in pairs(t) do t[k] = v end
> + return t
> + end
> +]]
> +test_run:cmd("setopt delimiter ''");
> +---
> +...
> +box.schema.func.create('addrsplit', {body = body, language = "C"})
> +---
> +- error: 'Failed to create function ''addrsplit'': body and is_sandboxed options are
> + not compatible with C language'
> +...
> +box.schema.func.create('addrsplit', {is_sandboxed = true, language = "C"})
> +---
> +- error: 'Failed to create function ''addrsplit'': body and is_sandboxed options are
> + not compatible with C language'
> +...
> +box.schema.func.create('addrsplit', {is_sandboxed = true})
> +---
> +- error: 'Failed to create function ''addrsplit'': is_sandboxed option may be set
> + only for persistent Lua function (when body option is set)'
> +...
> +box.schema.func.create('invalid', {body = "function(tuple) ret tuple"})
> +---
> +- error: 'Failed to dynamically load function ''invalid'': function(tuple) ret tuple'
> +...
Please also check what happens when body is loaded okay, but happens to
be not a function.
next prev parent reply other threads:[~2019-06-18 16:23 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-13 14:08 [PATCH v3 0/6] box: rework functions machinery Kirill Shcherbatov
2019-06-13 14:08 ` [PATCH v3 1/6] box: rework func cache update machinery Kirill Shcherbatov
2019-06-18 10:52 ` Vladimir Davydov
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-13 14:08 ` [PATCH v3 2/6] box: rework box_lua_{call, eval} to use input port Kirill Shcherbatov
2019-06-17 9:35 ` [tarantool-patches] " Konstantin Osipov
2019-06-17 10:27 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-18 12:12 ` Vladimir Davydov
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-19 16:11 ` Vladimir Davydov
2019-06-18 13:58 ` Vladimir Davydov
2019-06-19 18:28 ` [tarantool-patches] " Konstantin Osipov
2019-06-20 7:53 ` Kirill Shcherbatov
2019-06-20 8:09 ` Konstantin Osipov
2019-06-20 8:44 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-19 18:30 ` [tarantool-patches] " Konstantin Osipov
2019-06-13 14:08 ` [PATCH v3 3/6] box: rework func object as a function frontend Kirill Shcherbatov
2019-06-18 13:23 ` Vladimir Davydov
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-13 14:08 ` [PATCH v3 4/6] box: export registered functions in box.func folder Kirill Shcherbatov
2019-06-18 14:06 ` Vladimir Davydov
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-18 16:11 ` Vladimir Davydov
2019-06-13 14:08 ` [PATCH v3 5/6] box: refactor box_lua_find helper Kirill Shcherbatov
2019-06-18 14:22 ` Vladimir Davydov
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
2019-06-13 14:08 ` [PATCH v3 6/6] box: introduce Lua persistent functions Kirill Shcherbatov
2019-06-18 16:23 ` Vladimir Davydov [this message]
2019-06-19 15:51 ` [tarantool-patches] " Kirill Shcherbatov
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=20190618162309.agkhc7lp6i4c2wsx@esperanza \
--to=vdavydov.dev@gmail.com \
--cc=kshcherbatov@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='Re: [PATCH v3 6/6] box: introduce Lua persistent functions' \
/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