[tarantool-patches] Re: [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 SQL_PreferBuiltin user session flag.

Closes #4219
Needed for #3691
---
 src/box/sql/callback.c        | 12 ++----------
 src/box/sql/expr.c            |  8 ++++----
 src/box/sql/func.c            | 33 +++++++++++----------------------
 src/box/sql/main.c            | 12 +++++++++---
 src/box/sql/resolve.c         |  5 +++--
 src/box/sql/sqlInt.h          |  5 +++--
 src/box/sql/vdbemem.c         |  2 +-
 test/sql-tap/lua_sql.test.lua | 11 ++++++++++-
 8 files changed, 43 insertions(+), 45 deletions(-)

diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index 54d59bbd3..b5163c208 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -37,7 +37,6 @@
 
 #include "box/coll_id_cache.h"
 #include "sqlInt.h"
-#include "box/session.h"
 
 struct coll *
 sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id)
@@ -173,7 +172,7 @@ sqlInsertBuiltinFuncs(FuncDef * aDef,	/* List of global functions to be inserted
 
 struct FuncDef *
 sql_find_function(struct sql *db, const char *func_name, int arg_count,
-		  bool is_create)
+		  bool is_builtin, bool is_create)
 {
 	assert(arg_count >= -2);
 	assert(arg_count >= -1 || !is_create);
@@ -185,9 +184,6 @@ sql_find_function(struct sql *db, const char *func_name, int arg_count,
 	int func_score = 0;
 	/* Best match found so far. */
 	struct FuncDef *func = NULL;
-
-	struct session *user_session = current_session();
-	bool is_builtin = (user_session->sql_flags & SQL_PreferBuiltin) != 0;
 	if (is_builtin)
 		goto lookup_for_builtin;
 
@@ -207,10 +203,6 @@ sql_find_function(struct sql *db, const char *func_name, int arg_count,
 	/**
 	 * 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 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
@@ -219,7 +211,7 @@ sql_find_function(struct sql *db, const char *func_name, int arg_count,
 	 * So we must not search for built-ins when creating a
 	 * new function.
 	 */
-	if (!is_create && (func == NULL || is_builtin)) {
+	if (!is_create && func == NULL) {
 lookup_for_builtin:
 		func_score = 0;
 		int h = sql_builtin_func_name_hash(func_name, func_name_len);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 12b54ff65..7133ec3ac 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -287,7 +287,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 					     p->x.pList->nExpr;
 			struct FuncDef *func =
 				sql_find_function(parse->db, p->u.zToken,
-						  arg_count, false);
+						  arg_count, false, 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 = sql_find_function(db, zId, nFarg, false);
+			pDef = sql_find_function(db, zId, nFarg, false, false);
 #ifdef SQL_ENABLE_UNKNOWN_SQL_FUNCTION
 			if (pDef == 0 && pParse->explain) {
 				pDef = sql_find_function(db, "unknown", nFarg,
-							 false);
+							 false, false);
 			}
 #endif
 			if (pDef == 0 || pDef->xFinalize != 0) {
@@ -5466,7 +5466,7 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
 							pExpr->u.zToken,
 							pExpr->x.pList ?
 							pExpr->x.pList->nExpr : 0,
-							false);
+							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 c110b205c..afc25aab5 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 (sql_find_function(db, zName, nArg, false) == 0) {
+	if (sql_find_function(db, zName, nArg, false, false) == 0) {
 		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 = sql_find_function(db, zName, 2, false);
-	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 = sql_find_function(db, expr->u.zToken, 2, false);
+	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 0e8f3e559..cd2d00642 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -428,13 +428,19 @@ 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 = sql_find_function(db, zFunctionName, nArg, false);
+	p = sql_find_function(db, zFunctionName, nArg, false, false);
 	if (p && p->nArg == nArg) {
 		if (db->nVdbeActive) {
 			sqlErrorWithMsg(db, SQL_BUSY,
@@ -446,7 +452,7 @@ sqlCreateFunc(sql * db,
 		}
 	}
 
-	p = sql_find_function(db, zFunctionName, nArg, true);
+	p = sql_find_function(db, zFunctionName, nArg, false, true);
 	if (p == NULL)
 		return SQL_TARANTOOL_ERROR;
 
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 7e2eed7bb..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 = sql_find_function(pParse->db, zId, n, false);
+			pDef = sql_find_function(pParse->db, zId, n,
+						 false, false);
 			if (pDef == 0) {
 				pDef = sql_find_function(pParse->db, zId, -2,
-							 false);
+							 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 61406cdc9..8cdad4749 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 */
@@ -3934,6 +3933,8 @@ void sqlInsertBuiltinFuncs(FuncDef *, int);
  * @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 the built-in 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
@@ -3946,7 +3947,7 @@ void sqlInsertBuiltinFuncs(FuncDef *, int);
  */
 struct FuncDef *
 sql_find_function(struct sql *db, const char *func_name, int arg_count,
-		  bool is_create);
+		  bool is_builtin, bool is_create);
 
 void sqlRegisterBuiltinFunctions(void);
 void sqlRegisterDateTimeFunctions(void);
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index c56eb73e6..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 = sql_find_function(db, p->u.zToken, nVal, false);
+	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