From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5A3AC22146 for ; Sat, 29 Dec 2018 08:19:44 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hAx-DDp6Xoy9 for ; Sat, 29 Dec 2018 08:19:44 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DEF0822134 for ; Sat, 29 Dec 2018 08:19:43 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v6 4/5] lua: parameter binding for new execute() References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 29 Dec 2018 16:19:41 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@tarantool.org Thanks for the patch! See 10 comments below. On 28/12/2018 21:11, imeevma@tarantool.org wrote: > Hi! Thank you for review! My answers, diff between versions and > new version below. > > > On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote: >> Thanks for the patch! See 9 comments below. > >> 1. It is not msgpack. > Fixed. > >> 2. In Lua syntax {'name' : value} is incorrect. > Fixed. > >> 3. Just -1. Stack indexes can be negative. > lua_next() throws an assert when I change arg value to -1: > > tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed. It is suspicious. I looked at lua_next source and it does convert negative index to positive with ease. Also, it is used already in cosole.c, space.cc, httpc. and other places. I tried it myself, and -2 value worked for me. Yes, -1 was incorrect, my fault. > >> 4. Please, do not pop the value stored in a luaL_field. Pop 2 >> values at one time below, on line 294. > Fixed. Actually, I found out that my popping policy was quite a > mess in previous version. Now everything will be popped at the > end of the function. > >> 5. As I know, luaL_checklstring never returns NULL. It throws. Please, >> check for string type explicitly and throw normal usage error. > Fixed. > >> 6. As I said above, you've already popped it. > Fixed. > >> 7. Please, check for overflow. > Fixed. > >> 8. To be honest, I feel utterly uncomfortable with luaL_field. IMO, >> this vast structure is too overloaded. Can you please avoid its usage? >> Almost nothing will change as I see. For switch on types you can use >> lua_type(). > After some discussion it was decided to left it as it is now. The > main reason for this is that if we remove luaL_field() than this > function become downgraded version of luaL_field(). > >> 9. Not msgpack. Please, do not blindly copy-paste code. Check out the >> commit more accurately. I will not point out other places. > Fixed. Moved this check to sql.c. > > > Diff between versions: > > commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44 > Author: Mergen Imeev > Date: Thu Dec 27 17:00:51 2018 +0300 > > Temporary: review fixes > > diff --git a/src/box/execute.c b/src/box/execute.c > index 457403b..ade6e6c 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -278,43 +278,43 @@ static inline int > lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > { > struct luaL_field field; > + struct region *region = &fiber()->gc; > char *buf; > lua_rawgeti(L, idx, i + 1); > luaL_tofield(L, luaL_msgpack_default, -1, &field); > bind->pos = i + 1; > if (field.type == MP_MAP) { > /* > - * A named parameter is an MP_MAP with > - * one key - {'name': value}. > - * Report parse error otherwise. > + * A named parameter is a single-row table that > + * contains one value associated with a key. The > + * key must be a string. This key is the name of > + * the parameter, and the value associated with > + * the key is the value of the parameter. 1. Or just change MP_MAP to table, and {'name': value} to {name = value}, and in other places too. But it is up to you. Your comment is ok as well. > */ > if (field.size != 1) { > diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > - "parameter should be {'name': value}"); > + "named parameter should be a single-row "\ > + "table with one key-value pair"); > return -1; > } > /* > - * Get key and value of the only map element to > + * Get key and value of the only table element to > * lua stack. > */ > lua_pushnil(L); > lua_next(L, lua_gettop(L) - 1); > - /* At first we should deal with the value. */ > - luaL_tofield(L, luaL_msgpack_default, -1, &field); > - lua_pop(L, 1); > - /* Now key is on the top of Lua stack. */ > - size_t name_len = 0; > - bind->name = luaL_checklstring(L, -1, &name_len); > - if (bind->name == NULL) { > - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > - "parameter should be {'name': value}"); > + if (lua_type(L, -2) != LUA_TSTRING) { 2. Why so complex? Why not "! lua_isstring()" ? > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ > + "parameter should be a string."); > return -1; > } > + size_t name_len; > + bind->name = lua_tolstring(L, -2, &name_len); > /* > * Name should be saved in allocated memory as it > * will be poped from Lua stack. > */ > - buf = region_alloc(&fiber()->gc, name_len + 1); > + buf = region_alloc(region, name_len + 1); > if (buf == NULL) { > diag_set(OutOfMemory, name_len + 1, "region_alloc", > "buf"); > @@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > memcpy(buf, bind->name, name_len + 1); > bind->name = buf; > bind->name_len = name_len; > - lua_pop(L, 1); > + luaL_tofield(L, luaL_msgpack_default, -1, &field); > } else { > bind->name = NULL; > bind->name_len = 0; > } > switch (field.type) { > case MP_UINT: { > + if (field.ival < 0) { > + diag_set(ClientError, ER_SQL_BIND_VALUE, > + sql_bind_name(bind), "INTEGER"); > + return -1; > + } 3. How is it possible? Lua field reports that it is an unsigned integer. If it is a way of checking for overflow, then please, make it more straightforward. I looked into the source of luaL_tofield and I saw that ival stores uint64_t just casting it to int64_t. So here you should cast it back and check if it is > INT64_MAX. (uint64_t) field.ival > INT64_MAX Also, you do not need braces {} for 'case MP_UINT'. > bind->i64 = field.ival; > bind->type = SQLITE_INTEGER; > bind->bytes = sizeof(bind->i64); > commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c > Author: Mergen Imeev > Date: Fri Dec 21 17:54:16 2018 +0300 > > lua: parameter binding for new execute() > > This patch defines parameters binding for SQL statements executed > through box. > > Part of #3505 > Closes #3401 > > diff --git a/src/box/execute.c b/src/box/execute.c > index 4606239..ade6e6c 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -261,6 +262,196 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind) > return bind_count; > } > > + > +/** > + * Decode a single bind column from Lua stack. > + * > + * @param L Lua stack. > + * @param[out] bind Bind to decode to. > + * @param idx Position of table with bind columns on Lua stack. > + * @param i Ordinal bind number. > + * > + * @retval 0 Success. > + * @retval -1 Memory or client error. > + */ > +static inline int > +lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > +{ > + struct luaL_field field; > + struct region *region = &fiber()->gc; > + char *buf; > + lua_rawgeti(L, idx, i + 1); > + luaL_tofield(L, luaL_msgpack_default, -1, &field); 4. I remembered why I do not like luaL_field. When it meets a non-scalar value like map, it traverses it without necessity. Lets refactor it a bit. diff --git a/src/box/execute.c b/src/box/execute.c index ade6e6c95..3a45fc524 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -281,33 +281,28 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) struct region *region = &fiber()->gc; char *buf; lua_rawgeti(L, idx, i + 1); - luaL_tofield(L, luaL_msgpack_default, -1, &field); bind->pos = i + 1; - if (field.type == MP_MAP) { - /* - * A named parameter is a single-row table that - * contains one value associated with a key. The - * key must be a string. This key is the name of - * the parameter, and the value associated with - * the key is the value of the parameter. - */ - if (field.size != 1) { - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ - "named parameter should be a single-row "\ - "table with one key-value pair"); - return -1; - } + if (lua_istable(L, -1)) { /* * Get key and value of the only table element to * lua stack. */ lua_pushnil(L); - lua_next(L, lua_gettop(L) - 1); + lua_next(L, -2); if (lua_type(L, -2) != LUA_TSTRING) { diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ "parameter should be a string."); return -1; } + /* Check that the table is one-row sized. */ + lua_pushvalue(L, -2); + if (lua_next(L, -4) != 0) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ + "named parameter should be a single-row "\ + "table with one key-value pair"); + return -1; + } + lua_pop(L, 1); size_t name_len; bind->name = lua_tolstring(L, -2, &name_len); /* @@ -323,11 +318,11 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) memcpy(buf, bind->name, name_len + 1); bind->name = buf; bind->name_len = name_len; - luaL_tofield(L, luaL_msgpack_default, -1, &field); } else { bind->name = NULL; bind->name_len = 0; } + luaL_tofield(L, luaL_msgpack_default, -1, &field); switch (field.type) { case MP_UINT: { if (field.ival < 0) { Here I removed common traversal and added an explicit check that a table consists of one key-value pair. Are you ok with these changes? If you are - squash. But I did not test it though. > + bind->pos = i + 1; > + if (field.type == MP_MAP) { > + /* > + * A named parameter is a single-row table that > + * contains one value associated with a key. The > + * key must be a string. This key is the name of > + * the parameter, and the value associated with > + * the key is the value of the parameter. > + */ 5. Are you sure this comment is necessary? It just repeats the error message below. > + if (field.size != 1) { > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > + "named parameter should be a single-row "\ > + "table with one key-value pair"); > + return -1; > + } > + /* > + * Get key and value of the only table element to > + * lua stack. > + */ > + lua_pushnil(L); > + lua_next(L, lua_gettop(L) - 1); > + if (lua_type(L, -2) != LUA_TSTRING) { > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ > + "parameter should be a string."); > + return -1; > + } > + size_t name_len; > + bind->name = lua_tolstring(L, -2, &name_len); > + /* > + * Name should be saved in allocated memory as it > + * will be poped from Lua stack. > + */ > + buf = region_alloc(region, name_len + 1); > + if (buf == NULL) { > + diag_set(OutOfMemory, name_len + 1, "region_alloc", > + "buf"); > + return -1; > + } > + memcpy(buf, bind->name, name_len + 1); > + bind->name = buf; > + bind->name_len = name_len; > + luaL_tofield(L, luaL_msgpack_default, -1, &field); > + } else { > + bind->name = NULL; > + bind->name_len = 0; > + } > + switch (field.type) { > + case MP_UINT: { > + if (field.ival < 0) { > + diag_set(ClientError, ER_SQL_BIND_VALUE, > + sql_bind_name(bind), "INTEGER"); > + return -1; > + } > + bind->i64 = field.ival; > + bind->type = SQLITE_INTEGER; > + bind->bytes = sizeof(bind->i64); > + break; > + } > + case MP_INT: > + bind->i64 = field.ival; > + bind->type = SQLITE_INTEGER; > + bind->bytes = sizeof(bind->i64); > + break; 6. How about this? diff --git a/src/box/execute.c b/src/box/execute.c index ade6e6c95..c1b06b345 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -335,11 +335,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) sql_bind_name(bind), "INTEGER"); return -1; } - bind->i64 = field.ival; - bind->type = SQLITE_INTEGER; - bind->bytes = sizeof(bind->i64); - break; - } + FALLTHROUGH; case MP_INT: bind->i64 = field.ival; bind->type = SQLITE_INTEGER; > + case MP_STR: > + /* > + * Data should be saved in allocated memory as it > + * will be poped from Lua stack. > + */ > + buf = region_alloc(region, field.sval.len + 1); > + if (buf == NULL) { > + diag_set(OutOfMemory, field.sval.len + 1, > + "region_alloc", "buf"); > + return -1; > + } > + memcpy(buf, field.sval.data, field.sval.len + 1); > + bind->s = buf; > + bind->type = SQLITE_TEXT; > + bind->bytes = field.sval.len; > + break; > + case MP_DOUBLE: > + bind->d = field.dval; > + bind->type = SQLITE_FLOAT; > + bind->bytes = sizeof(bind->d); > + break; > + case MP_FLOAT: > + bind->d = field.dval; > + bind->type = SQLITE_FLOAT; > + bind->bytes = sizeof(bind->d); > + break; 7. diff --git a/src/box/execute.c b/src/box/execute.c index ade6e6c95..fb2e5ea29 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -362,10 +362,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) bind->bytes = field.sval.len; break; case MP_DOUBLE: - bind->d = field.dval; - bind->type = SQLITE_FLOAT; - bind->bytes = sizeof(bind->d); - break; case MP_FLOAT: bind->d = field.dval; bind->type = SQLITE_FLOAT; > + case MP_NIL: > + bind->type = SQLITE_NULL; > + bind->bytes = 1; > + break; > + case MP_BOOL: > + /* SQLite doesn't support boolean. Use int instead. */ > + bind->i64 = field.bval ? 1 : 0; > + bind->type = SQLITE_INTEGER; > + bind->bytes = sizeof(bind->i64); > + break; > + case MP_BIN: > + bind->s = mp_decode_bin(&field.sval.data, &bind->bytes); > + bind->type = SQLITE_BLOB; > + break; > + case MP_EXT: > + /* > + * Data should be saved in allocated memory as it > + * will be poped from Lua stack. > + */ > + buf = region_alloc(region, sizeof(field)); > + if (buf == NULL) { > + diag_set(OutOfMemory, sizeof(field), "region_alloc", > + "buf"); > + return -1; > + } > + memcpy(buf, &field, sizeof(field)); 8. Why? What a point of copying struct luaL_field onto region? As I see, MP_EXT is set for unknown cdata like just void *, and the same for unknown userdata. When void * is met, we do not even know its size, nor what a value it stores. When we meet userdata, we can learn its size, but we do not know what a value is stored here and can not decode it. Please, set and error when such thing happens. > + bind->s = buf; > + bind->bytes = sizeof(field); > + bind->type = SQLITE_BLOB; > + break; > + case MP_ARRAY: > + diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY", > + sql_bind_name(bind)); > + return -1; > + case MP_MAP: > + diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP", > + sql_bind_name(bind)); > + return -1; > + default: > + unreachable(); > + } > + lua_pop(L, lua_gettop(L) - idx); > + return 0; > +} 9. Also, I noticed one more problem with luaL_tofield. It can deliberately throw during a number decoding, see at CHECK_NUMBER macro. Moreover, it throws during table/map traversing in lua_field_inspect_table. I once tried to remove these exceptions but failed. Here on a throw region leaks. Also port_sql_dump_lua can throw during pushing data onto the stack and the whole port_sql leaks. Both problems can be solved using lua_cpcall, but please, ask in the server team chat if it is worth doing. As I remember, Kostja says 'let it crash' in such cases, but if a caller will use pcall(box.execute) then it will not crash, but just leak. > diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c > index a55566e..20ba43b 100644 > --- a/src/box/lua/sql.c > +++ b/src/box/lua/sql.c > @@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L) > int bind_count = 0; > size_t length; > struct port port; > + int top = lua_gettop(L); > > - if (lua_type(L, 1) != LUA_TSTRING) > - return luaL_error(L, "Usage: box.execute(sqlstring)"); > + if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING) > + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); > > const char *sql = lua_tolstring(L, 1, &length); > if (sql == NULL) > - return luaL_error(L, "Usage: box.execute(sqlstring)"); > + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); 10. How can it be NULL, if you have checked above that here a string is stored? > + > + if (top == 2) { > + if (! lua_istable(L, 2)) > + return luaL_error(L, "Second argument must be a table"); > + bind_count = lua_sql_bind_list_decode(L, &bind, 2); > + if (bind_count < 0) > + return luaT_error(L); > + } > > if (sql_prepare_and_execute(sql, length, bind, bind_count, &port, > &fiber()->gc) != 0) >