From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0ADFD46970E for ; Tue, 17 Dec 2019 16:30:16 +0300 (MSK) Date: Tue, 17 Dec 2019 16:30:16 +0300 From: Sergey Ostanevich Message-ID: <20191217133016.GB27451@tarantool.org> References: <1ca68695d7cd7d3d83f4b6829363ef533a23be38.1574846892.git.korablev@tarantool.org> <20191205114007.GA47637@tarantool.org> <23fe796e-4ea5-0438-78d6-ed9f1faafc89@tarantool.org> <20191206124829.GA48159@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191206124829.GA48159@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy 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();") >