[Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Dec 9 03:31:34 MSK 2021
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.
More information about the Tarantool-patches
mailing list