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

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

Hi! Thank you for the review! And I'm sorry for such poor quality of my
self-review this time. I will try to avoid this in future. My asnwers,
diff and new patch below.

On Tue, Nov 30, 2021 at 11:02:40PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>> 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)
> >>
> >> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
> >> To be consistent with our naming policy for constructors allocating memory
> >> and for consistency with with sql_expr_new_column(), sql_expr_new(),
> >> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
> >>
> > Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
> > prefix for functions that only accessible in SQL.
> 
> It would work for static functions. But if a function is visible in other
> modules as a symbol, then you would get a conflict during linking if we
> ever introduce another 'struct expr' somewhere. Even if they do not interest
> anywhere in the code. However I don't mind leaving it as is. It can be fixed
> later if ever needed.
> 
I agree. However, I think we need to rework all the places where BOX uses
internal SQL functions and structures. In this case, the struct expr should
never be available in the BOX, so there should be no conflicts.

> See 5 comments below.
> 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index eb169aeb8..8ff01dd33 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -1314,6 +1314,59 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
> >  	}
> >  }
> >  
> > +struct Expr *
> > +expr_new_variable(struct Parse *parse, const struct Token *spec,
> > +		  const struct Token *id)
> > +{
> > +	assert(spec != 0 && spec->n == 1);
> > +	uint32_t len = 1;
> > +	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 (id != NULL) {
> > +		assert(spec->z[0] != '?');
> > +		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 (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
> 
> 1. You already checked for 'id != NULL' several lines above. Also you
> don't need () around the second &&.
> 
Thanks, fixed.

> > +			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
> > +				 parse->line_count, spec->n, spec->z);
> > +			parse->is_aborted = true;
> > +			return NULL;
> > +		}
> > +		len += id->n;
> > +	}
> > +	struct Expr *expr = sqlDbMallocRawNN(parse->db,
> > +					     sizeof(*expr) + len + 1);
> 
> 2. sql_expr_new_empty().
> 
Fixed.

