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 13D682A50C for ; Wed, 20 Mar 2019 07:02:56 -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 mD99ACrnZH30 for ; Wed, 20 Mar 2019 07:02:55 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 8B9C02A509 for ; Wed, 20 Mar 2019 07:02:55 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag References: <97dde83dd0ad392cfe47a315ce7ad931cb750a4f.1551265819.git.kshcherbatov@tarantool.org> <4bb2ebb3-dfc7-b95c-cd76-6992baa92128@tarantool.org> <9e87fae4-138e-f727-f9fa-8904687d2acc@tarantool.org> From: Kirill Shcherbatov Message-ID: <5ec2891c-fd41-581c-42ee-1259260b08df@tarantool.org> Date: Wed, 20 Mar 2019 14:02:53 +0300 MIME-Version: 1.0 In-Reply-To: <9e87fae4-138e-f727-f9fa-8904687d2acc@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, Vladislav Shpilevoy On 18.03.2019 22:33, Vladislav Shpilevoy wrote: > The diff in your email has nothing to do with sql_name_from_token. > You've not sent a new version of that patch, so I did it below > with my 2 comments inlined. > >> commit b052b1b0a43159ac79320d5dca43a201fffa6ab9 >> Author: Kirill Shcherbatov >> Date: Wed Feb 13 15:15:22 2019 +0300 >> >> sql: rework sqlNameFromToken to set diag >> >> Refactored sqlNameFromToken routine as sql_name_from_token and >> reworked it to use diag_set in case of memory allocation error. >> This change is necessary because the sql_name_from_token body has >> a sqlNameFromToken call that will be changed in subsequent >> patches. > > 1. Now, reread that paragraph and fix what is wrong. You do not call > sqlNameFromToken from sql_name_from_token. sql: rework sqlNameFromToken to set diag Refactored sqlNameFromToken routine as sql_name_from_token and reworked it to use diag_set in case of memory allocation error. This change is necessary because the sql_name_from_token body has a sqlNormalizeName call that will be changed in subsequent patches. Part of #3931 >> - zName = sqlNameFromToken(pParse->db, pName); >> - if (zName && pWith) { >> + char *name = sql_name_from_token(db, pName); >> + if (name == NULL) { >> + sql_parser_error(pParse); >> + return NULL; > > 2. Leak. sqlWithAdd should delete some structures in a case of OOM. > Lines 3035 - 3038. Also, that function does not return NULL on an > error - it should return the old value. Otherwise you have a second > leak in parse.y:1494. diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 88383b7c5..40bab170d 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3009,7 +3009,7 @@ sqlWithAdd(Parse * pParse, /* Parsing context */ char *name = sql_name_from_token(db, pName); if (name == NULL) { sql_parser_error(pParse); - return NULL; + goto oom_error; } if (pWith != NULL) { int i; @@ -3032,6 +3032,7 @@ sqlWithAdd(Parse * pParse, /* Parsing context */ assert((pNew != NULL && name != NULL) || db->mallocFailed); if (db->mallocFailed) { +oom_error: sql_expr_list_delete(db, pArglist); sql_select_delete(db, pQuery); sqlDbFree(db, name); ====================================================== Refactored sqlNameFromToken routine as sql_name_from_token and reworked it to use diag_set in case of memory allocation error. This change is necessary because the sql_name_from_token body has a sqlNormalizeName call that will be changed in subsequent patches. Part of #3931 --- src/box/sql/alter.c | 4 +- src/box/sql/analyze.c | 44 +++++++----- src/box/sql/build.c | 163 +++++++++++++++++++++++++----------------- src/box/sql/pragma.c | 26 +++++-- src/box/sql/sqlInt.h | 21 +++++- src/box/sql/trigger.c | 4 +- 6 files changed, 165 insertions(+), 97 deletions(-) diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index bd9b034c4..bf4f03a17 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -43,9 +43,9 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab, { assert(src_tab->nSrc == 1); struct sql *db = parse->db; - char *new_name = sqlNameFromToken(db, new_name_tk); + char *new_name = sql_name_from_token(db, new_name_tk); if (new_name == NULL) - goto exit_rename_table; + goto tnt_error; /* Check that new name isn't occupied by another table. */ if (space_by_name(new_name) != NULL) { diag_set(ClientError, ER_SPACE_EXISTS, new_name); diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 6ea598c13..86b13ffad 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -1110,34 +1110,40 @@ vdbe_emit_analyze_table(struct Parse *parse, struct space *space) void sqlAnalyze(Parse * pParse, Token * pName) { - sql *db = pParse->db; + char *name = NULL; + struct sql *db = pParse->db; + struct Vdbe *v = sqlGetVdbe(pParse); + if (v == NULL) + return; if (pName == NULL) { /* Form 1: Analyze everything */ sql_analyze_database(pParse); } else { /* Form 2: Analyze table named */ - char *z = sqlNameFromToken(db, pName); - if (z != NULL) { - struct space *sp = space_by_name(z); - if (sp != NULL) { - if (sp->def->opts.is_view) { - diag_set(ClientError, - ER_SQL_ANALYZE_ARGUMENT, - sp->def->name); - pParse->is_aborted = true; - } else { - vdbe_emit_analyze_table(pParse, sp); - } - } else { - diag_set(ClientError, ER_NO_SUCH_SPACE, z); + char *z = sql_name_from_token(db, pName); + if (z == NULL) { + pParse->is_aborted = true; + goto cleanup; + } + struct space *sp = space_by_name(z); + if (sp != NULL) { + if (sp->def->opts.is_view) { + diag_set(ClientError, ER_SQL_ANALYZE_ARGUMENT, + sp->def->name); pParse->is_aborted = true; + } else { + vdbe_emit_analyze_table(pParse, sp); } - sqlDbFree(db, z); + } else { + diag_set(ClientError, ER_NO_SUCH_SPACE, z); + pParse->is_aborted = true; } + sqlDbFree(db, z); } - Vdbe *v = sqlGetVdbe(pParse); - if (v != NULL) - sqlVdbeAddOp0(v, OP_Expire); + sqlVdbeAddOp0(v, OP_Expire); +cleanup: + sqlDbFree(db, name); + return; } ssize_t diff --git a/src/box/sql/build.c b/src/box/sql/build.c index dae582d1f..f52cfd7dd 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -229,30 +229,18 @@ sql_space_column_is_in_pk(struct space *space, uint32_t column) return false; } -/* - * Given a token, return a string that consists of the text of that - * token. Space to hold the returned string - * is obtained from sqlMalloc() and must be freed by the calling - * function. - * - * Any quotation marks (ex: "name", 'name', [name], or `name`) that - * surround the body of the token are removed. - * - * Tokens are often just pointers into the original SQL text and so - * are not \000 terminated and are not persistent. The returned string - * is \000 terminated and is persistent. - */ char * -sqlNameFromToken(sql * db, Token * pName) +sql_name_from_token(struct sql *db, struct Token *name_token) { - char *zName; - if (pName) { - zName = sqlDbStrNDup(db, (char *)pName->z, pName->n); - sqlNormalizeName(zName); - } else { - zName = 0; + assert(name_token != NULL && name_token->z != NULL); + char *name = sqlDbStrNDup(db, name_token->z, name_token->n); + if (name == NULL) { + diag_set(OutOfMemory, name_token->n + 1, "sqlDbStrNDup", + "name"); + return NULL; } - return zName; + sqlNormalizeName(name); + return name; } /* @@ -329,11 +317,13 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr) goto cleanup; sqlVdbeCountChanges(v); - zName = sqlNameFromToken(db, pName); + zName = sql_name_from_token(db, pName); + if (zName == NULL) { + pParse->is_aborted = true; + goto cleanup; + } pParse->sNameToken = *pName; - if (zName == 0) - return; if (sqlCheckIdentifierName(pParse, zName) != SQL_OK) goto cleanup; @@ -700,11 +690,13 @@ sqlAddCollateType(Parse * pParse, Token * pToken) struct space *space = pParse->new_space; uint32_t i = space->def->field_count - 1; sql *db = pParse->db; - char *zColl = sqlNameFromToken(db, pToken); - if (!zColl) + char *coll_name = sql_name_from_token(db, pToken); + if (coll_name == NULL) { + pParse->is_aborted = true; return; + } uint32_t *coll_id = &space->def->fields[i].coll_id; - if (sql_get_coll_seq(pParse, zColl, coll_id) != NULL) { + if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) { /* If the column is declared as " PRIMARY KEY COLLATE ", * then an index may have been created on this column before the * collation type was added. Correct this if it is the case. @@ -718,7 +710,7 @@ sqlAddCollateType(Parse * pParse, Token * pToken) } } } - sqlDbFree(db, zColl); + sqlDbFree(db, coll_name); } struct coll * @@ -1733,10 +1725,9 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, memset(fk_parse, 0, sizeof(*fk_parse)); rlist_add_entry(&parse_context->new_fk_constraint, fk_parse, link); } - assert(parent != NULL); - parent_name = sqlNameFromToken(db, parent); + parent_name = sql_name_from_token(db, parent); if (parent_name == NULL) - goto exit_create_fk; + goto tnt_error; /* * Within ALTER TABLE ADD CONSTRAINT FK also can be * self-referenced, but in this case parent (which is @@ -1769,15 +1760,19 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, sqlMPrintf(db, "FK_CONSTRAINT_%d_%s", ++parse_context->fk_constraint_count, space->def->name); + if (constraint_name == NULL) + goto exit_create_fk; } else { struct Token *cnstr_nm = &parse_context->constraintName; - constraint_name = sqlNameFromToken(db, cnstr_nm); + constraint_name = sql_name_from_token(db, cnstr_nm); + if (constraint_name == NULL) + goto tnt_error; } } else { - constraint_name = sqlNameFromToken(db, constraint); + constraint_name = sql_name_from_token(db, constraint); + if (constraint_name == NULL) + goto tnt_error; } - if (constraint_name == NULL) - goto exit_create_fk; const char *error_msg = "number of columns in foreign key does not " "match the number of columns in the primary " "index of referenced table"; @@ -1908,11 +1903,14 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, parse_context->is_aborted = true; return; } - char *constraint_name = sqlNameFromToken(parse_context->db, - constraint); - if (constraint_name != NULL) - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, - child->def->id); + char *constraint_name = + sql_name_from_token(parse_context->db, constraint); + if (constraint_name == NULL) { + parse_context->is_aborted = true; + return; + } + vdbe_emit_fk_constraint_drop(parse_context, constraint_name, + child->def->id); /* * We account changes to row count only if drop of * foreign keys take place in a separate @@ -2172,9 +2170,11 @@ sql_create_index(struct Parse *parse, struct Token *token, */ if (token != NULL) { assert(token->z != NULL); - name = sqlNameFromToken(db, token); - if (name == NULL) + name = sql_name_from_token(db, token); + if (name == NULL) { + parse->is_aborted = true; goto exit_create_index; + } if (sql_space_index_by_name(space, name) != NULL) { if (!if_not_exist) { diag_set(ClientError, ER_INDEX_EXISTS_IN_SPACE, @@ -2185,10 +2185,15 @@ sql_create_index(struct Parse *parse, struct Token *token, } } else { char *constraint_name = NULL; - if (parse->constraintName.z != NULL) + if (parse->constraintName.z != NULL) { constraint_name = - sqlNameFromToken(db, - &parse->constraintName); + sql_name_from_token(db, + &parse->constraintName); + if (constraint_name == NULL) { + parse->is_aborted = true; + goto exit_create_index; + } + } /* * This naming is temporary. Now it's not @@ -2426,8 +2431,9 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list, /* Never called with prior errors. */ assert(!parse_context->is_aborted); assert(table_token != NULL); - const char *table_name = sqlNameFromToken(db, table_token); - if (db->mallocFailed) { + const char *table_name = sql_name_from_token(db, table_token); + if (table_name == NULL) { + parse_context->is_aborted = true; goto exit_drop_index; } sqlVdbeCountChanges(v); @@ -2531,7 +2537,7 @@ sql_id_list_append(struct sql *db, struct IdList *list, &list->nId, &i); if (i < 0) goto error; - list->a[i].zName = sqlNameFromToken(db, name_token); + list->a[i].zName = sql_name_from_token(db, name_token); if (list->a[i].zName == NULL) goto error; return list; @@ -2646,7 +2652,13 @@ sql_src_list_append(struct sql *db, struct SrcList *list, list = new_list; } struct SrcList_item *item = &list->a[list->nSrc - 1]; - item->zName = sqlNameFromToken(db, name_token); + if (name_token != NULL) { + item->zName = sql_name_from_token(db, name_token); + if (item->zName == NULL) { + sqlSrcListDelete(db, list); + return NULL; + } + } return list; } @@ -2747,8 +2759,12 @@ sqlSrcListAppendFromTerm(Parse * pParse, /* Parsing context */ assert(p->nSrc != 0); pItem = &p->a[p->nSrc - 1]; assert(pAlias != 0); - if (pAlias->n) { - pItem->zAlias = sqlNameFromToken(db, pAlias); + if (pAlias->n != 0) { + pItem->zAlias = sql_name_from_token(db, pAlias); + if (pItem->zAlias == NULL) { + pParse->is_aborted = true; + goto append_from_error; + } } pItem->pSelect = pSubquery; pItem->pOn = pOn; @@ -2782,8 +2798,15 @@ sqlSrcListIndexedBy(Parse * pParse, SrcList * p, Token * pIndexedBy) */ pItem->fg.notIndexed = 1; } else { - pItem->u1.zIndexedBy = - sqlNameFromToken(pParse->db, pIndexedBy); + if (pIndexedBy->z != NULL) { + pItem->u1.zIndexedBy = + sql_name_from_token(pParse->db, + pIndexedBy); + if (pItem->u1.zIndexedBy == NULL) { + pParse->is_aborted = true; + return; + } + } pItem->fg.isIndexedBy = (pItem->u1.zIndexedBy != 0); } } @@ -2869,11 +2892,12 @@ sql_transaction_rollback(Parse *pParse) void sqlSavepoint(Parse * pParse, int op, Token * pName) { - char *zName = sqlNameFromToken(pParse->db, pName); + struct sql *db = pParse->db; + char *zName = sql_name_from_token(db, pName); if (zName) { Vdbe *v = sqlGetVdbe(pParse); if (!v) { - sqlDbFree(pParse->db, zName); + sqlDbFree(db, zName); return; } if (op == SAVEPOINT_BEGIN && @@ -2883,6 +2907,8 @@ sqlSavepoint(Parse * pParse, int op, Token * pName) return; } sqlVdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, P4_DYNAMIC); + } else { + pParse->is_aborted = true; } } @@ -2958,19 +2984,23 @@ sqlWithAdd(Parse * pParse, /* Parsing context */ { sql *db = pParse->db; With *pNew; - char *zName; - /* Check that the CTE name is unique within this WITH clause. If - * not, store an error in the Parse structure. + /* + * Check that the CTE name is unique within this WITH + * clause. If not, store an error in the Parse structure. */ - zName = sqlNameFromToken(pParse->db, pName); - if (zName && pWith) { + char *name = sql_name_from_token(db, pName); + if (name == NULL) { + pParse->is_aborted = true; + goto oom_error; + } + if (pWith != NULL) { int i; for (i = 0; i < pWith->nCte; i++) { - if (strcmp(zName, pWith->a[i].zName) == 0) { + if (strcmp(name, pWith->a[i].zName) == 0) { sqlErrorMsg(pParse, - "duplicate WITH table name: %s", - zName); + "duplicate WITH table name: %s", + name); } } } @@ -2982,17 +3012,18 @@ sqlWithAdd(Parse * pParse, /* Parsing context */ } else { pNew = sqlDbMallocZero(db, sizeof(*pWith)); } - assert((pNew != 0 && zName != 0) || db->mallocFailed); + assert((pNew != NULL && name != NULL) || db->mallocFailed); if (db->mallocFailed) { +oom_error: sql_expr_list_delete(db, pArglist); sql_select_delete(db, pQuery); - sqlDbFree(db, zName); + sqlDbFree(db, name); pNew = pWith; } else { pNew->a[pNew->nCte].pSelect = pQuery; pNew->a[pNew->nCte].pCols = pArglist; - pNew->a[pNew->nCte].zName = zName; + pNew->a[pNew->nCte].zName = name; pNew->a[pNew->nCte].zCteErr = 0; pNew->nCte++; } diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 2b9c9b441..53524b617 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -423,19 +423,31 @@ sqlPragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ sqlVdbeRunOnlyOnce(v); pParse->nMem = 2; - zLeft = sqlNameFromToken(db, pId); - if (!zLeft) { + if (pId == NULL) { vdbe_emit_pragma_status(pParse); return; } - + zLeft = sql_name_from_token(db, pId); + if (zLeft == NULL) { + pParse->is_aborted = true; + goto pragma_out; + } if (minusFlag) { zRight = sqlMPrintf(db, "-%T", pValue); - } else { - zRight = sqlNameFromToken(db, pValue); + } else if (pValue != NULL) { + zRight = sql_name_from_token(db, pValue); + if (zRight == NULL) { + pParse->is_aborted = true; + goto pragma_out; + } + } + if (pValue2 != NULL) { + zTable = sql_name_from_token(db, pValue2); + if (zTable == NULL) { + pParse->is_aborted = true; + goto pragma_out; + } } - zTable = sqlNameFromToken(db, pValue2); - /* Locate the pragma in the lookup table */ pPragma = pragmaLocate(zLeft); if (pPragma == 0) { diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 8f56f3e63..3aedde4cc 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -3666,7 +3666,26 @@ int sqlExprCodeExprList(Parse *, ExprList *, int, int, u8); void sqlExprIfTrue(Parse *, Expr *, int, int); void sqlExprIfFalse(Parse *, Expr *, int, int); -char *sqlNameFromToken(sql *, Token *); +/** + * Given a token, return a string that consists of the text of + * that token. Space to hold the returned string is obtained + * from sqlMalloc() and must be freed by the calling function. + * + * Any quotation marks (ex: "name", 'name', [name], or `name`) + * that surround the body of the token are removed. + * + * Tokens are often just pointers into the original SQL text and + * so are not \000 terminated and are not persistent. The returned + * string is \000 terminated and is persistent. + * + * @param db The database connection. + * @param name_token The source token with text. + * @retval Not NULL String pointer on success. + * @retval NULL Otherwise. Diag message is set. + */ +char * +sql_name_from_token(struct sql *db, struct Token *name_token); + int sqlExprCompare(Expr *, Expr *, int); int sqlExprListCompare(ExprList *, ExprList *, int); int sqlExprImpliesExpr(Expr *, Expr *, int); diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index b1f5033c4..dc30c5f2c 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -82,9 +82,9 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm, goto trigger_cleanup; assert(table->nSrc == 1); - trigger_name = sqlNameFromToken(db, name); + trigger_name = sql_name_from_token(db, name); if (trigger_name == NULL) - goto trigger_cleanup; + goto set_tarantool_error_and_cleanup; if (sqlCheckIdentifierName(parse, trigger_name) != SQL_OK) goto trigger_cleanup; -- 2.21.0