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 C282624CA9 for ; Fri, 5 Jul 2019 21:04:59 -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 iWjFY2F6W0i8 for ; Fri, 5 Jul 2019 21:04:59 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 DAABA24CA5 for ; Fri, 5 Jul 2019 21:04:58 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 2/2 v2] sql: allow only for string-like args From: Roman Khabibov In-Reply-To: <2551430B-3DD4-4DAC-BC2B-7FA4F142A0FB@tarantool.org> Date: Sat, 6 Jul 2019 04:04:55 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3162540F-C878-4E46-879F-E0691CA5B775@tarantool.org> References: <91edd7e8b72a93c5b5c0592c181576fca98e66fd.1561372731.git.roman.habibov@tarantool.org> <23ABB827-11CA-4A4E-AB5C-839BCA8F3A50@tarantool.org> <2551430B-3DD4-4DAC-BC2B-7FA4F142A0FB@tarantool.org> 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: tarantool-patches@freelists.org Cc: "n. pettik" Hi! Thanks for the review. I decided to drop first commit. > On Jul 2, 2019, at 6:55 PM, n.pettik wrote: >=20 >=20 >>>> @@ -4396,6 +4433,8 @@ sqlExprCodeAtInit(Parse * pParse, /* = Parsing context */ >>>> int >>>> sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg) >>>> { >>>> + if (pExpr->op =3D=3D TK_COLLATE && check_collate_arg(pParse, = pExpr) !=3D 0) >>>> + return 0; >>>=20 >>> Why this check is required (in addition to the same check in = ExprCodeTarget)? >> It is needed e.g. for the following queries: =E2=80=9CSELECT NOT TRUE = COLLATE =E2=80=9Cunicode=E2=80=9D=E2=80=9D. >> =E2=80=9CNOT=E2=80=9D will be on the root. The sequence of calls: = sqlExprCodeTagret -> (case TK_NOT) >> -> sqlExprCodeTemp. >> NOT >> / \ >> unicode 0x0 >> / \ >> TRUE 0x0 >>>> int r2; >>>> pExpr =3D sqlExprSkipCollate(pExpr); >=20 > In fact, problem is here: this function skips collate token. > So, diff is following: >=20 > @@ -4427,10 +4426,7 @@ sqlExprCodeAtInit(Parse * pParse, /* = Parsing context */ > int > sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg) > { > - if (pExpr->op =3D=3D TK_COLLATE && check_collate_arg(pParse, = pExpr) !=3D 0) > - return 0; > int r2; > - pExpr =3D sqlExprSkipCollate(pExpr); > if (ConstFactorOk(pParse) > && pExpr->op !=3D TK_REGISTER && = sqlExprIsConstantNotJoin(pExpr) > ) { @@ -4397,7 +4427,6 @@ int sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg) { int r2; - pExpr =3D sqlExprSkipCollate(pExpr); if (ConstFactorOk(pParse) && pExpr->op !=3D TK_REGISTER && = sqlExprIsConstantNotJoin(pExpr) ) { > What is more, you don=E2=80=99t need to set type of expression > in sqlExprCodeTarget(): >=20 > @@ -4201,7 +4201,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) > } > case TK_SPAN: > case TK_COLLATE:{ > - pExpr->pLeft->type =3D = sql_expr_type(pExpr->pLeft); > if (check_collate_arg(pParse, pExpr) !=3D 0) > break; > return sqlExprCodeTarget(pParse, pExpr->pLeft, @@ -4173,6 +4201,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) } case TK_SPAN: case TK_COLLATE:{ + if (check_collate_arg(pParse, pExpr) !=3D 0) + break; return sqlExprCodeTarget(pParse, pExpr->pLeft, target); } >>>> diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua >>>> index 1ca6a6446..1fdee16b7 100755 >>>> --- a/test/sql-tap/in3.test.lua >>>> +++ b/test/sql-tap/in3.test.lua >>>> @@ -186,33 +186,6 @@ test:do_test( >>>> -- >>>> }) >>>>=20 >>>> - >>>> - >>>> --- The first of these queries has to use the temp-table, because = the=20 >>>> --- collation sequence used for the index on "t1.a" does not match = the >>>> --- collation sequence used by the "IN" comparison. The second does = not >>>> --- require a temp-table, because the collation sequences match. >>>> --- >>>> -test:do_test( >>>> - "in3-1.14", >>>> - function() >>>> - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE = \"unicode_ci\" IN (SELECT a FROM t1) ") >>>> - end, { >>>> - -- >>>> - 1, 1, 3, 5 >>>> - -- >>>> - }) >>>> - >>>> -test:do_test( >>>> - "in3-1.15", >>>> - function() >>>> - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE = \"binary\" IN (SELECT a FROM t1) ") >>>> - end, { >>>> - -- >>>> - 1, 1, 3, 5 >>>> - -- >>>> - }) >>>=20 >>> I=E2=80=99d better fix these tests rather than delete. >>=20 >> Previous test in this file will be same if I remove COLLATE clauses. >=20 > You don=E2=80=99t have to remove collate clause, you should create new > table with string field type and run this test using that table. > Comment above the test clearly indicates its purpose. diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua index 1ca6a6446..67883d2a0 100755 --- a/test/sql-tap/in3.test.lua +++ b/test/sql-tap/in3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(27) +test:plan(29) =20 --!./tcltestrunner.lua -- 2007 November 29 @@ -186,7 +186,17 @@ test:do_test( -- }) =20 - +test:do_execsql_test( + "in3-1.14", + [[ + CREATE TABLE t2(a TEXT PRIMARY KEY); + INSERT INTO t2 VALUES('A'); + INSERT INTO t2 VALUES('B'); + INSERT INTO t2 VALUES('C'); + ]], { + -- + -- + }) =20 -- The first of these queries has to use the temp-table, because the=20 -- collation sequence used for the index on "t1.a" does not match the @@ -194,23 +204,32 @@ test:do_test( -- require a temp-table, because the collation sequences match. -- test:do_test( - "in3-1.14", + "in3-1.15", function() - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE = \"unicode_ci\" IN (SELECT a FROM t1) ") + return exec_neph(" SELECT a FROM t2 WHERE a COLLATE = \"unicode_ci\" IN (SELECT a FROM t2) ") end, { - -- - 1, 1, 3, 5 - -- + -- + 1, "A", "B", "C" + -- }) =20 test:do_test( - "in3-1.15", + "in3-1.16", function() - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" = IN (SELECT a FROM t1) ") + return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"binary\" = IN (SELECT a FROM t2) ") end, { - -- - 1, 1, 3, 5 - -- + -- + 1, "A", "B", "C" + -- + }) + +test:do_execsql_test( + "in3-1.17", + [[ + DROP TABLE t2 + ]], { + -- + -- }) commit 7a000757faeab3e2fddefb02b3dc65ef6398c17f Author: Roman Khabibov Date: Mon May 6 14:30:21 2019 +0300 sql: allow only for string-like args =20 Before this patch, user could use COLLATE with non-string-like literals, columns or subquery results. Disallow such usage. =20 Closes #3804 diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 898d16cf3..96824e8e3 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -197,6 +197,34 @@ sqlExprSkipCollate(Expr * pExpr) return pExpr; } =20 +/* + * Check that left node of @a expr with the collation in the root + * can be used with . If it is not, leave an error + * message in pParse. + * + * @param parse Parser context. + * @param expr Expression for checking. + * + * @retval 0 on success. + * @retval -1 on error. + */ +static int +check_collate_arg(struct Parse *parse, struct Expr *expr) +{ + struct Expr *left =3D expr->pLeft; + while (left->op =3D=3D TK_COLLATE) + left =3D left->pLeft; + enum field_type type =3D sql_expr_type(left); + if (type !=3D FIELD_TYPE_STRING && type !=3D FIELD_TYPE_SCALAR) = { + diag_set(ClientError, ER_SQL_PARSER_GENERIC, + "COLLATE clause can't be used with non-string " + "arguments"); + parse->is_aborted =3D true; + return -1; + } + return 0; +} + int sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t = *coll_id, struct coll **coll) @@ -4173,6 +4201,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) } case TK_SPAN: case TK_COLLATE:{ + if (check_collate_arg(pParse, pExpr) !=3D 0) + break; return sqlExprCodeTarget(pParse, pExpr->pLeft, target); } @@ -4397,7 +4427,6 @@ int sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg) { int r2; - pExpr =3D sqlExprSkipCollate(pExpr); if (ConstFactorOk(pParse) && pExpr->op !=3D TK_REGISTER && = sqlExprIsConstantNotJoin(pExpr) ) { diff --git a/test/sql-tap/collation.test.lua = b/test/sql-tap/collation.test.lua index 79547361c..edf4a28b9 100755 --- a/test/sql-tap/collation.test.lua +++ b/test/sql-tap/collation.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(174) +test:plan(190) =20 local prefix =3D "collation-" =20 @@ -529,4 +529,87 @@ test:do_catchsql_test( 'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a = COLLATE foo, b, c))', {1, "Collation 'FOO' does not exist"}) =20 +-- gh-3805 Check COLLATE passing with string-like args only. + +test:do_execsql_test( + "collation-2.7.0", + [[ CREATE TABLE test1 (one INT PRIMARY KEY, two INT) ]], + {}) + +test:do_catchsql_test( + "collation-2.7.1", + 'SELECT one COLLATE "binary" FROM test1', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.2", + 'SELECT one COLLATE "unicode_ci" FROM test1', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.3", + 'SELECT two COLLATE "binary" FROM test1', + {1, "COLLATE clause can't be used with non-string arguments"}) + + +test:do_catchsql_test( + "collation-2.7.4", + 'SELECT (one + two) COLLATE "binary" FROM test1', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.5", + 'SELECT (SELECT one FROM test1) COLLATE "binary"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_execsql_test( + "collation-2.7.6", + 'SELECT TRIM(\'A\') COLLATE "binary"', + {"A"}) + +test:do_catchsql_test( + "collation-2.7.7", + 'SELECT RANDOM() COLLATE "binary"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.8", + 'SELECT LENGTH(\'A\') COLLATE "binary"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.9", + 'SELECT TRUE COLLATE "unicode"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.10", + 'SELECT NOT TRUE COLLATE "unicode"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.11", + 'SELECT TRUE AND TRUE COLLATE "unicode"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_catchsql_test( + "collation-2.7.12", + 'SELECT 1 | 1 COLLATE "unicode"', + {1, "COLLATE clause can't be used with non-string arguments"}) + +test:do_execsql_test( + "collation-2.7.14", + 'SELECT +\'str\' COLLATE "unicode"', + {"str"}) + +test:do_execsql_test( + "collation-2.7.15", + 'SELECT (\'con\'||\'cat\') COLLATE "unicode"', + {"concat"}) + +test:do_execsql_test( + "collation-2.7.16", + 'SELECT (SELECT \'str\') COLLATE "binary" COLLATE "binary";', + {"str"}) + test:finish_test() diff --git a/test/sql-tap/distinct.test.lua = b/test/sql-tap/distinct.test.lua index e6970084e..ae35a0db5 100755 --- a/test/sql-tap/distinct.test.lua +++ b/test/sql-tap/distinct.test.lua @@ -121,12 +121,12 @@ local data =3D { {"12.1", 0, "SELECT DISTINCT a, d FROM t1"}, {"12.2", 0, "SELECT DISTINCT a, d FROM t4"}, {"13.1", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM = t1"}, - {"13.2", 0, "SELECT DISTINCT a, b, c COLLATE \"unicode_ci\" FROM = t4"}, + {"13.2", 1, "SELECT DISTINCT a, b, c FROM t4"}, {"14.1", 0, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t1"}, {"14.2", 1, "SELECT DISTINCT a, d COLLATE \"unicode_ci\" FROM t4"}, {"15 ", 0, "SELECT DISTINCT a, d COLLATE \"binary\" FROM t1"}, {"16.1", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t1"}, - {"16.2", 0, "SELECT DISTINCT a, b, c COLLATE \"binary\" FROM t4"}, + {"16.2", 1, "SELECT DISTINCT a, b, c FROM t4"}, {"17", 0, --{ \/* Technically, it would be possible to detect = that DISTINCT\n ** is a no-op in cases like the following. = But sql does not\n ** do so. *\/\n "SELECT DISTINCT t1.id FROM t1, t2 WHERE t1.id=3Dt2.x" }, {"18 ", 1, "SELECT DISTINCT c1, c2 FROM t3"}, @@ -173,7 +173,7 @@ data =3D { {"a, b, c FROM t1", {"btree"}, {"A", "B", "C", "a", "b", "c"}}, {"a, b, c FROM t1 ORDER BY a, b, c", {"btree"}, {"A", "B", "C", = "a", "b", "c"}}, {"b FROM t1 WHERE a =3D 'a'", {}, {"b"}}, - {"b FROM t1 ORDER BY +b COLLATE \"binary\"", {"btree", "btree"}, = {"B", "b"}}, + {"b FROM t1 ORDER BY b COLLATE \"binary\"", {"btree", "btree"}, = {"B", "b"}}, {"a FROM t1", {}, {"A", "a"}}, {"b COLLATE \"unicode_ci\" FROM t1", {}, {"b"}}, {"b COLLATE \"unicode_ci\" FROM t1 ORDER BY b COLLATE = \"unicode_ci\"", {}, {"b"}}, diff --git a/test/sql-tap/identifier_case.test.lua = b/test/sql-tap/identifier_case.test.lua index 4db729f11..65ed9aea1 100755 --- a/test/sql-tap/identifier_case.test.lua +++ b/test/sql-tap/identifier_case.test.lua @@ -166,7 +166,7 @@ test:do_execsql_test( =20 test:do_execsql_test( test_prefix.."4.1", - string.format([[select * from table1 order by a collate = "unicode_ci"]]), + string.format([[select * from table1 order by a]]), {} ) =20 diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua index 1ca6a6446..67883d2a0 100755 --- a/test/sql-tap/in3.test.lua +++ b/test/sql-tap/in3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(27) +test:plan(29) =20 --!./tcltestrunner.lua -- 2007 November 29 @@ -186,7 +186,17 @@ test:do_test( -- }) =20 - +test:do_execsql_test( + "in3-1.14", + [[ + CREATE TABLE t2(a TEXT PRIMARY KEY); + INSERT INTO t2 VALUES('A'); + INSERT INTO t2 VALUES('B'); + INSERT INTO t2 VALUES('C'); + ]], { + -- + -- + }) =20 -- The first of these queries has to use the temp-table, because the=20 -- collation sequence used for the index on "t1.a" does not match the @@ -194,23 +204,32 @@ test:do_test( -- require a temp-table, because the collation sequences match. -- test:do_test( - "in3-1.14", + "in3-1.15", function() - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE = \"unicode_ci\" IN (SELECT a FROM t1) ") + return exec_neph(" SELECT a FROM t2 WHERE a COLLATE = \"unicode_ci\" IN (SELECT a FROM t2) ") end, { - -- - 1, 1, 3, 5 - -- + -- + 1, "A", "B", "C" + -- }) =20 test:do_test( - "in3-1.15", + "in3-1.16", function() - return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" = IN (SELECT a FROM t1) ") + return exec_neph(" SELECT a FROM t2 WHERE a COLLATE \"binary\" = IN (SELECT a FROM t2) ") end, { - -- - 1, 1, 3, 5 - -- + -- + 1, "A", "B", "C" + -- + }) + +test:do_execsql_test( + "in3-1.17", + [[ + DROP TABLE t2 + ]], { + -- + -- }) =20 -- Neither of these queries require a temp-table. The collation = sequence diff --git a/test/sql-tap/resolver01.test.lua = b/test/sql-tap/resolver01.test.lua index 9fcf0c7c0..315c892ac 100755 --- a/test/sql-tap/resolver01.test.lua +++ b/test/sql-tap/resolver01.test.lua @@ -106,7 +106,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.1", [[ - SELECT 2 AS y FROM t1, t2 ORDER BY y COLLATE "unicode_ci"; + SELECT 2 AS y FROM t1, t2 ORDER BY y; ]], { -- 0, {2} @@ -116,7 +116,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.2", [[ - SELECT 2 AS yy FROM t1, t2 ORDER BY y COLLATE "unicode_ci"; + SELECT 2 AS yy FROM t1, t2 ORDER BY y; ]], { -- 1, "ambiguous column name: Y" @@ -126,7 +126,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.3", [[ - SELECT x AS y FROM t3 ORDER BY y COLLATE "unicode_ci"; + SELECT x AS y FROM t3 ORDER BY y; ]], { -- 0, {11, 33} @@ -136,7 +136,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.4", [[ - SELECT x AS yy FROM t3 ORDER BY y COLLATE "unicode_ci"; + SELECT x AS yy FROM t3 ORDER BY y; ]], { -- 0, {33, 11} @@ -146,7 +146,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.5", [[ - SELECT x AS yy FROM t3 ORDER BY yy COLLATE "unicode_ci"; + SELECT x AS yy FROM t3 ORDER BY yy; ]], { -- 0, {11, 33} @@ -156,7 +156,7 @@ test:do_catchsql_test( test:do_catchsql_test( "resolver01-2.6", [[ - SELECT x AS yy FROM t3 ORDER BY 1 COLLATE "unicode_ci"; + SELECT x AS yy FROM t3 ORDER BY 1; ]], { -- 0, {11, 33} diff --git a/test/sql-tap/select1.test.lua = b/test/sql-tap/select1.test.lua index 6beeb34cb..6811f7dcb 100755 --- a/test/sql-tap/select1.test.lua +++ b/test/sql-tap/select1.test.lua @@ -1672,7 +1672,7 @@ test:do_execsql_test( test:do_execsql_test( "select1-10.7", [[ - SELECT f1 COLLATE "unicode_ci" AS x FROM test1 ORDER BY x + SELECT f1 AS x FROM test1 ORDER BY x ]], { -- 11, 33 diff --git a/test/sql-tap/tkt-b75a9ca6b0.test.lua = b/test/sql-tap/tkt-b75a9ca6b0.test.lua index 89817d2af..dde6ab502 100755 --- a/test/sql-tap/tkt-b75a9ca6b0.test.lua +++ b/test/sql-tap/tkt-b75a9ca6b0.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test =3D require("sqltester") -test:plan(22) +test:plan(20) =20 --!./tcltestrunner.lua -- 2014-04-21 @@ -57,7 +57,6 @@ local eqps =3D { {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x, y DESC", {1, 3, 2, = 2, 3, 1}, {idxscan, sort}}, {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x DESC, y DESC", {3, 1, = 2, 2, 1, 3}, {idxscan, sort}}, {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x ASC, y ASC", {1, 3, = 2, 2, 3, 1}, {idxscan}}, - {"SELECT x,y FROM t1 GROUP BY x, y ORDER BY x COLLATE = \"unicode_ci\", y", {1, 3, 2, 2, 3, 1}, {idxscan, sort}}, } for tn, val in ipairs(eqps) do local q =3D val[1] diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua index ec45e5e76..6985c589e 100755 --- a/test/sql-tap/with1.test.lua +++ b/test/sql-tap/with1.test.lua @@ -1043,7 +1043,7 @@ test:do_catchsql_test(13.3, [[ -- 2015-04-12 -- test:do_execsql_test(14.1, [[ - WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1 = COLLATE "binary"; + WITH x AS (SELECT * FROM t) SELECT 0 EXCEPT SELECT 0 ORDER BY 1; ]], { -- <14.1> =20