[tarantool-patches] Re: [PATCH v6 4/5] lua: parameter binding for new execute()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 29 16:19:41 MSK 2018
Thanks for the patch! See 10 comments below.
On 28/12/2018 21:11, imeevma at 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 <imeevma at gmail.com>
> 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 <imeevma at gmail.com>
> 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)
>
More information about the Tarantool-patches
mailing list