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 1B41524C5E for ; Fri, 25 May 2018 07:53:52 -0400 (EDT) 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 b-h9g7uEddzl for ; Fri, 25 May 2018 07:53:51 -0400 (EDT) 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 41AA624C57 for ; Fri, 25 May 2018 07:53:50 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server References: <2baee418a3799c91c5a8b92ac62e59de3456da0d.1527084287.git.kshcherbatov@tarantool.org> <4221becc-9d9c-852a-ccd1-6ed3618cde9f@tarantool.org> From: Kirill Shcherbatov Message-ID: <6b860615-cd91-414a-3948-32600b69e05f@tarantool.org> Date: Fri, 25 May 2018 14:53:48 +0300 MIME-Version: 1.0 In-Reply-To: <4221becc-9d9c-852a-ccd1-6ed3618cde9f@tarantool.org> Content-Type: text/plain; charset=utf-8 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 Cc: "v.shpilevoy@tarantool.org" > 1. You must not reset OOM error. @@ -1846,7 +1846,9 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, sql_parser_destroy(&parser); if (parser.rc != SQLITE_OK) { - diag_set(IllegalParams, parser.zErrMsg); + /* Error already set with diag. */ + if (parser.rc != SQL_TARANTOOL_ERROR) + diag_set(IllegalParams, parser.zErrMsg); return -1; } @@ -525,8 +525,13 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode, if (def->opts.checks != NULL && sql_checks_resolve_space_def_reference(def->opts.checks, def) != 0) { - tnt_raise(ClientError, errcode, def->name, - box_error_message(box_error_last())); + box_error_t *err = box_error_last(); + if (box_error_code(err) != ENOMEM) { + tnt_raise(ClientError, errcode, def->name, + box_error_message(err)); + } else { + diag_raise(); + } > 2. Garbage diff. > 3. Same. And I do not see this diff on the branch. Have you sent me an out of > date patch? Yep, a little different. > 4. I do not like 'expr_str' name. Obviously it is string, a user can not > use another type. Sounds like "much of a muchness". Lets do like > defaults: just 'expr > 6. It is not index and not ER_WRONG_INDEX_OPTIONS. Here you parse space > options. You must do like opts_parse_key - take error code and field > number as arguments. And do diag_set(ClientError, errcode, fieldno, errmessage); > 7. When you use strncmp, I can do this: > I have used 'exp' instead of 'expr_str'. Strncmp is a useless function as I > think, because it can be broken easy. Better use > a_len == b_len && memcmp(a, b) == 0.@@ -297,19 +302,23 @@ checks_array_decode(const char **str, uint32_t len, char *opt) uint32_t map_size = mp_decode_map(map); for (uint32_t j = 0; j < map_size; j++) { if (mp_typeof(**map) != MP_STR) { - diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0, + diag_set(ClientError, errcode, 0, "key must be a string"); goto error; } uint32_t key_len; const char *key = mp_decode_str(map, &key_len); - if (strncmp(key, "expr_str", key_len) == 0) { + if (key_len == 4 && memcmp(key, "expr", key_len) == 0) { expr_str = mp_decode_str(map, &expr_str_len); - } else if (strncmp(key, "name", key_len) == 0) { + } else if (key_len == 4 && + memcmp(key, "name", key_len) == 0) { expr_name = mp_decode_str(map, &expr_name_len); } else { - diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0, - "invalid MsgPack"); + char errmsg[DIAG_ERRMSG_MAX]; + snprintf(errmsg, sizeof(errmsg), + "invalid MsgPack map field '%.*s'", + key_len, key); + diag_set(ClientError, errcode, field_no, errmsg); goto error; } } > 5. I have refactored this and the previous hunks (fixed tuple_dictionary leak in > space_def_new). But it is bad still because of code duplicating. Looks like it is > time to create a function like 'space_def_dup_opts(space_def a, opts b)', > that copies `b` into `a->opts`. This function duplicates sql, checks and > updates space_def references. +/** + * Initialize def->opts with opts duplicate. + * @param def Def to initialize. + * @param opts Opts to duplicate. + * @retval 0 on success. + * @retval not 0 on error. + */ +static int +space_def_dup_opts(struct space_def *def, const struct space_opts *opts) +{ + def->opts = *opts; + if (opts->sql != NULL) { + def->opts.sql = strdup(opts->sql); + if (def->opts.sql == NULL) { + diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup", + "def->opts.sql"); + return -1; + } + } + if (opts->checks != NULL) { + def->opts.checks = sql_expr_list_dup(sql_get(), opts->checks, 0); + if (def->opts.checks == NULL) { + diag_set(OutOfMemory, 0, "sql_expr_list_dup", + "def->opts.checks"); + return -1; + } + sql_checks_update_space_def_reference(def->opts.checks, def); + } + return 0; +} + @@ -101,15 +135,7 @@ space_def_dup(const struct space_def *src) return NULL; } memcpy(ret, src, size); - if (src->opts.sql != NULL) { - ret->opts.sql = strdup(src->opts.sql); - if (ret->opts.sql == NULL) { - diag_set(OutOfMemory, strlen(src->opts.sql) + 1, - "strdup", "ret->opts.sql"); - free(ret); - return NULL; - } - } + memset(&ret->opts, 0, sizeof(ret->opts)); @@ -139,16 +165,9 @@ space_def_dup(const struct space_def *src) } } tuple_dictionary_ref(ret->dict); - if (src->opts.checks != NULL) { - ret->opts.checks = - sql_expr_list_dup(sql_get(), src->opts.checks, 0); - if (ret->opts.checks == NULL) { - diag_set(OutOfMemory, 0, "sql_expr_list_dup", - "ret->opts.checks"); - space_def_delete(ret); - return NULL; - } - sql_checks_update_space_def_reference(ret->opts.checks, ret); + if (space_def_dup_opts(ret, &src->opts) != 0) { + space_def_delete(ret); + return NULL; } @@ -183,16 +202,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, def->name[name_len] = 0; memcpy(def->engine_name, engine_name, engine_len); def->engine_name[engine_len] = 0; - def->opts = *opts; - if (opts->sql != NULL) { - def->opts.sql = strdup(opts->sql); - if (def->opts.sql == NULL) { - diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup", - "def->opts.sql"); - free(def); - return NULL; - } - } + @@ -234,15 +244,9 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, } } } - if (opts->checks != NULL) { - def->opts.checks = sql_expr_list_dup(sql_get(), opts->checks, 0); - if (def->opts.checks == NULL) { - diag_set(OutOfMemory, 0, "sql_expr_list_dup", - "def->opts.checks"); - space_def_delete(def); - return NULL; - } - sql_checks_update_space_def_reference(def->opts.checks, def); + if (space_def_dup_opts(def, opts) != 0) { + space_def_delete(def); + return NULL; } return def; > box.cfg{} > opts = {checks = {{exp = 'X<5'}}} > format = {{name = 'X', type = 'unsigned'}} > t = {513, 1, 'test', 'memtx', 0, opts, format} > box.space._space:insert(t) > > >> + } >> + sql_check_list_item_init(checks, i, expr_name, >> + expr_name_len, expr_str, >> + expr_str_len); >> + } >> + *(void **)opt = checks; >> + return 0; >> +error: >> + sql_expr_list_delete(db, checks); >> + return -1; >> +} >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 0845e65..d62e14c 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -1761,3 +1797,72 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) >> pTable->def->opts.temporary = false; >> return 0; >> } >> + >> +int >> +sql_check_list_item_init(struct ExprList *expr_list, int idx, >> + const char *expr_name, uint32_t expr_name_len, >> + const char *expr_str, uint32_t expr_str_len) >> +{ >> + assert(idx < expr_list->nExpr); >> + struct ExprList_item *item = &expr_list->a[idx]; >> + memset(item, 0, sizeof(struct ExprList_item)); >> + if (expr_name != NULL) { >> + item->zName = sqlite3DbStrNDup(db, expr_name, expr_name_len); > > 8. Please, use strndup. And set diag on OOM. First, sqlite3DbStrNDup is not a problem in sql.c. As ExprList is not only about checks and sql_expr_list_delete destructor releases such allocations with sqlite3DbFree I believe that such changes shouldn't going with this patch. > 9. Why can not you replace pTab in struct SrcList_item with space_def? This structure is required in ctx where pIndex is required. Look for sqlite3FindInIndex; sqlite3PrimaryKeyIndex in sqlite3ExprCodeIN and so on. - if (item->zName == NULL) + if (item->zName == NULL) { + diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup", + "item->zName"); return -1; + } > 10. deleteTable leaks with the same reason: you delete space_def only when > opts.is_temporary flag is set. Including checks, that are on malloc. > 11. Why do you space->def->opts.checks here? I scanned sql_view_column_names, > and looks like it does not require compiled check. It needs only check names > and check span. Table.def has both, it is not? >> + /* >> + * As rebuild creates a new ExpList tree and old_def >> + * is allocated on region release old tree manually. >> + */ >> + struct ExprList *old_checks = p->def->opts.checks; >> if (sql_table_def_rebuild(db, p) != 0) >> return; >> + /* Get server checks. */ >> + ExprList *checks = space_checks_expr_list(table->def->id); > We don't replicate checks in SQL. At the end of sqlite3EndTable checks are released and the only checks collection present on the server side. I've wrote comments about this in sqlite3EndTable: /* * As rebuild creates a new ExpList tree and old_def * allocated on region release old tree manually. */ /* * Checks are useless for now as all operations process with * the server checks instance. Remove to do not consume extra memory, * don't require make a copy on space_def_dup and to improve * debuggability. */ > > 12. Please, write tests on checks insertion from Lua. I know such checks will > not work, but at least you should do sanity test. You should test, that a user > can insert a check with no name, that a check can be compiled even if no SQL > Table, that a check expr can not be number or bool and the same about name. > In the same tests you can check errors on non-existing column names. Fixed error handling: diff --git a/src/box/space_def.c b/src/box/space_def.c index d4c0c90..dda9d95 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -285,6 +285,7 @@ static int checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, uint32_t field_no) { + char errmsg[DIAG_ERRMSG_MAX]; struct ExprList *checks = NULL; const char **map = str; struct sqlite3 *db = sql_get(); @@ -314,7 +315,6 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, memcmp(key, "name", key_len) == 0) { expr_name = mp_decode_str(map, &expr_name_len); } else { - char errmsg[DIAG_ERRMSG_MAX]; snprintf(errmsg, sizeof(errmsg), "invalid MsgPack map field '%.*s'", key_len, key); @@ -322,8 +322,17 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode, goto error; } } - sql_check_list_item_init(checks, i, expr_name, expr_name_len, - expr_str, expr_str_len); + if (sql_check_list_item_init(checks, i, expr_name, expr_name_len, + expr_str, expr_str_len) != 0) { + box_error_t *err = box_error_last(); + if (box_error_code(err) != ENOMEM) { + snprintf(errmsg, sizeof(errmsg), + "invalid expression specified"); + diag_set(ClientError, errcode, field_no, + errmsg); + goto error; + } + } } *(struct ExprList **)opt = checks; return 0; diff --git a/src/box/sql.c b/src/box/sql.c index f129f0b..9abda39 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1797,8 +1797,11 @@ sql_check_list_item_init(struct ExprList *expr_list, int idx, memset(item, 0, sizeof(*item)); if (expr_name != NULL) { item->zName = sqlite3DbStrNDup(db, expr_name, expr_name_len); - if (item->zName == NULL) + if (item->zName == NULL) { + diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup", + "item->zName"); return -1; + } } if (expr_str != NULL && sql_expr_compile(db, expr_str, expr_str_len, &item->pExpr) != 0) { And some tests: +++ b/test/sql/checks.test.lua @@ -0,0 +1,21 @@ +env = require('test_run') +test_run = env.new() +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:: '") + +-- +-- gh-3272: Move SQL CHECK into server +-- + +-- invalid expression +opts = {checks = {{expr = 'X><5'}}} +format = {{name = 'X', type = 'unsigned'}} +t = {513, 1, 'test', 'memtx', 0, opts, format} +s = box.space._space:insert(t) + +opts = {checks = {{expr = 'X>5'}}} +format = {{name = 'X', type = 'unsigned'}} +t = {513, 1, 'test', 'memtx', 0, opts, format} +s = box.space._space:insert(t) +box.space._space:delete(513) + +test_run:cmd("clear filter")