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

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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;