> > +	if (expr == NULL) {
> > +		parse->is_aborted = true;
> > +		return NULL;
> > +	}
> > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > index ee319d5ad..15f6223b0 100644
> > --- a/src/box/sql/parse.y
> > +++ b/src/box/sql/parse.y
> > @@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
> >        p->flags = EP_Leaf;
> >        p->iAgg = -1;
> >        p->u.zToken = (char*)&p[1];
> > -      if (op != TK_VARIABLE) {
> > -        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> > -        if (rc > name_sz) {
> > -          name_sz = rc;
> > -          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> > -          if (p == NULL)
> > -            goto tarantool_error;
> > -          p->u.zToken = (char *) &p[1];
> > -          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
> > -              unreachable();
> > +      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> > +      if (rc > name_sz) {
> > +        name_sz = rc;
> > +        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
> > +        if (p == NULL) {
> > +          sqlDbFree(pParse->db, p);
> 
> 3. From the name sqlDbReallocOrFree() it looks like 'p' is already
> freed. Also it is NULL here anyway, what makes sqlDbFree() nop.
> 
Fixed.

> > +          pParse->is_aborted = true;
> 
> 4. You will get a crash below, because p is NULL but you dereference
> it right afterwards.
> 

Fixed.

> >          }
> > -      } else {
> > -        memcpy(p->u.zToken, t.z, t.n);
> > -        p->u.zToken[t.n] = 0;
> > +        p->u.zToken = (char *) &p[1];
> 
> 5. Don't need whitespace after cast operator.
> 
Fixed.

> > +        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
> >        }


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8ff01dd33..e832984c3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1318,7 +1318,7 @@ struct Expr *
 expr_new_variable(struct Parse *parse, const struct Token *spec,
 		  const struct Token *id)
 {
-	assert(spec != 0 && spec->n == 1);
+	assert(spec != NULL && spec->n == 1);
 	uint32_t len = 1;
 	if (parse->parse_only) {
 		diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS,
@@ -1336,7 +1336,7 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
 			parse->is_aborted = true;
 			return NULL;
 		}
-		if (spec->z[0] == '#' && (id != NULL && sqlIsdigit(id->z[0]))) {
+		if (spec->z[0] == '#' && sqlIsdigit(id->z[0])) {
 			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
 				 parse->line_count, spec->n, spec->z);
 			parse->is_aborted = true;
@@ -1344,15 +1344,8 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
 		}
 		len += 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));
+	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
 	expr->type = FIELD_TYPE_BOOLEAN;
-	expr->op = TK_VARIABLE;
 	expr->flags = EP_Leaf;
 	expr->iAgg = -1;
 	expr->u.zToken = (char *)(expr + 1);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 15f6223b0..ecc7f7778 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1015,10 +1015,10 @@ idlist(A) ::= nm(Y). {
         name_sz = rc;
         p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
         if (p == NULL) {
-          sqlDbFree(pParse->db, p);
           pParse->is_aborted = true;
+          return;
         }
-        p->u.zToken = (char *) &p[1];
+        p->u.zToken = (char *)&p[1];
         sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0


New patch:

commit 9a4bfea66a2e76340e96dff4ba0b04fe54f9d681
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Nov 18 11:02:20 2021 +0300

    sql: properly check bind variable names
    
    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

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb169aeb8..e832984c3 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_new_variable(struct Parse *parse, const struct Token *spec,
+		  const struct Token *id)
+{
+	assert(spec != NULL && spec->n == 1);
+	uint32_t len = 1;
+	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 (id != NULL) {
+		assert(spec->z[0] != '?');
+		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 (spec->z[0] == '#' && 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;
+		}
+		len += id->n;
+	}
+	struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
+	expr->type = FIELD_TYPE_BOOLEAN;
+	expr->flags = EP_Leaf;
+	expr->iAgg = -1;
+	expr->u.zToken = (char *)(expr + 1);
+	expr->u.zToken[0] = spec->z[0];
+	if (id != NULL)
+		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 ee319d5ad..ecc7f7778 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1002,15 +1002,6 @@ idlist(A) ::= nm(Y). {
       case TK_UNKNOWN:
         p->type = FIELD_TYPE_BOOLEAN;
         break;
-      case TK_VARIABLE:
-        /*
-         * For variables we set BOOLEAN type since
-         * unassigned bindings will be replaced
-         * with NULL automatically, i.e. without
-         * explicit call of sql_bind_*().
-         */
-        p->type = FIELD_TYPE_BOOLEAN;
-        break;
       default:
         p->type = FIELD_TYPE_SCALAR;
         break;
@@ -1019,20 +1010,16 @@ idlist(A) ::= nm(Y). {
       p->flags = EP_Leaf;
       p->iAgg = -1;
       p->u.zToken = (char*)&p[1];
-      if (op != TK_VARIABLE) {
-        int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
-        if (rc > name_sz) {
-          name_sz = rc;
-          p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
-          if (p == NULL)
-            goto tarantool_error;
-          p->u.zToken = (char *) &p[1];
-          if (sql_normalize_name(p->u.zToken, name_sz, t.z, t.n) > name_sz)
-              unreachable();
+      int rc = sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
+      if (rc > name_sz) {
+        name_sz = rc;
+        p = sqlDbReallocOrFree(pParse->db, p, sizeof(*p) + name_sz);
+        if (p == NULL) {
+          pParse->is_aborted = true;
+          return;
         }
-      } else {
-        memcpy(p->u.zToken, t.z, t.n);
-        p->u.zToken[t.n] = 0;
+        p->u.zToken = (char *)&p[1];
+        sql_normalize_name(p->u.zToken, name_sz, t.z, t.n);
       }
 #if SQL_MAX_EXPR_DEPTH>0
       p->nHeight = 1;
@@ -1042,9 +1029,6 @@ idlist(A) ::= nm(Y). {
     pOut->zStart = t.z;
     pOut->zEnd = &t.z[t.n];
     return;
-tarantool_error:
-    sqlDbFree(pParse->db, p);
-    pParse->is_aborted = true;
   }
 }
 
@@ -1087,30 +1071,17 @@ 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;
-  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;
-    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;
-  }
+expr(A) ::= VARNUM(X). {
+  A.pExpr = expr_new_variable(pParse, &X, NULL);
+  spanSet(&A, &X, &X);
+}
+expr(A) ::= VARIABLE(X) id(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
+}
+expr(A) ::= VARIABLE(X) INTEGER(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
 expr(A) ::= expr(A) COLLATE id(C). {
   A.pExpr = sqlExprAddCollateToken(pParse, A.pExpr, &C, 1);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dcd71e5bd..1c469e3fc 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2617,6 +2617,17 @@ 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_new_variable(struct Parse *parse, const struct Token *spec,
+		  const 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})
 ---