[Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names
Mergen Imeev
imeevma at tarantool.org
Mon Dec 13 10:34:40 MSK 2021
Hi! Thank you for the review! My answers and diff below.
On Thu, Dec 09, 2021 at 01:31:34AM +0100, Vladislav Shpilevoy wrote:
> 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
>
Got it, thanks for the explanation. It might be better to rename "struct Expr"
to "struct sql_expr", in which case we will naturally use the sql_expr_ * prefix
for such functions. How do you think?
> 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().
>
True, fixed.
> > + 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.
Thanks, dropped.
Diff:
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e832984c3..8df314b17 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1345,16 +1345,15 @@ expr_new_variable(struct Parse *parse, const struct Token *spec,
len += id->n;
}
struct Expr *expr = sql_expr_new_empty(parse->db, TK_VARIABLE, len + 1);
+ if (expr == NULL)
+ return NULL;
expr->type = FIELD_TYPE_BOOLEAN;
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;
More information about the Tarantool-patches
mailing list