[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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@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