Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
Date: Thu, 25 Nov 2021 11:33:36 +0300	[thread overview]
Message-ID: <20211125083336.GA56448@tarantool.org> (raw)
In-Reply-To: <f20d62b1-a6eb-ecd4-ed2d-2e699f2a4418@tarantool.org>

Hi! Thank you for the review! My answers, diff and new patch below.

On Sat, Nov 20, 2021 at 01:45:36AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Shouldn't these be the same?
> 
I don't know why, but we (like SQLIte) prohibit the use of the "#" character in
front of digits, even if the name begins with a digit. In addition, we disallow
the use of the "$" character with names (we can only use this with numbers),
although this is allowed in SQLite. In general, I think that the work with bound
variables needs to be redesigned. I'll create an issue about this.

> tarantool> box.execute('SELECT $name', {})
> ---
> - null
> - Index of binding slots must start from 1
> ...
> 
> tarantool> box.execute('SELECT @name', {})
> ---
> - metadata:
>   - name: COLUMN_1
>     type: boolean
>   rows:
>   - [null]
> ...
> 
> See 4 comments below.
> 
> > 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.

> 
> 2. spec and id can be made const.
> 
Thanks, fixed.

> > +{
> > +	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;
> > +	}
> > 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;
> 
> 3. Hm. It looks not so good that this complicated error is now in
> 2 places and is exactly the same. Maybe we could have a common
> function both for positional and named arguments? Or 2 functions,
> but they would use one more generic function inside?
> 
> For instance, one function could be
> 
> 	sql_expr_new_var(struct Parse *parse, const struct Token *spec,
> 			 const struct Token *id);
> 
> For positionals it would be called as
> 
> 	sql_expr_new_var(pParse, &X, NULL);
> 
> For named:
> 
> 	sql_expr_new_var(pParse, &X, &Y); (like now)
> 
> Then we could also drop TK_VARIABLE from spanExpr() completely.
> 
Thank you for the suggestions! Fixed.

> >      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];
> > +}
> 
> 4. Lets drop the extra spaces before { symbols.
Fixed.


Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 74a98c550..8ff01dd33 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1315,15 +1315,11 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 }
 
 struct Expr *
-expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
+expr_new_variable(struct Parse *parse, const struct Token *spec,
+		  const 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;
-	}
+	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,
@@ -1331,13 +1327,23 @@ expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
 		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;
+	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]))) {
+			diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN,
+				 parse->line_count, spec->n, spec->z);
+			parse->is_aborted = true;
+			return NULL;
+		}
+		len += id->n;
 	}
-	uint32_t len = spec->n + id->n;
 	struct Expr *expr = sqlDbMallocRawNN(parse->db,
 					     sizeof(*expr) + len + 1);
 	if (expr == NULL) {
@@ -1351,7 +1357,8 @@ expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
 	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);
+	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;
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index f6d17a2be..15f6223b0 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) {
+          sqlDbFree(pParse->db, p);
+          pParse->is_aborted = true;
         }
-      } 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;
   }
 }
 
@@ -1088,27 +1072,16 @@ term(A) ::= INTEGER(X). {
   if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
 }
 expr(A) ::= VARNUM(X). {
-  if (pParse->parse_only) {
-    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;
-    A.zEnd = &X.z[X.n];
-  } else {
-    spanExpr(&A, pParse, TK_VARIABLE, X);
-    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
-    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
-  }
+  A.pExpr = expr_new_variable(pParse, &X, NULL);
+  spanSet(&A, &X, &X);
 }
-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) id(Y). {
+  A.pExpr = expr_new_variable(pParse, &X, &Y);
+  spanSet(&A, &X, &Y);
 }
-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) ::= 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 ab5b098ee..1c469e3fc 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2625,7 +2625,8 @@ ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
  * @param id Name or position number of bound variable.
  */
 struct Expr *
-expr_variable(struct Parse *parse, struct Token *spec, struct Token *id);
+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.


New patch:

commit d971ccc20c6902097aa85aef5592a38c083d4dd7
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..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]))) {
+			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);
+	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];
+	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..15f6223b0 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) {
+          sqlDbFree(pParse->db, p);
+          pParse->is_aborted = true;
         }
-      } 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})
 ---

  reply	other threads:[~2021-11-25  8:33 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches [this message]
2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:47               ` Vladislav Shpilevoy via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:48               ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211125083336.GA56448@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox