From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B92816EC58; Mon, 2 Aug 2021 23:12:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B92816EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627935152; bh=sWSNIhEPlwMO456hrErmva1UHHqENYSmNJLubpnoRoM=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=eyDv9yYfoc0cKApPMQWUYhQpgG0xY9WIDSXRPEkgQt8Qg5IJAG1fXU/cwZQUOw5DE Mi2NYCPHZty3FZ1sHZpBlI/nWbpOaFN37jpT0noYZEqksgRefbQsUsRwiPMGz/+jCe MAfsvPzEQnnEFhh7FRLOIAE+gLYejtBZWwU0lo0Y= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 F24E16EC58 for ; Mon, 2 Aug 2021 23:12:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F24E16EC58 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1mAeIY-0006is-S8; Mon, 02 Aug 2021 23:12:31 +0300 To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: Message-ID: <9acba339-2934-e5ca-b2eb-47282ba24a89@tarantool.org> Date: Mon, 2 Aug 2021 22:12:29 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B20CA14D9DFB46B94A182A05F538085040180F8F364FCCC3CFA965BC0BE2206C2A3203A9F7AA8FC2CBB74D6EF04F25A0F5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD3501B5CE9C42E3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637EEFC5FE85C3736658638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B1A97355965BAA4302680960A06515F3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B50514F1D5C7392F491510513D2235703460 X-C1DE0DAB: 0D63561A33F958A513733E505E66B1CD64054DA8EFD508B8BD2B76C5826849F3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3403182E70AE6895E00CCC980131C9CE153F6C69BCB89C2CC128FF31969DA0D444BF21D589BCF96AE11D7E09C32AA3244C480299C83BB4561D36A82AF1D6FB3AC13C6EB905E3A8056B729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9N286KAyvN610dylh5Z9xA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D682D86ADA68B783CE9C1CE28B822D4873841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove SQL built-in functions from _func X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > #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); > +}