From: Nikita Pettik <korablev@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll() Date: Fri, 6 Dec 2019 15:48:29 +0300 [thread overview] Message-ID: <20191206124829.GA48159@tarantool.org> (raw) In-Reply-To: <23fe796e-4ea5-0438-78d6-ed9f1faafc89@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();")
next prev parent reply other threads:[~2019-12-06 12:48 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-27 12:15 [Tarantool-patches] [PATCH 0/6] sql: extend response metadata Nikita Pettik 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 1/6] sql: refactor resulting set metadata Nikita Pettik 2019-11-28 22:41 ` Vladislav Shpilevoy 2019-12-05 11:39 ` Nikita Pettik 2019-12-05 23:58 ` Vladislav Shpilevoy 2019-12-06 12:48 ` Nikita Pettik 2019-12-17 13:23 ` Sergey Ostanevich 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll() Nikita Pettik 2019-11-28 22:42 ` Vladislav Shpilevoy 2019-12-05 11:40 ` Nikita Pettik 2019-12-05 23:59 ` Vladislav Shpilevoy 2019-12-06 12:48 ` Nikita Pettik [this message] 2019-12-17 13:30 ` Sergey Ostanevich 2019-12-17 14:44 ` Nikita Pettik 2019-12-17 19:53 ` Nikita Pettik 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 3/6] sql: extend result set with collation Nikita Pettik 2019-11-28 22:41 ` Vladislav Shpilevoy 2019-12-05 11:50 ` Nikita Pettik 2019-12-18 11:08 ` Sergey Ostanevich 2019-12-24 0:44 ` Nikita Pettik 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 4/6] sql: extend result set with nullability Nikita Pettik 2019-11-28 22:41 ` Vladislav Shpilevoy 2019-12-05 11:50 ` Nikita Pettik 2019-12-06 0:00 ` Vladislav Shpilevoy 2019-12-06 12:49 ` Nikita Pettik 2019-12-18 13:31 ` Sergey Ostanevich 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 5/6] sql: extend result set with autoincrement Nikita Pettik 2019-11-28 22:41 ` Vladislav Shpilevoy 2019-12-05 11:51 ` Nikita Pettik 2019-12-18 15:17 ` Sergey Ostanevich 2019-12-24 0:47 ` Nikita Pettik 2019-11-27 12:15 ` [Tarantool-patches] [PATCH 6/6] sql: extend result set with alias Nikita Pettik 2019-11-28 22:41 ` Vladislav Shpilevoy 2019-12-05 11:51 ` Nikita Pettik 2019-12-06 0:02 ` Vladislav Shpilevoy 2019-12-06 12:50 ` Nikita Pettik 2019-12-06 21:52 ` Vladislav Shpilevoy 2019-12-19 15:17 ` Sergey Ostanevich 2019-12-24 0:27 ` Nikita Pettik 2019-11-28 22:55 ` [Tarantool-patches] [PATCH 0/6] sql: extend response metadata Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191206124829.GA48159@tarantool.org \ --to=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/6] sql: fix possible null dereference in sql_expr_coll()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox