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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Previously, it was possible to create a function with name
that is already reserved for some predefined builtin.
It is a bug. This patch bans sql_create_function for such cases.

Also removed FUNC_PERFECT_MATCH user session flag.

Closes #4219
Needed for #3691
---
 src/box/lua/lua_sql.c         |   8 +-
 src/box/sql/callback.c        | 174 +++++++++++++++-------------------
 src/box/sql/expr.c            |  18 ++--
 src/box/sql/func.c            |  33 +++----
 src/box/sql/main.c            |  33 ++++---
 src/box/sql/resolve.c         |   7 +-
 src/box/sql/sqlInt.h          |  25 ++++-
 src/box/sql/vdbemem.c         |   2 +-
 test/sql-tap/lua_sql.test.lua |  11 ++-
 9 files changed, 157 insertions(+), 154 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..b4247324d 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,79 @@ 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_builtin, 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(nArg >= (-2));
-	assert(nArg >= (-1) || createFlag == 0);
-	nName = sqlStrlen30(zName);
-
-	/* First search for a match amongst the application-defined functions.
+	assert(arg_count >= -2);
+	assert(arg_count >= -1 || !is_create);
+	assert(!is_create || !is_builtin);
+	uint32_t func_name_len = strlen(func_name);
+	/*
+	 * The 'score' of the best match is calculated
+	 * with matchQuality estimator.
+	 */
+	int func_score = 0;
+	/*
+	 * The pointer of a function having the highest 'score'
+	 * for given name and arg_count parameters.
 	 */
-	p = (FuncDef *) sqlHashFind(&db->aFunc, zName);
-	while (p) {
-		int score = matchQuality(p, nArg);
-		if (score > bestScore) {
-			pBest = p;
-			bestScore = score;
+	struct FuncDef *func = NULL;
+	if (is_builtin)
+		goto lookup_for_builtin;
+	/* Search amongst the user-defined functions. */
+	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 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.
-	 */
-	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) {
+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, 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");
+			return NULL;
 		} 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..f7507938d 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, false);
 			if (func == NULL)
 				break;
 			if (func->is_coll_derived) {
@@ -3991,14 +3991,14 @@ 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, 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, false);
 			}
 #endif
-			if (pDef == 0 || pDef->xFinalize != 0) {
+			if (pDef == NULL || pDef->xFinalize != NULL) {
 				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
 					 zId);
 				pParse->is_aborted = true;
@@ -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, 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..cbcf26125 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, false) == NULL) {
 		rc = sqlCreateFunc(db, zName, type, nArg, 0, 0,
 				       sqlInvalidFunction, 0, 0, 0);
 	}
@@ -1834,19 +1834,6 @@ sqlRegisterPerConnectionBuiltinFunctions(sql * db)
 	}
 }
 
-/*
- * Set the LIKEOPT flag on the 2-argument function with the given name.
- */
-static void
-setLikeOptFlag(sql * db, const char *zName, u8 flagVal)
-{
-	FuncDef *pDef;
-	pDef = sqlFindFunction(db, zName, 2, 0);
-	if (ALWAYS(pDef)) {
-		pDef->funcFlags |= flagVal;
-	}
-}
-
 /**
  * Register the built-in LIKE function.
  */
@@ -1859,13 +1846,14 @@ sqlRegisterLikeFunctions(sql *db, int is_case_insensitive)
 	 * supplied pattern and FALSE otherwise.
 	 */
 	int *is_like_ci = SQL_INT_TO_PTR(is_case_insensitive);
-	sqlCreateFunc(db, "LIKE", FIELD_TYPE_BOOLEAN, 2, 0,
-			  is_like_ci, likeFunc, 0, 0, 0);
-	sqlCreateFunc(db, "LIKE", FIELD_TYPE_BOOLEAN, 3, 0,
-			  is_like_ci, likeFunc, 0, 0, 0);
-	setLikeOptFlag(db, "LIKE",
-		       !(is_case_insensitive) ? (SQL_FUNC_LIKE |
-		       SQL_FUNC_CASE) : SQL_FUNC_LIKE);
+	struct FuncDef *like2 = sql_find_function(db, "LIKE", 2, true, false);
+	like2->funcFlags = is_case_insensitive ? SQL_FUNC_LIKE :
+			   SQL_FUNC_LIKE | SQL_FUNC_CASE;
+	like2->pUserData = is_like_ci;
+	assert(like2 != NULL);
+	struct FuncDef *like3 = sql_find_function(db, "LIKE", 2, true, false);
+	assert(like3 != NULL);
+	like3->pUserData = is_like_ci;
 }
 
 int
