[tarantool-patches] Re: [PATCH v1 2/3] sql: ban sql functions coinciding with builtins

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

The code of the original sqlFindFunction function does not match
our codestyle and is difficult to maintain and extend. This patch
does not make any functional changes in Tarantool, only optimizes
the sqlFindFunction function,

Needed #3691
---
 src/box/lua/lua_sql.c  |   8 +-
 src/box/sql/callback.c | 179 ++++++++++++++++++++---------------------
 src/box/sql/expr.c     |  16 ++--
 src/box/sql/func.c     |   6 +-
 src/box/sql/main.c     |  10 +--
 src/box/sql/resolve.c  |   6 +-
 src/box/sql/sqlInt.h   |  22 ++++-
 src/box/sql/vdbemem.c  |   2 +-
 8 files changed, 135 insertions(+), 114 deletions(-)

diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
index 36b75ff08..561353520 100644
--- a/src/box/lua/lua_sql.c
+++ b/src/box/lua/lua_sql.c
@@ -195,7 +195,11 @@ lbox_sql_create_function(struct lua_State *L)
 					   is_deterministic ? SQL_DETERMINISTIC : 0,
 					   func_info, lua_sql_call, NULL, NULL,
 					   lua_sql_destroy);
-	if (rc != 0)
-		return luaL_error(L, sqlErrStr(rc));
+	if (rc != 0) {
+		const char *err = rc == SQL_TARANTOOL_ERROR ?
+				  box_error_message(box_error_last()) :
+				  sqlErrStr(rc);
+		return luaL_error(L, err);
+	}
 	return 0;
 }
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 49197532e..54d59bbd3 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -131,6 +131,19 @@ functionSearch(int h,		/* Hash of the name */
 	return 0;
 }
 
+/**
+ * Cache function is used to organise sqlBuiltinFunctions table.
+ * @param func_name The name of builtin function.
+ * @param func_name_len The length of the @a name.
+ * @retval Hash value is calculated for given name.
+ */
+static int
+sql_builtin_func_name_hash(const char *func_name, uint32_t func_name_len)
+{
+	return (sqlUpperToLower[(u8) func_name[0]] +
+		func_name_len) % SQL_FUNC_HASH_SZ;
+}
+
 /*
  * Insert a new FuncDef into a FuncDefHash hash table.
  */
