[Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Dec 1 01:02:40 MSK 2021
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);
> }
More information about the Tarantool-patches
mailing list