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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

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 &&.

> +			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().

> +	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.

> +          pParse->is_aborted = true;

4. You will get a crash below, because p is NULL but you dereference
it right afterwards.

>          }
> -      } 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.

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