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

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

Hi! 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.

It is a misunderstanding. It does not matter if you use a function in box or
not. If it is not static and is defined in 2 places - you will get a conflict
during link stage. Try to add a function with the same name to any file in
sql and to any file in box. Something like

	void
	link_test123(void)
	{
		printf("in link test\n");
	}

(in a .cc file you would need to add 'extern "C"' for it). It will raise an
error during build. I added it to expr.c and to box.cc (with 'extern "C"'):

	duplicate symbol '_link_test123' in:
	    ../../src/box/libbox.a(box.cc.o)
	    ../../src/box/libbox.a(expr.c.o)
	ld: 1 duplicate symbol for architecture x86_64

It means if we ever have another expr, there will be a conflict. Does not
matter if they intersect in code. We will get a compile error even on the
struct name duplicate I think. But not sure.

See 2 comments below.

> 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;

1. It will crash in case allocation fails and expr == NULL. Although maybe
it is not important if we plan to add panic() on malloc failure in SQL. The
same way as it already works in xmalloc().

> +	expr->flags = EP_Leaf;
> +	expr->iAgg = -1;

2. iAgg -1 is already set in sql_expr_new_empty(). nHeight too. And
then the assert about SQL_MAX_EXPR_DEPTH > 0 is not needed either.