From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove SQL built-in functions from _func Date: Mon, 2 Aug 2021 22:12:29 +0200 [thread overview] Message-ID: <9acba339-2934-e5ca-b2eb-47282ba24a89@tarantool.org> (raw) In-Reply-To: <b1a6aace4fcb886cdb839fbab422014630406061.1627598805.git.imeevma@gmail.com> Hi! Thanks for the patch! See 11 comments below. On 30.07.2021 00:48, imeevma@tarantool.org wrote: > This patch removes SQL built-in functions from _func. These functions > could be called directly from Lua, however all they did was returnd an 1. returnd -> returned. > error. After this patch, no SQL built-in functions can be called > directly from LUA. > > Closes #6106 > --- > https://github.com/tarantool/tarantool/issues/6106 > https://github.com/tarantool/tarantool/tree/imeevma/gh-6106-remove-sql-builtins-from-_func > > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index 97afc0b4d..d0ca3db97 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua > @@ -1003,19 +1003,19 @@ end > -------------------------------------------------------------------------------- > -- Tarantool 2.9.1 > -------------------------------------------------------------------------------- > -local function sql_builtin_function_uuid() > +local function remove_sql_builtin_functions_from_func() > local _func = box.space._func > local _priv = box.space._priv > - local datetime = os.date("%Y-%m-%d %H:%M:%S") > - local t = _func:auto_increment({ADMIN, 'UUID', 1, 'SQL_BUILTIN', '', > - 'function', {}, 'any', 'none', 'none', > - false, false, true, {}, setmap({}), '', > - datetime, datetime}) > - _priv:replace{ADMIN, PUBLIC, 'function', t.id, box.priv.X} > + for _, v in _func:pairs() do > + if (v.language == "SQL_BUILTIN") then 2. You do not need () around the condition. > + _priv:delete({2, 'function', v.id}) > + _func:delete({v.id}) > + end > + end > end > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 3772596d6..7c1616a3a 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -326,11 +326,13 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id, > break; > } > if (op == TK_FUNCTION) { > - uint32_t arg_count = p->x.pList == NULL ? 0 : > - p->x.pList->nExpr; > + int arg_count = p->x.pList == NULL ? 0 : > + p->x.pList->nExpr; > struct func *func = > - sql_func_by_signature(p->u.zToken, arg_count); > - if (func == NULL) > + sql_func_by_name(p->u.zToken); > + if (func == NULL || !func->def->exports.sql || 3. Why don't you filter by !func->def->exports.sql in sql_func_by_name()? Is it possible in some places to call a function not exported to SQL? > + (func->def->param_count != -1 && > + func->def->param_count != arg_count)) 4. Why is argument count important? Isn't SQL_FUNC_DERIVEDCOLL below enough? The same 2 comments in the next diff block. > break; > if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) && > arg_count > 0) { > @@ -3979,8 +3981,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > nFarg = pFarg ? pFarg->nExpr : 0; > assert(!ExprHasProperty(pExpr, EP_IntValue)); > zId = pExpr->u.zToken; > - struct func *func = sql_func_by_signature(zId, nFarg); > - if (func == NULL) { > + struct func *func = sql_func_by_name(zId); > + if (func == NULL || !func->def->exports.sql || > + (func->def->param_count != -1 && > + func->def->param_count != nFarg)) { > diag_set(ClientError, ER_NO_SUCH_FUNCTION, > zId); > pParse->is_aborted = true; > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 6ca852dec..b05038e2e 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -47,6 +47,37 @@ > #include <unicode/ucol.h> > #include "box/coll_id_cache.h" > #include "box/schema.h" > +#include "core/assoc.h" 5. core/ prefix is not not necessary. Also it might look better if you group all box/ includes together. > +#include "box/user.h" > + > +static struct mh_strnptr_t *built_in_functions = NULL; > + > +static struct func * > +get_built_in_function(const char *name) > +{ > + uint32_t len = strlen(name); > + mh_int_t func = mh_strnptr_find_inp(built_in_functions, name, len); > + if (func == mh_end(built_in_functions)) > + return NULL; > + return (struct func *) mh_strnptr_node(built_in_functions, func)->val; 6. In the new code we do not use whitespaces after unary operators like cast. But even better - in C you do not need an explicit cast from void * to any other pointer. > +} > + > +static int > +built_in_functions_cache_insert(struct func *func) 7. Please, make the hash methods have the same prefix. For instance, sql_builtin_func_get(), sql_builtin_func_put(). > +{ > + const char *name = func->def->name; > + uint32_t len = strlen(name); > + assert(get_built_in_function(name) == NULL); > + > + uint32_t hash = mh_strn_hash(name, len); > + const struct mh_strnptr_node_t strnode = {name, len, hash, func}; > + mh_int_t k = mh_strnptr_put(built_in_functions, &strnode, NULL, NULL); > + if (k == mh_end(built_in_functions)) { > + diag_set(OutOfMemory, sizeof(strnode), "malloc", "strnode"); > + return -1; > + } > + return 0; > +} > @@ -2657,6 +2685,67 @@ static struct { > > static struct func_vtab func_sql_builtin_vtab; > > +int > +sql_built_in_functions_cache_init(void) > +{ > + built_in_functions = mh_strnptr_new(); 8. It might make sense to check the result for NULL and call panic() if it is. > + for (uint32_t i = 0; i < nelem(sql_builtins); ++i) { > + const char *name = sql_builtins[i].name; > + uint32_t len = strlen(name); > + uint32_t size = sizeof(struct func_def) + len + 1; > + struct func_def *def = malloc(size); 9. Ditto. > + def->fid = i; > + def->uid = 1; > + def->body = NULL; > + def->comment = NULL; > + def->setuid = true; > + def->is_deterministic = sql_builtins[i].is_deterministic; > + def->is_sandboxed = false; > + def->param_count = sql_builtins[i].param_count; > + def->returns = sql_builtins[i].returns; > + def->aggregate = sql_builtins[i].aggregate; > + def->language = FUNC_LANGUAGE_SQL_BUILTIN; > + def->name_len = len; > + def->exports.sql = sql_builtins[i].export_to_sql; > + func_opts_create(&def->opts); > + memcpy(def->name, name, len + 1); > + > + struct func_sql_builtin *func = malloc(sizeof(*func)); > + if (func == NULL) { > + diag_set(OutOfMemory, sizeof(*func), "malloc", "func"); > + return -1; 10. The result of sql_built_in_functions_cache_init() is never checked. I would suggest to make it void and panic on all errors. > + } > + > + func->base.def = def; > + func->base.vtab = &func_sql_builtin_vtab; > + credentials_create_empty(&func->base.owner_credentials); > + memset(func->base.access, 0, sizeof(func->base.access)); > + > + func->flags = sql_builtins[i].flags; > + func->call = sql_builtins[i].call; > + func->finalize = sql_builtins[i].finalize; > + if (built_in_functions_cache_insert(&func->base) != 0) > + return -1; > + } > + return 0; > +} > + > +void > +sql_built_in_functions_cache_free(void) > +{ > + if (built_in_functions == NULL) > + return; > + for (uint32_t i = 0; i < nelem(sql_builtins); ++i) { > + const char *name = sql_builtins[i].name; > + uint32_t len = strlen(name); > + mh_int_t k = mh_strnptr_find_inp(built_in_functions, name, len); > + if (k == mh_end(built_in_functions)) > + continue; > + mh_strnptr_del(built_in_functions, k, NULL); 11. Perhaps a more 'canonical' way would be like while (mh_size() > 0) { mh_del(mh_first()); ... } Also you didn't free the function objects. > + } > + mh_strnptr_delete(built_in_functions); > +}
prev parent reply other threads:[~2021-08-02 20:12 UTC|newest] Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-29 22:48 Mergen Imeev via Tarantool-patches 2021-08-02 20:12 ` Vladislav Shpilevoy via Tarantool-patches [this message]
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=9acba339-2934-e5ca-b2eb-47282ba24a89@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imeevma@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove SQL built-in functions from _func' \ /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