From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 0A3CF46971A for ; Fri, 6 Dec 2019 15:48:30 +0300 (MSK) Date: Fri, 6 Dec 2019 15:48:29 +0300 From: Nikita Pettik Message-ID: <20191206124829.GA48159@tarantool.org> References: <1ca68695d7cd7d3d83f4b6829363ef533a23be38.1574846892.git.korablev@tarantool.org> <20191205114007.GA47637@tarantool.org> <23fe796e-4ea5-0438-78d6-ed9f1faafc89@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <23fe796e-4ea5-0438-78d6-ed9f1faafc89@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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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();")