Re: [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll()

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Although the patch is correct, I don't see a reason to push it as part
of this series. 
We have a bug, we have a repro - it's a standalone fix?

Regards,
Sergos


On 06 Dec 15:48, Nikita Pettik wrote:
> On 06 Dec 00:59, Vladislav Shpilevoy wrote:
> > On 05/12/2019 12:40, Nikita Pettik wrote:
> > > On 28 Nov 23:42, Vladislav Shpilevoy wrote:
> > >> Thanks for the patch!
> > >>
> > >> Is it possible to test this?
> > > 
> > > As a pure test case - I've failed to come up with it.
> > > But the next patch (which adds collation to metadata) definitely fails
> > > without this fix (when TRIM() or REPLACE() are called without args).
> > 
> > Why does not it fail right after finding that there are no
> > arguments? TRIM looks strange, but REPLACE expects 3 arguments.
> > 
> > > I can dive into details, but I guess it's not so important here (since
> > > this is obviously buggy place). 
> > 
> > Is it? As I pointed above, maybe the bug is really in another
> > place? I see, that REPLACE checks argument count. But somewhy
> > too late:
> > 
> >     tarantool> box.execute('SELECT REPLACE()')
> >     ---
> >     - null
> >     - 'Wrong number of arguments is passed to REPLACE(): expected 3, got 0'
> >     ...
> > 
> > > If you want further investigation, let me
> > > know and I will do it.
> 
> Sorry, in fact I mean SUBSTR() function. It can take 1 or 2 arguments.
> But the check of arguments count is delayed till function execution (see
> substrFunc in sql/func.c).
> To generate collation for metadata we should call sql_expr_coll()
> (without such necessity we avoid call of this function). And it leads
> to null-dereference.
> 
> Finally, I've come up with example which results in null-dereference on
> master branch as well (added it to the current patch):
> 
> box.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT, b TEXT COLLATE \"binary\", c TEXT COLLATE \"unicode_ci\");")
> box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = substr();")
> 
> Here we have to call sql_expr_coll() with expression representing
> substr() in order to verify that collations of LHS and RHS are
> compatible. So we eventually we get in the same situation.
>  
> > Yeah. I think we need to understand the bug before fixing
> > it.
> 
> New patch:
> 
> 
>     sql: fix possible null dereference in sql_expr_coll()
>     
>     Some built-in functions can accept different number of arguments.
>     Check of argument count for such functions takes place right before
>     its execution. So it is possible that expression-list representing
>     arguments for built-in function is NULL. On the other hand, in
>     sql_expr_coll() (which returns collation of expression) it is assumed
>     that if function features SQL_FUNC_DERIVEDCOLL flag (it implies that
>     resulting collation depends on collation of arguments) then it has at
>     least one argument. The last assumption is wrong considering for example
>     SUBSTR() function: it may have 1 or 2 arguments, so check of argument
>     count doesn't occur during compilation. Hence, if it is required to
>     calculate collation for SUBSTR() function and there's no arguments,
>     Tarantool crashes due to null-dereference.
>     This patch fixes this bug with one additional check in sql_expr_coll().
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 648b7170e..0bdcfe576 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -332,7 +332,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
>                                 sql_func_by_signature(p->u.zToken, arg_count);
>                         if (func == NULL)
>                                 break;
> -                       if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL)) {
> +                       if (sql_func_flag_is_set(func, SQL_FUNC_DERIVEDCOLL) &&
> +                           arg_count > 0) {
>                                 /*
>                                  * Now we use quite straightforward
>                                  * approach assuming that resulting
> diff --git a/test/sql/collation.result b/test/sql/collation.result
> index 11962ef47..fbc7ce9aa 100644
> --- a/test/sql/collation.result
> +++ b/test/sql/collation.result
> @@ -292,6 +292,15 @@ box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = c COLLATE \"unicode\";
>  - null
>  - Illegal mix of collations
>  ...
> +-- Make sure that using function featuring variable arguemnts
> +-- length  and resulting collation which depends on arguments
> +-- is processed correctly.
> +--
> +box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = substr();")
> +---
> +- null
> +- 'Wrong number of arguments is passed to SUBSTR(): expected 1 or 2, got 0'
> +...
>  -- Compound queries perform implicit comparisons between values.
>  -- Hence, rules for collations compatibilities are the same.
>  --
> diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
> index 1be28b3ff..a013253cd 100644
> --- a/test/sql/collation.test.lua
> +++ b/test/sql/collation.test.lua
> @@ -80,6 +80,11 @@ box.execute("SELECT * FROM t WHERE b = c;")
>  box.execute("SELECT * FROM t WHERE b COLLATE \"binary\" = c;")
>  box.execute("SELECT * FROM t WHERE a = c;")
>  box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = c COLLATE \"unicode\";")
> +-- Make sure that using function featuring variable arguemnts
> +-- length  and resulting collation which depends on arguments
> +-- is processed correctly.
> +--
> +box.execute("SELECT * FROM t WHERE a COLLATE \"binary\" = substr();")
>