[Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

After this patch, variable names will have to follow the rules defined
for identifiers in SQL. This essentially means that the number will no
longer be used as the first character of the bind variable name.

Part of #4763
---
 src/box/sql/expr.c          | 46 +++++++++++++++++++++++++++++++++++++
 src/box/sql/parse.y         | 32 ++++++++++++--------------
 src/box/sql/sqlInt.h        | 10 ++++++++
 src/box/sql/tokenize.c      | 26 ++++++---------------
 test/sql-tap/bind.test.lua  | 15 ++++++++++++
 test/sql-tap/misc1.test.lua |  4 ++--
 test/sql/iproto.result      |  2 +-
 7 files changed, 96 insertions(+), 39 deletions(-)
 create mode 100755 test/sql-tap/bind.test.lua

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb169aeb8..74a98c550 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 	}
 }
 
+struct Expr *
+expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
+{
+	assert(spec != 0 && id != NULL && spec->n == 1);
+	if (id->z - spec->z != 1) {
+		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
+			 spec->z - parse->zTail + 1, spec->n, spec->z);
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (parse->parse_only) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
+			 parse->line_count, parse->line_pos,
+			 "bindings are not allowed in DDL");
+		parse->is_aborted = true;
+		return NULL;
+	}
+	if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+		diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+			 parse->line_count, spec->n, spec->z);
+		parse->is_aborted = true;
+		return NULL;
+	}
+	uint32_t len = spec->n + id->n;
+	struct Expr *expr = sqlDbMallocRawNN(parse->db,
+					     sizeof(*expr) + len + 1);
+	if (expr == NULL) {
+		parse->is_aborted = true;
+		return NULL;
+	}
+	memset(expr, 0, sizeof(*expr));
+	expr->type = FIELD_TYPE_BOOLEAN;
+	expr->op = TK_VARIABLE;
+	expr->flags = EP_Leaf;
+	expr->iAgg = -1;
+	expr->u.zToken = (char *)(expr + 1);
+	expr->u.zToken[0] = spec->z[0];
+	memcpy(expr->u.zToken + 1, id->z, id->n);
+	expr->u.zToken[len] = '\0';
+	assert(SQL_MAX_EXPR_DEPTH > 0);
+	expr->nHeight = 1;
+
+	sqlExprAssignVarNumber(parse, expr, len);
+	return expr;
+}
+
 /*
  * Recursively delete an expression tree.
  */
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 92fb37dd4..06e6244e3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1087,31 +1087,29 @@ term(A) ::= INTEGER(X). {
   A.zEnd = X.z + X.n;
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
-expr(A) ::= VARIABLE(X).     {
-  Token t = X;
+expr(A) ::= VARNUM(X). {
   if (pParse->parse_only) {
-    spanSet(&A, &t, &t);
     diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
              pParse->line_pos, "bindings are not allowed in DDL");
     pParse->is_aborted = true;
     A.pExpr = NULL;
-  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
-    u32 n = X.n;
+    A.zEnd = &X.z[X.n];
+  } else {
     spanExpr(&A, pParse, TK_VARIABLE, X);
-    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
-      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-      pParse->is_aborted = true;
-    } else {
-      sqlExprAssignVarNumber(pParse, A.pExpr, n);
-    }
-  }else{
-    assert( t.n>=2 );
-    spanSet(&A, &t, &t);
-    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
-    pParse->is_aborted = true;
-    A.pExpr = NULL;
+    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
+    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
   }
 }
+expr(A) ::= VARIABLE(X) id(Y).     {
+  A.pExpr = expr_variable(pParse, &X, &Y);
+  A.zStart = X.z;
+  A.zEnd = &Y.z[Y.n];
+}
+expr(A) ::= VARIABLE(X) INTEGER(Y).     {
+  A.pExpr = expr_variable(pParse, &X, &Y);
+  A.zStart = X.z;
+  A.zEnd = &Y.z[Y.n];
+}
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
   A.zEnd = &C.z[C.n];
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dcd71e5bd..ab5b098ee 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2617,6 +2617,16 @@ Expr *sqlExprFunction(Parse *, ExprList *, Token *);
 void sqlExprAssignVarNumber(Parse *, Expr *, u32);
 ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
 