@@ -1875,7 +1863,8 @@ 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, 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..d52ec1c49 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -428,29 +428,28 @@ sqlCreateFunc(sql * db,
 	assert(SQL_FUNC_CONSTANT == SQL_DETERMINISTIC);
 	extraFlags = flags & SQL_DETERMINISTIC;
 
-
+	/* Ensure there is no builtin function with this name. */
+	p = sql_find_function(db, zFunctionName, -2, true, false);
+	if (p != NULL) {
+		diag_set(ClientError, ER_SQL, "cannot create a function with "
+			 "a signature that coincides builtin function");
+		return SQL_TARANTOOL_ERROR;
+	}
 	/* Check if an existing function is being overridden or deleted. If so,
 	 * and there are active VMs, then return SQL_BUSY. If a function
 	 * 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);
-	if (p && p->nArg == nArg) {
-		if (db->nVdbeActive) {
-			sqlErrorWithMsg(db, SQL_BUSY,
-					    "unable to delete/modify user-function due to active statements");
-			assert(!db->mallocFailed);
-			return SQL_BUSY;
-		} else {
-			sqlExpirePreparedStatements(db);
-		}
-	}
-
-	p = sqlFindFunction(db, zFunctionName, nArg, 1);
-	assert(p || db->mallocFailed);
-	if (!p) {
-		return SQL_NOMEM;
+	p = sql_find_function(db, zFunctionName, nArg, false, false);
+	if (p != NULL && p->nArg == nArg && db->nVdbeActive > 0) {
+		sqlErrorWithMsg(db, SQL_BUSY, "unable to delete/modify "
+				"user-function due to active statements");
+		return SQL_BUSY;
 	}
+	/* Create a new function with given signature. */
+	p = sql_find_function(db, zFunctionName, nArg, false, 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..7cf79692c 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -602,10 +602,11 @@ 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, false);
 			if (pDef == 0) {
-				pDef =
-				    sqlFindFunction(pParse->db, zId, -2,0);
+				pDef = sql_find_function(pParse->db, zId, -2,
+							 false, 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..9384ec9e8 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1506,7 +1506,6 @@ struct sql {
 #define SQL_ReverseOrder   0x00020000	/* Reverse unordered SELECTs */
 #define SQL_RecTriggers    0x00040000	/* Enable recursive triggers */
 #define SQL_AutoIndex      0x00100000	/* Enable automatic indexes */
-#define SQL_PreferBuiltin  0x00200000	/* Preference to built-in funcs */
 #define SQL_EnableTrigger  0x01000000	/* True to enable triggers */
 #define SQL_DeferFKs       0x02000000	/* Defer all FK constraints */
 #define SQL_VdbeEQP        0x08000000	/* Debug EXPLAIN QUERY PLAN */
@@ -3927,7 +3926,29 @@ 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_builtin If this flag is set true, perform lookup
+ *                   for builtin function.
+ * @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_builtin, 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..537a72f10 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, false);
 	assert(pFunc);
 	if ((pFunc->funcFlags & (SQL_FUNC_CONSTANT | SQL_FUNC_SLOCHNG)) ==
 	    0 || (pFunc->funcFlags & SQL_FUNC_NEEDCOLL)
diff --git a/test/sql-tap/lua_sql.test.lua b/test/sql-tap/lua_sql.test.lua
index b0913e7f4..fd7cd732a 100755
--- a/test/sql-tap/lua_sql.test.lua
+++ b/test/sql-tap/lua_sql.test.lua
@@ -1,7 +1,7 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
 NULL = require('msgpack').NULL
-test:plan(24)
+test:plan(25)
 
 local function func1(a)
     return a
@@ -18,6 +18,15 @@ test:do_test(
     end,
     {2})
 
+test:do_test(
+    "lua_sql-1.1",
+    function ()
+        local ret, errmsg = pcall(box.internal.sql_create_function,
+                                  "upper", "TEXT", func1)
+        return {ret, errmsg}
+    end,
+    {0, "SQL error: cannot create a function with a signature that coincides builtin function"})
+
 -- new function should replace prewious one
 test:do_test(
     "lua_sql-1.1",
-- 
2.21.0