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 7B6CF266A8 for ; Mon, 4 Feb 2019 09:20:16 -0500 (EST) 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 kOnzX3BRm1Df for ; Mon, 4 Feb 2019 09:20:16 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F3C0D2653B for ; Mon, 4 Feb 2019 09:20:15 -0500 (EST) 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] sql: prohibit negative values under LIMIT clause From: "n.pettik" In-Reply-To: <20190204095108.18768-1-szudin@tarantool.org> Date: Mon, 4 Feb 2019 17:20:13 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190204095108.18768-1-szudin@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: szudin@tarantool.org > On 4 Feb 2019, at 12:51, Stanislav Zudin wrote: >=20 > If LIMIT or OFFSET expressions can be casted to the > negative integer value VDBE returns an error >=20 > Closes #3467 > --- > Branch: = https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negat= ive-limits > Issue: https://github.com/tarantool/tarantool/issues/3467 Travis status is negative: https://travis-ci.org/tarantool/tarantool/jobs/488414647 Seems like you forgot to drop table named =E2=80=99test=E2=80=99. Another problem concerns your approach. Just several examples: select * from t1 limit -0.5 select * from t1 limit 0.1 select * from t1 limit 'a' They return =E2=80=9C- error: datatype mismatch=E2=80=9D. On the other hand, queries like =E2=80=9Cselect * from t1 limit -1=E2=80=9D= returns error in format =E2=80=9Cerror: Only positive numbers allowed in the LIMIT = clause=E2=80=9D. This difference is likely to be misleading. It happens due to different handling of incorrect values under LIMIT = clause. See below how we can fix it. Yep, last two examples may seem to be out of scope of current issue, but anyway let's fix it right now. > src/box/sql/select.c | 40 +++++++ > test/sql-tap/e_select1.test.lua | 19 ++- > test/sql-tap/limit.test.lua | 167 ++++++++++++++++++++------- > test/sql-tap/orderby8.test.lua | 2 +- > test/sql-tap/select4.test.lua | 40 +++++-- > test/sql-tap/select6.test.lua | 10 +- > test/sql-tap/select8.test.lua | 2 +- > test/sql-tap/subquery2.test.lua | 2 +- > test/sql-tap/with1.test.lua | 6 +- > test/sql/gh-3467-bind-limit.result | 130 +++++++++++++++++++++ > test/sql/gh-3467-bind-limit.test.lua | 53 +++++++++ > 11 files changed, 395 insertions(+), 76 deletions(-) > create mode 100644 test/sql/gh-3467-bind-limit.result > create mode 100644 test/sql/gh-3467-bind-limit.test.lua >=20 > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 02ee225f1..2aeae96b3 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -2075,6 +2075,9 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) > int iLimit =3D 0; > int iOffset; > int n; > + const char *negativeLimitError =3D > + "Only positive numbers allowed " > + "in the LIMIT clause=E2=80=9D; In freshly added code we tend to use Tarantool codestyle even despite the fact that whole function is formatted in SQLite one. > if (p->iLimit) > return; >=20 > @@ -2098,6 +2101,13 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) > v =3D sqlite3GetVdbe(pParse); > assert(v !=3D 0); > if (sqlite3ExprIsInteger(p->pLimit, &n)) { > + if (n < 0) { > + sqlite3VdbeAddOp4(v, OP_Halt, > + SQLITE_MISMATCH, > + 0, 0, negativeLimitError, > + P4_STATIC); > + } Broken indentation: please, make tabs and spaces visible in your redactor and fix it. Also, you should pass SQL_TARANTOOL_ERROR instead of SQLITE_MISMATCH as an error code. Otherwise, you won=E2=80=99t get error if it appears during execution via iproto: tarantool> cn:execute('select * from t1 where id =3D -1') --- - metadata: - name: ID type: INTEGER rows: [] ... As you may see, query results in empty set, instead of raising an error. > + > sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit); > VdbeComment((v, "LIMIT counter")); > if (n =3D=3D 0) { > @@ -2108,8 +2118,21 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) > p->selFlags |=3D SF_FixedLimit; > } > } else { > + int lPosLimitValue =3D sqlite3VdbeMakeLabel(v); > sqlite3ExprCode(pParse, p->pLimit, iLimit); > sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit); > + /* If LIMIT clause >=3D 0 continue execution */ > + int r1 =3D sqlite3GetTempReg(pParse); > + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1); > + sqlite3VdbeAddOp3(v, OP_Ge, r1, lPosLimitValue, iLimit); > + /* Otherwise return an error and stop */ > + sqlite3VdbeAddOp4(v, OP_Halt, Again broken indents. You distinguish two cases: a) If value under limit clause is simple literal constant and its value is negative, VDBE program will be fated to fail (since there won=E2=80=99t be leap over HALT opcode). Thus, it makes no sense to continue generating any opcodes and execute VDBE program at all. You can simply stop query compilation right now. b) If value under limit clause is expression or non-integer literal we are going to compute it and test on int type by OP_MustBeInt (which doesn=E2=80=99t set an appropriate message). I suggest to unite these cases into one: always treat value limit clause as an expression, and compute it. Then, test with OP_MustBeInt. Note that it accepts second argument which points to address to be executed next in case of error. Then, comes check on that the value is positive. Overall, plan is following: sqlite3ExprCode(); OP_MustBeInt p1, halt_address OP_Integer OP_Ge =E2=80=A6 , pos_limit=20 halt_address: OP_Halt =E2=80=A6 =E2=80=9COnly positive integers are = allowed in the LIMIT clause=E2=80=9D. pos_limit: ... Such approach would allows us to display always the same error message for all wrong inputs and simplify code flow.=20 Please, consider this idea. > + = SQLITE_MISMATCH, > + 0, 0, = negativeLimitError, > + P4_STATIC); > + > + sqlite3VdbeResolveLabel(v, lPosLimitValue); > + sqlite3ReleaseTempReg(pParse, r1); > VdbeCoverage(v); > VdbeComment((v, "LIMIT counter")); > sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak); > @@ -2149,10 +2172,27 @@ computeLimitRegisters(Parse * pParse, Select * = p, int iBreak) > } > } > if (p->pOffset) { > + int lPosOffsetValue =3D sqlite3VdbeMakeLabel(v); > + const char *negativeOffsetError =3D > + "Only positive numbers allowed " > + "in the OFFSET clause"; > p->iOffset =3D iOffset =3D ++pParse->nMem; > pParse->nMem++; /* Allocate an extra register = for limit+offset */ > sqlite3ExprCode(pParse, p->pOffset, iOffset); > sqlite3VdbeAddOp1(v, OP_MustBeInt, iOffset); > + /* If OFFSET clause >=3D 0 continue execution */ > + int r1 =3D sqlite3GetTempReg(pParse); > + sqlite3VdbeAddOp2(v, OP_Integer, 0, r1); > + > + sqlite3VdbeAddOp3(v, OP_Ge, r1, lPosOffsetValue, = iOffset); > + /* Otherwise return an error and stop */ > + sqlite3VdbeAddOp4(v, OP_Halt, > + = SQLITE_MISMATCH, > + 0, 0, = negativeOffsetError, > + P4_STATIC); > + > + sqlite3VdbeResolveLabel(v, lPosOffsetValue); > + sqlite3ReleaseTempReg(pParse, r1); > VdbeCoverage(v); > VdbeComment((v, "OFFSET counter")); > sqlite3VdbeAddOp3(v, OP_OffsetLimit, iLimit, > diff --git a/test/sql-tap/e_select1.test.lua = b/test/sql-tap/e_select1.test.lua > index 464233e56..b419ca640 100755 > --- a/test/sql-tap/e_select1.test.lua > +++ b/test/sql-tap/e_select1.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(523) > +test:plan(520) >=20 > --!./tcltestrunner.lua > -- 2010 July 16 > @@ -2186,9 +2186,9 @@ end > test:do_select_tests( > "e_select-9.4", > { > - {"1", "SELECT b FROM f1 ORDER BY a LIMIT -1 ", {"a", "b", = "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", = "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"}}, > - {"2", "SELECT b FROM f1 ORDER BY a LIMIT length('abc')-100 ", = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", = "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", = "z"}}, > - {"3", "SELECT b FROM f1 ORDER BY a LIMIT (SELECT count(*) = FROM f1)/2 - 14 ", {"a", "b", "c", "d", "e", "f", "g", "h", "i", = "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", = "w", "x", "y", "z"}}, > + {"1", "SELECT b FROM f1 ORDER BY a LIMIT 10000 ", {"a", "b", = "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", = "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"}}, > + {"2", "SELECT b FROM f1 ORDER BY a LIMIT = 123+length('abc')-100 ", {"a", "b", "c", "d", "e", "f", "g", "h", = "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", = "u", "v", "w", "x", "y", "z"}}, > + {"3", "SELECT b FROM f1 ORDER BY a LIMIT (SELECT count(*) = FROM f1)/2 - 10 ", {"a", "b", "c"}}, > }) >=20 > -- EVIDENCE-OF: R-33750-29536 Otherwise, the SELECT returns the first = N > @@ -2270,9 +2270,9 @@ test:do_select_tests( > test:do_select_tests( > "e_select-9.10", > { > - {"1", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET -1 ", {"a", = "b", "c", "d", "e"}}, > - {"2", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET -500 ", = {"a", "b", "c", "d", "e"}}, > - {"3", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET 0 ", {"a", = "b", "c", "d", "e"}}, > + {"1", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET 0 ", {"a", = "b", "c", "d", "e"}}, > +-- {"2", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET -500 ", = {"a", "b", "c", "d", "e"}}, > +-- {"3", "SELECT b FROM f1 ORDER BY a LIMIT 5 OFFSET -1 ", = {"a", "b", "c", "d", "e"}}, Don=E2=80=99t comment tests: either delete them or fix. > diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua > index 97585c13b..1556cce3c 100755 > --- a/test/sql-tap/with1.test.lua > +++ b/test/sql-tap/with1.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test =3D require("sqltester") > -test:plan(67) > +test:plan(65) >=20 > --!./tcltestrunner.lua > -- 2014 January 11 > @@ -663,8 +663,8 @@ limit_test(9.4, 20, -1) > limit_test(9.5, 5, 5) > limit_test(9.6, 0, -1) > limit_test(9.7, 40, -1) > -limit_test(9.8, -1, -1) > -limit_test(9.9, -1, -1) > +-- limit_test(9.8, -1, -1) > +-- limit_test(9.9, -1, -1) The same (and in all other places): delete or fix test case. > -- = #-------------------------------------------------------------------------= - > -- # Test the ORDER BY clause on recursive tables. > -- # > diff --git a/test/sql/gh-3467-bind-limit.test.lua = b/test/sql/gh-3467-bind-limit.test.lua > new file mode 100644 > index 000000000..6c4704970 > --- /dev/null > +++ b/test/sql/gh-3467-bind-limit.test.lua There is already existing sql/iproto.test.lua which is aimed at testing SQL bindings. Please, put these tests to that file. > @@ -0,0 +1,53 @@ > +-- > +-- Created by IntelliJ IDEA. > +-- User: szudin > +-- Date: 04.02.19 > +-- Time: 12:26 > +-- To change this template use File | Settings | File Templates. Don=E2=80=99t put such auto-generated headers in files next time. > +-- > +remote =3D require('net.box') > +test_run =3D require('test_run').new() > +engine =3D test_run:get_cfg('engine') > +box.sql.execute('pragma sql_default_engine=3D\''..engine..'\'') > + > +box.sql.execute('create table test (id int primary key, a float, b = text)') > + > +-- > +-- setup permissions > +-- > +box.sql.execute('select * from test') > +box.schema.user.grant('guest','read,write,execute', 'universe') > +box.schema.user.grant('guest', 'create', 'space') > + > +-- > +-- > +cn =3D remote.connect(box.cfg.listen) > +cn:ping() > + > + > +-- Simple select. > +ret =3D cn:execute('select * from test') > +ret Note that you don=E2=80=99t really need that =E2=80=98ret=E2=80=99 val. The same result set will be displayed for simple cn:execute(=E2=80=99select * from test') > + > +cn:execute('insert into test values (1, 1.1, \'preved\')') > +cn:execute('insert into test values (2, 2.2, \'medved\')') > +cn:execute('insert into test values (3, 3.3, \'bread\')') > +cn:execute('insert into test values (4, 4.4, \'honey\')') > +cn:execute('insert into test values (5, 5.5, \'beer\')') > + > +-- > +-- Parmaeters bindig. > +-- > +cn:execute('select * from test where id =3D ?', {1}) > +cn:execute('select ?, ?, ?', {1, 2, 3}) These queries don=E2=80=99t really test anything, just wasting execution time. > +cn:execute('select * from test limit ?', {2}) This test again doesn=E2=80=99t test that negative values are banned for now. I would put some extra cases like: select * from test limit ?, {-0.5} -0.5 is a negative number too. And so forth.