From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D22132F092 for ; Sun, 2 Jun 2019 13:09:44 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZOnbMku2A6Ed for ; Sun, 2 Jun 2019 13:09:44 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 77A5C2EE9D for ; Sun, 2 Jun 2019 13:09:44 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: allow only for string-like args References: <015351a65570062bdd3577b7222f047f7de7e89b.1557312975.git.roman.habibov@tarantool.org> <1abf945a-736f-735a-98c5-a85db184ddb5@tarantool.org> <4389A940-D474-4977-A47A-FFBF809C3CC3@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6bb13018-d6f4-75b0-97bf-19a4d2613f38@tarantool.org> Date: Sun, 2 Jun 2019 19:09:39 +0200 MIME-Version: 1.0 In-Reply-To: <4389A940-D474-4977-A47A-FFBF809C3CC3@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Roman Khabibov , tarantool-patches@freelists.org Thanks for the fixes! See 8 comments below. >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >>> index ba7eea59d..29e3386fa 100644 >>> --- a/src/box/sql/expr.c >>> +++ b/src/box/sql/expr.c >>> @@ -4215,6 +4215,22 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) >>> } >>> case TK_SPAN: >>> case TK_COLLATE:{ >>> + enum field_type type; >>> + struct Expr *left = pExpr->pLeft; >>> + if (left->op == TK_COLUMN) { >>> + int col_num = left->iColumn; >>> + type = left->space_def->fields[col_num].type; >>> + } else >>> + type = left->type; >>> + if (left->op != TK_CONCAT && >> >> Why do you check for TK_CONCAT? Its type should be FIELD_TYPE_STRING. > Concatenations are rejected without this check. 1. It is not a good answer, when you can't explain, why you need a line of code, but a program does not work without it. Please, investigate and explain, why do you check for both 'type == STRING' and 'op == CONCAT'. If an op is CONCAT, then type is already STRING, so the additional check for 'op' should not be necessary. > > commit 5feaa9330f2c642fc16b479383b1c5cac96c28cc > Author: Roman Khabibov > Date: Mon May 6 14:30:21 2019 +0300 > > sql: allow only for string-like args > > Before this patch, user could use COLLATE with non-string-like > literals, columns or subquery results. Disallow such usage. > > Closes #3804 > > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index ba7eea59d..e558cbb18 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -197,6 +197,40 @@ sqlExprSkipCollate(Expr * pExpr) > return pExpr; > } > > +/* > + * Check that left node of @expr with the collation in the root 2. '@expr' is an invalid Doxygen command. If you want to reference a parameter, use <@a parameter_name>. See Doxygen documentation. > + * can be used with . If it is not, leave an error > + * message in pParse. 3. There is no argument 'pParse'. > + * > + * @param pParse Parser context. > + * @param expr Expression for checking. > + * > + * @retval 0 on success. > + * @retval -1 on error. > + */ > +static int > +check_collate_arg(Parse *parse, Expr *expr) 4. We use 'struct' prefix in all new SQL code. > +{ > + struct Expr *left = expr->pLeft; > + while (left->op == TK_COLLATE) > + left = left->pLeft; > + enum field_type type; > + if (left->op == TK_COLUMN) { > + int col_num = left->iColumn; > + type = left->space_def->fields[col_num].type; > + } else > + type = left->type; 5. We always either use '{}' for both 'if-else' parts, or do not use it at all. > + if (left->op != TK_CONCAT && type != FIELD_TYPE_STRING && > + type != FIELD_TYPE_SCALAR) { > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "COLLATE can't be used with " > + "non-string arguments"); > + parse->is_aborted = true; > + return -1; > + } > + return 0; > @@ -3935,6 +3969,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > testcase(op == TK_BITNOT); > assert(TK_NOT == OP_Not); > testcase(op == TK_NOT); > + if (pExpr->pLeft->op == TK_COLLATE && > + check_collate_arg(pParse, pExpr->pLeft) != 0) > + break; > r1 = sqlExprCodeTemp(pParse, pExpr->pLeft, > ®Free1); 6. Why do you check for COLLATE before CodeTemp()? This function does exactly the same check. > testcase(regFree1 == 0); > diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua > index c4724e636..566f2e723 100755 > --- a/test/sql-tap/e_select1.test.lua > +++ b/test/sql-tap/e_select1.test.lua > @@ -1938,7 +1938,7 @@ test:do_execsql_test( > test:do_execsql_test( > "e_select-8.9.2", > [[ > - SELECT x COLLATE "binary" FROM d4 ORDER BY 1 COLLATE "unicode_ci" > + SELECT x COLLATE "unicode_ci" FROM d4 ORDER BY 1 7. Why? The old test should pass as well. '1' here is not a value, but an index of a selected column. > ]], { > -- > "abc", "DEF", "ghi", "JKL" > diff --git a/test/sql-tap/selectE.test.lua b/test/sql-tap/selectE.test.lua > index 32c3d6a06..16075d38e 100755 > --- a/test/sql-tap/selectE.test.lua > +++ b/test/sql-tap/selectE.test.lua > @@ -57,8 +57,7 @@ test:do_test( > CREATE TABLE t3(a TEXT primary key); > INSERT INTO t3 VALUES('def'),('jkl'); > > - SELECT a FROM t1 EXCEPT SELECT a FROM t2 > - ORDER BY a COLLATE "unicode_ci"; > + SELECT a FROM t1 EXCEPT SELECT a FROM t2 ORDER BY a COLLATE "unicode_ci"; > ]] > end, { > -- > @@ -70,8 +69,7 @@ test:do_test( > "selectE-1.1", > function() > return test:execsql [[ > - SELECT a FROM t2 EXCEPT SELECT a FROM t3 > - ORDER BY a COLLATE "unicode_ci"; > + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "unicode_ci"; > ]] > end, { > -- > @@ -83,8 +81,7 @@ test:do_test( > "selectE-1.2", > function() > return test:execsql [[ > - SELECT a FROM t2 EXCEPT SELECT a FROM t3 > - ORDER BY a COLLATE "binary"; > + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "binary"; > ]] > end, { > -- > @@ -96,8 +93,7 @@ test:do_test( > "selectE-1.3", > function() > return test:execsql [[ > - SELECT a FROM t2 EXCEPT SELECT a FROM t3 > - ORDER BY a; > + SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a; > ]] > end, { > -- > @@ -113,8 +109,7 @@ test:do_test( > DELETE FROM t3; > INSERT INTO t2 VALUES('ABC'),('def'),('GHI'),('jkl'); > INSERT INTO t3 SELECT lower(a) FROM t2; > - SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3 > - ORDER BY 1 > + SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3 ORDER BY 1 > ]] > end, { 8. Why do you need changes in this file? You just made multiline requests one line.