+/**
+ * Parse tokens as a name or a position of bound variable.
+ *
+ * @param parse Parse context.
+ * @param spec Special symbol for bound variable.
+ * @param id Name or position number of bound variable.
+ */
+struct Expr *
+expr_variable(struct Parse *parse, struct Token *spec, struct Token *id);
+
 /**
  * Set the sort order for the last element on the given ExprList.
  *
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index f2d5a2df5..8bc519b9d 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -58,8 +58,8 @@
 #define CC_KYWD       1		/* Alphabetics or '_'.  Usable in a keyword */
 #define CC_ID         2		/* unicode characters usable in IDs */
 #define CC_DIGIT      3		/* Digits */
-#define CC_DOLLAR     4		/* '$' */
-#define CC_VARALPHA   5		/* '@', '#', ':'.  Alphabetic SQL variables */
+/** SQL variables: '@', '#', ':', and '$'. */
+#define CC_VARALPHA   5
 #define CC_VARNUM     6		/* '?'.  Numeric SQL variables */
 #define CC_SPACE      7		/* Space characters */
 #define CC_QUOTE      8		/* '\''. String literals */
@@ -90,7 +90,7 @@ static const char sql_ascii_class[] = {
 /*       x0  x1  x2  x3  x4  x5  x6  x7  x8 x9  xa xb  xc xd xe  xf */
 /* 0x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 7, 28, 7, 7, 7, 27, 27,
 /* 1x */ 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27, 27,
-/* 2x */ 7, 15, 9, 5, 4, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
+/* 2x */ 7, 15, 9, 5, 5, 22, 24, 8, 17, 18, 21, 20, 23, 11, 26, 16,
 /* 3x */ 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 19, 12, 14, 13, 6,
 /* 4x */ 5, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
 /* 5x */ 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 29, 27, 30, 27, 1,
@@ -184,7 +184,7 @@ int
 sql_token(const char *z, int *type, bool *is_reserved)
 {
 	*is_reserved = false;
-	int i, n;
+	int i;
 	char c, delim;
 	/* Switch on the character-class of the first byte
 	 * of the token. See the comment on the CC_ defines
@@ -369,23 +369,11 @@ sql_token(const char *z, int *type, bool *is_reserved)
 		}
 		return i;
 	case CC_VARNUM:
-		*type = TK_VARIABLE;
-		for (i = 1; sqlIsdigit(z[i]); i++) {
-		}
-		return i;
-	case CC_DOLLAR:
+		*type = TK_VARNUM;
+		return 1;
 	case CC_VARALPHA:
-		n = 0;
 		*type = TK_VARIABLE;
-		for (i = 1; (c = z[i]) != 0; i++) {
-			if (IdChar(c))
-				n++;
-			else
-				break;
-		}
-		if (n == 0)
-			*type = TK_ILLEGAL;
-		return i;
+		return 1;
 	case CC_KYWD:
 		for (i = 1; sql_ascii_class[*(unsigned char*)(z+i)] <= CC_KYWD;
 		     i++) {
diff --git a/test/sql-tap/bind.test.lua b/test/sql-tap/bind.test.lua
new file mode 100755
index 000000000..aaf14a44d
--- /dev/null
+++ b/test/sql-tap/bind.test.lua
@@ -0,0 +1,15 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(1)
+
+-- Make sure that bind variable names cannot start with a number.
+test:do_test(
+    "bind-1",
+    function()
+        local res = {pcall(box.execute, [[SELECT @1asd;]], {{['@1asd'] = 123}})}
+        return {tostring(res[3])}
+    end, {
+        "At line 1 at or near position 9: unrecognized token '1asd'"
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
index f207d3e92..204e070d2 100755
--- a/test/sql-tap/misc1.test.lua
+++ b/test/sql-tap/misc1.test.lua
@@ -1052,7 +1052,7 @@ test:do_catchsql_test(
         select''like''like''like#0;
     ]], {
         -- <misc1-21.1>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.1>
     })
 
@@ -1062,7 +1062,7 @@ test:do_catchsql_test(
         VALUES(0,0x0MATCH#0;
     ]], {
         -- <misc1-21.2>
-        1, [[Syntax error at line 1 near '#0']]
+        1, [[Syntax error at line 1 near '#']]
         -- </misc1-21.2>
     })
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6212aa0c0..5a424596e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -351,7 +351,7 @@ cn:execute('drop table if exists test3')
 --
 cn:execute('select ?1, ?2, ?3', {1, 2, 3})
 ---
-- error: Syntax error at line 1 near '?1'
+- error: Syntax error at line 1 near '1'
 ...
 cn:execute('select $name, $name2', {1, 2})
 ---
-- 
2.25.1