[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag
Kirill Shcherbatov
kshcherbatov at tarantool.org
Wed Mar 20 14:02:53 MSK 2019
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 <kshcherbatov at tarantool.org>
>> 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 "<name> PRIMARY KEY COLLATE <type>",
* 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
More information about the Tarantool-patches
mailing list