[tarantool-patches] Re: [PATCH] sql: prohibit negative values under LIMIT clause

["n.pettik" <korablev@tarantool.org>]

> On 4 Feb 2019, at 12:51, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> If LIMIT or OFFSET expressions can be casted to the
> negative integer value VDBE returns an error
> 
> Closes #3467
> ---
> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-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 ’test’.

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 “- error: datatype mismatch”.

On the other hand, queries like “select * from t1 limit -1” returns error
in format “error: Only positive numbers allowed in the LIMIT clause”.
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
> 
> 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 = 0;
> 	int iOffset;
> 	int n;
> +	const char *negativeLimitError =
> +			"Only positive numbers allowed "
> +			"in the LIMIT clause”;

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;
> 
> @@ -2098,6 +2101,13 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 		v = sqlite3GetVdbe(pParse);
> 		assert(v != 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’t get
error if it appears during execution via iproto:

tarantool> cn:execute('select * from t1 where id = -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 == 0) {
> @@ -2108,8 +2118,21 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 				p->selFlags |= SF_FixedLimit;
> 			}
> 		} else {
> +			int lPosLimitValue = sqlite3VdbeMakeLabel(v);
> 			sqlite3ExprCode(pParse, p->pLimit, iLimit);
> 			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
> +			/* If LIMIT clause >= 0 continue execution */
> +            int r1 = 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’t 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’t 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 … , pos_limit 
halt_address: OP_Halt … “Only positive integers are allowed in the LIMIT clause”.
pos_limit: ...

Such approach would allows us to display always the same error
message for all wrong inputs and simplify code flow. 

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 = sqlite3VdbeMakeLabel(v);
> +			const char *negativeOffsetError =
> +					"Only positive numbers allowed "
> +					"in the OFFSET clause";
> 			p->iOffset = iOffset = ++pParse->nMem;
> 			pParse->nMem++;	/* Allocate an extra register for limit+offset */
> 			sqlite3ExprCode(pParse, p->pOffset, iOffset);
> 			sqlite3VdbeAddOp1(v, OP_MustBeInt, iOffset);
> +			/* If OFFSET clause >= 0 continue execution */
> +            int r1 = 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 = require("sqltester")
> -test:plan(523)
> +test:plan(520)
> 
> --!./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"}},
>     })
> 
> -- 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’t 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 = require("sqltester")
> -test:plan(67)
> +test:plan(65)
> 
> --!./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’t put such auto-generated headers in files next time.

> +--
> +remote = require('net.box')
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.sql.execute('pragma sql_default_engine=\''..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 = remote.connect(box.cfg.listen)
> +cn:ping()
> +
> +
> +-- Simple select.
> +ret = cn:execute('select * from test')
> +ret

Note that you don’t really need that ‘ret’ val.
The same result set will be displayed for simple
cn:execute(’select * 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 = ?', {1})
> +cn:execute('select ?, ?, ?', {1, 2, 3})

These queries don’t really test anything, just
wasting execution time.

> +cn:execute('select * from test limit ?', {2})

This test again doesn’t 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.