@@ -144,9 +157,7 @@ sqlInsertBuiltinFuncs(FuncDef * aDef,	/* List of global functions to be inserted
 		FuncDef *pOther;
 		const char *zName = aDef[i].zName;
 		int nName = sqlStrlen30(zName);
-		int h =
-		    (sqlUpperToLower[(u8) zName[0]] +
-		     nName) % SQL_FUNC_HASH_SZ;
+		int h = sql_builtin_func_name_hash(zName, nName);
 		pOther = functionSearch(h, zName);
 		if (pOther) {
 			assert(pOther != &aDef[i] && pOther->pNext != &aDef[i]);
@@ -160,110 +171,98 @@ sqlInsertBuiltinFuncs(FuncDef * aDef,	/* List of global functions to be inserted
 	}
 }
 
-/*
- * Locate a user function given a name, a number of arguments and a flag
- * indicating whether the function prefers UTF-16 over UTF-8.  Return a
- * pointer to the FuncDef structure that defines that function, or return
- * NULL if the function does not exist.
- *
- * If the createFlag argument is true, then a new (blank) FuncDef
- * structure is created and liked into the "db" structure if a
- * no matching function previously existed.
- *
- * If nArg is -2, then the first valid function found is returned.  A
- * function is valid if xSFunc is non-zero.  The nArg==(-2)
- * case is used to see if zName is a valid function name for some number
- * of arguments.  If nArg is -2, then createFlag must be 0.
- *
- * If createFlag is false, then a function with the required name and
- * number of arguments may be returned even if the eTextRep flag does not
- * match that requested.
- */
-FuncDef *
-sqlFindFunction(sql * db,	/* An open database */
-		    const char *zName,	/* Name of the function.  zero-terminated */
-		    int nArg,	/* Number of arguments.  -1 means any number */
-		    u8 createFlag	/* Create new entry if true and does not otherwise exist */
-    )
+struct FuncDef *
+sql_find_function(struct sql *db, const char *func_name, int arg_count,
+		  bool is_create)
 {
-	FuncDef *p;		/* Iterator variable */
-	FuncDef *pBest = 0;	/* Best match found so far */
-	int bestScore = 0;	/* Score of best match */
-	int h;			/* Hash value */
-	int nName;		/* Length of the name */
-	struct session *user_session = current_session();
+	assert(arg_count >= -2);
+	assert(arg_count >= -1 || !is_create);
+	uint32_t func_name_len = strlen(func_name);
+	/*
+	 * The 'score' of the best match is calculated with
+	 * matchQuality estimator.
+	 */
+	int func_score = 0;
+	/* Best match found so far. */
+	struct FuncDef *func = NULL;
 
-	assert(nArg >= (-2));
-	assert(nArg >= (-1) || createFlag == 0);
-	nName = sqlStrlen30(zName);
+	struct session *user_session = current_session();
+	bool is_builtin = (user_session->sql_flags & SQL_PreferBuiltin) != 0;
+	if (is_builtin)
+		goto lookup_for_builtin;
 
-	/* First search for a match amongst the application-defined functions.
+	/*
+	 * First search for a match amongst the
+	 * application-defined functions.
 	 */
-	p = (FuncDef *) sqlHashFind(&db->aFunc, zName);
-	while (p) {
-		int score = matchQuality(p, nArg);
-		if (score > bestScore) {
-			pBest = p;
-			bestScore = score;
+	for (struct FuncDef *p = sqlHashFind(&db->aFunc, func_name);
+	      p != NULL; p = p->pNext) {
+		int score = matchQuality(p, arg_count);
+		if (score > func_score) {
+			func = p;
+			func_score = score;
 		}
-		p = p->pNext;
 	}
 
-	/* If no match is found, search the built-in functions.
+	/**
+	 * If no match is found, search the built-in functions.
 	 *
-	 * If the SQL_PreferBuiltin flag is set, then search the built-in
-	 * functions even if a prior app-defined function was found.  And give
-	 * priority to built-in functions.
+	 * If the SQL_PreferBuiltin flag is set, then search the
+	 * built-in functions even if a prior app-defined function
+	 * was found.  And give priority to built-in functions.
 	 *
-	 * Except, if createFlag is true, that means that we are trying to
-	 * install a new function.  Whatever FuncDef structure is returned it will
-	 * have fields overwritten with new information appropriate for the
-	 * new function.  But the FuncDefs for built-in functions are read-only.
-	 * So we must not search for built-ins when creating a new function.
+	 * Except, if is_create is true, that means that we are
+	 * trying to install a new function. Whatever FuncDef
+	 * structure is returned it will have fields overwritten
+	 * with new information appropriate for the new function.
+	 * But the FuncDefs for built-in functions are read-only.
+	 * So we must not search for built-ins when creating a
+	 * new function.
 	 */
-	if (!createFlag &&
-	    (pBest == 0
-	     || (user_session->sql_flags & SQL_PreferBuiltin) != 0)) {
-		bestScore = 0;
-		h = (sqlUpperToLower[(u8) zName[0]] +
-		     nName) % SQL_FUNC_HASH_SZ;
-		p = functionSearch(h, zName);
-		while (p) {
-			int score = matchQuality(p, nArg);
-			if (score > bestScore) {
-				pBest = p;
-				bestScore = score;
+	if (!is_create && (func == NULL || is_builtin)) {
+lookup_for_builtin:
+		func_score = 0;
+		int h = sql_builtin_func_name_hash(func_name, func_name_len);
+		for (struct FuncDef *p = functionSearch(h, func_name);
+		     p != NULL; p = p->pNext) {
+			int score = matchQuality(p, arg_count);
+			if (score > func_score) {
+				func = p;
+				func_score = score;
 			}
-			p = p->pNext;
 		}
 	}
 
-	/* If the createFlag parameter is true and the search did not reveal an
-	 * exact match for the name, number of arguments and encoding, then add a
-	 * new entry to the hash table and return it.
+	/*
+	 * If the is_create parameter is true and the search did
+	 * not reveal an exact match for the name, number of
+	 * arguments and encoding, then add a new entry to the
+	 * hash table and return it.
 	 */
-	if (createFlag && bestScore < FUNC_PERFECT_MATCH &&
-	    (pBest =
-	     sqlDbMallocZero(db, sizeof(*pBest) + nName + 1)) != 0) {
-		FuncDef *pOther;
-		pBest->zName = (const char *)&pBest[1];
-		pBest->nArg = (u16) nArg;
-		pBest->funcFlags = 0;
-		memcpy((char *)&pBest[1], zName, nName + 1);
-		pOther =
-		    (FuncDef *) sqlHashInsert(&db->aFunc, pBest->zName,
-						  pBest);
-		if (pOther == pBest) {
-			sqlDbFree(db, pBest);
+	if (is_create && func_score < FUNC_PERFECT_MATCH) {
+		uint32_t func_sz = sizeof(func[0]) + func_name_len + 1;
+		func = sqlDbMallocZero(db, func_sz);
+		if (func == NULL) {
+			diag_set(OutOfMemory, func_sz, "sqlDbMallocZero",
+				 "func");
+			return NULL;
+		}
+		func->zName = (const char *) &func[1];
+		func->nArg = (u16) arg_count;
+		func->funcFlags = 0;
+		memcpy((char *)&func[1], func_name, func_name_len + 1);
+		struct FuncDef *old_func =
+			(struct FuncDef *) sqlHashInsert(&db->aFunc, func->zName,
+							 func);
+		if (old_func == func) {
+			sqlDbFree(db, func);
 			sqlOomFault(db);
-			return 0;
+			diag_set(OutOfMemory, func_sz, "sqlHashInsert",
+				 "func");
 		} else {
-			pBest->pNext = pOther;
+			func->pNext = old_func;
 		}
 	}
-
-	if (pBest && (pBest->xSFunc || createFlag)) {
-		return pBest;
-	}
-	return 0;
+	return func != NULL &&
+	       (func->xSFunc != NULL || is_create || is_builtin) ? func : NULL;
 }
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index ba7eea59d..12b54ff65 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -285,9 +285,9 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 		if (op == TK_FUNCTION) {
 			uint32_t arg_count = p->x.pList == NULL ? 0 :
 					     p->x.pList->nExpr;
-			struct FuncDef *func = sqlFindFunction(parse->db,
-							       p->u.zToken,
-							       arg_count, 0);
+			struct FuncDef *func =
+				sql_find_function(parse->db, p->u.zToken,
+						  arg_count, false);
 			if (func == NULL)
 				break;
 			if (func->is_coll_derived) {
@@ -3991,11 +3991,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			nFarg = pFarg ? pFarg->nExpr : 0;
 			assert(!ExprHasProperty(pExpr, EP_IntValue));
 			zId = pExpr->u.zToken;
-			pDef = sqlFindFunction(db, zId, nFarg, 0);
+			pDef = sql_find_function(db, zId, nFarg, false);
 #ifdef SQL_ENABLE_UNKNOWN_SQL_FUNCTION
 			if (pDef == 0 && pParse->explain) {
-				pDef =
-				    sqlFindFunction(db, "unknown", nFarg, 0);
+				pDef = sql_find_function(db, "unknown", nFarg,
+							 false);
 			}
 #endif
 			if (pDef == 0 || pDef->xFinalize != 0) {
@@ -5461,12 +5461,12 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
 						pItem->iMem = ++pParse->nMem;
 						assert(!ExprHasProperty
 						       (pExpr, EP_IntValue));
-						pItem->pFunc = sqlFindFunction(
+						pItem->pFunc = sql_find_function(
 							pParse->db,
 							pExpr->u.zToken,
 							pExpr->x.pList ?
 							pExpr->x.pList->nExpr : 0,
-							0);
+							false);
 						if (pExpr->flags & EP_Distinct) {
 							pItem->iDistinct =
 								pParse->nTab++;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index bb7405e68..c110b205c 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1811,7 +1811,7 @@ sql_overload_function(sql * db, const char *zName,
 {
 	int rc = SQL_OK;
 
-	if (sqlFindFunction(db, zName, nArg, 0) == 0) {
+	if (sql_find_function(db, zName, nArg, false) == 0) {
 		rc = sqlCreateFunc(db, zName, type, nArg, 0, 0,
 				       sqlInvalidFunction, 0, 0, 0);
 	}
@@ -1841,7 +1841,7 @@ static void
 setLikeOptFlag(sql * db, const char *zName, u8 flagVal)
 {
 	FuncDef *pDef;
-	pDef = sqlFindFunction(db, zName, 2, 0);
+	pDef = sql_find_function(db, zName, 2, false);
 	if (ALWAYS(pDef)) {
 		pDef->funcFlags |= flagVal;
 	}
@@ -1875,7 +1875,7 @@ sql_is_like_func(struct sql *db, struct Expr *expr, int *is_like_ci)
 	    expr->x.pList->nExpr != 2)
 		return 0;
 	assert(!ExprHasProperty(expr, EP_xIsSelect));
-	struct FuncDef *func = sqlFindFunction(db, expr->u.zToken, 2, 0);
+	struct FuncDef *func = sql_find_function(db, expr->u.zToken, 2, false);
 	assert(func != NULL);
 	if ((func->funcFlags & SQL_FUNC_LIKE) == 0)
 		return 0;
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index fe1135a71..0e8f3e559 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -434,7 +434,7 @@ sqlCreateFunc(sql * db,
 	 * is being overridden/deleted but there are no active VMs, allow the
 	 * operation to continue but invalidate all precompiled statements.
 	 */
-	p = sqlFindFunction(db, zFunctionName, nArg, 0);
+	p = sql_find_function(db, zFunctionName, nArg, false);
 	if (p && p->nArg == nArg) {
 		if (db->nVdbeActive) {
 			sqlErrorWithMsg(db, SQL_BUSY,
@@ -446,11 +446,9 @@ sqlCreateFunc(sql * db,
 		}
 	}
 
-	p = sqlFindFunction(db, zFunctionName, nArg, 1);
-	assert(p || db->mallocFailed);
-	if (!p) {
-		return SQL_NOMEM;
-	}
+	p = sql_find_function(db, zFunctionName, nArg, true);
+	if (p == NULL)
+		return SQL_TARANTOOL_ERROR;
 
 	/* If an older version of the function with a configured destructor is
 	 * being replaced invoke the destructor function here.
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 504096e6d..7e2eed7bb 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -602,10 +602,10 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 			assert(!ExprHasProperty(pExpr, EP_xIsSelect));
 			zId = pExpr->u.zToken;
 			nId = sqlStrlen30(zId);
-			pDef = sqlFindFunction(pParse->db, zId, n, 0);
+			pDef = sql_find_function(pParse->db, zId, n, false);
 			if (pDef == 0) {
-				pDef =
-				    sqlFindFunction(pParse->db, zId, -2,0);
+				pDef = sql_find_function(pParse->db, zId, -2,
+							 false);
 				if (pDef == 0) {
 					no_such_func = 1;
 				} else {
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d2be6701f..61406cdc9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3927,7 +3927,27 @@ void sqlSelectSetName(Select *, const char *);
 #define sqlSelectSetName(A,B)
 #endif
 void sqlInsertBuiltinFuncs(FuncDef *, int);
-FuncDef *sqlFindFunction(sql *, const char *, int, u8);
+
+/**
+ * Find a user function by given name and number of arguments.
+ * @param db The database handle.
+ * @param func_name The function name 0-terminated string.
+ * @param arg_count The count of function's arguments.
+ *                  May be set -1 for any number.
+ * @param is_create If this flag is set true, then a new (blank)
+ *                  FuncDef structure is created and liked into
+ *                  the "db" structure if a no matching function
+ *                  previously existed.
+ * @retval Returns FuncDef pointer when object was found and NULL
+ *         otherwise (if is_create is not set true).
+ *         When is_create is set true returns NULL only in case of
+ *         memory allocation error.
+ *         Sets diag message in case of error.
+ */
+struct FuncDef *
+sql_find_function(struct sql *db, const char *func_name, int arg_count,
+		  bool is_create);
+
 void sqlRegisterBuiltinFunctions(void);
 void sqlRegisterDateTimeFunctions(void);
 void sqlRegisterPerConnectionBuiltinFunctions(sql *);
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index f73ea0a71..c56eb73e6 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1245,7 +1245,7 @@ valueFromFunction(sql * db,	/* The database connection */
 	pList = p->x.pList;
 	if (pList)
 		nVal = pList->nExpr;
-	pFunc = sqlFindFunction(db, p->u.zToken, nVal, 0);
+	pFunc = sql_find_function(db, p->u.zToken, nVal, false);
 	assert(pFunc);
 	if ((pFunc->funcFlags & (SQL_FUNC_CONSTANT | SQL_FUNC_SLOCHNG)) ==
 	    0 || (pFunc->funcFlags & SQL_FUNC_NEEDCOLL)
-- 
2.21.0