Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v2] sql: prohibit negative values under LIMIT clause
@ 2019-02-05  8:56 Stanislav Zudin
  2019-02-05 15:15 ` [tarantool-patches] " n.pettik
  2019-02-07  8:53 ` Kirill Yukhin
  0 siblings, 2 replies; 5+ messages in thread
From: Stanislav Zudin @ 2019-02-05  8:56 UTC (permalink / raw)
  To: tarantool-patches, korablev; +Cc: Stanislav Zudin

If LIMIT or OFFSET expressions can be casted to the
negative integer value VDBE returns an error.
If expression in the LIMIT clause can't be converted
into integer without data loss the VDBE instead of
SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
message "Only positive numbers are allowed in the
LIMIT clause". The same for OFFSET clause.

Closes #3467
---
Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-limits
Issue: https://github.com/tarantool/tarantool/issues/3467

 src/box/sql/select.c            |  66 ++++++++----
 test/sql-tap/e_select1.test.lua |  21 ++--
 test/sql-tap/eqp.test.lua       |   6 +-
 test/sql-tap/limit.test.lua     | 171 +++++++++++++++++++++++---------
 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     |   4 +-
 test/sql/iproto.result          |  57 ++++++++++-
 test/sql/iproto.test.lua        |  12 ++-
 12 files changed, 288 insertions(+), 105 deletions(-)

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 02ee225f1..a8ed3c346 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 	Vdbe *v = 0;
 	int iLimit = 0;
 	int iOffset;
-	int n;
+	const char *negativeLimitError = "Only positive numbers are allowed "
+					 "in the LIMIT clause";
 	if (p->iLimit)
 		return;
 
@@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 		p->iLimit = iLimit = ++pParse->nMem;
 		v = sqlite3GetVdbe(pParse);
 		assert(v != 0);
-		if (sqlite3ExprIsInteger(p->pLimit, &n)) {
-			sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit);
-			VdbeComment((v, "LIMIT counter"));
-			if (n == 0) {
-				sqlite3VdbeGoto(v, iBreak);
-			} else if (n >= 0
-				   && p->nSelectRow > sqlite3LogEst((u64) n)) {
-				p->nSelectRow = sqlite3LogEst((u64) n);
-				p->selFlags |= SF_FixedLimit;
-			}
-		} else {
-			sqlite3ExprCode(pParse, p->pLimit, iLimit);
-			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
-			VdbeCoverage(v);
-			VdbeComment((v, "LIMIT counter"));
-			sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
-			VdbeCoverage(v);
-		}
+		int lPosLimitValue = sqlite3VdbeMakeLabel(v);
+		int labelHalt = sqlite3VdbeMakeLabel(v);
+		sqlite3ExprCode(pParse, p->pLimit, iLimit);
+		sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt);
+		/* 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 */
+		sqlite3VdbeResolveLabel(v, labelHalt);
+		sqlite3VdbeAddOp4(v, OP_Halt,
+				  SQL_TARANTOOL_ERROR,
+				  0, 0,
+				  negativeLimitError,
+				  P4_STATIC);
+
+		sqlite3VdbeResolveLabel(v, lPosLimitValue);
+		sqlite3ReleaseTempReg(pParse, r1);
+		VdbeCoverage(v);
+		VdbeComment((v, "LIMIT counter"));
+		sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
+		VdbeCoverage(v);
+
 		if ((p->selFlags & SF_SingleRow) != 0) {
 			if (ExprHasProperty(p->pLimit, EP_System)) {
 				/*
@@ -2149,10 +2155,30 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 			}
 		}
 		if (p->pOffset) {
+			int lPosOffsetValue = sqlite3VdbeMakeLabel(v);
+			int labelHalt = sqlite3VdbeMakeLabel(v);
+			const char *negativeOffsetError =
+					"Only positive numbers are 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);
+			sqlite3VdbeAddOp2(v, OP_MustBeInt, iOffset, labelHalt);
+			/* 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 */
+			sqlite3VdbeResolveLabel(v, labelHalt);
+			sqlite3VdbeAddOp4(v, OP_Halt,
+					  SQL_TARANTOOL_ERROR,
+					  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..507d28fe2 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
@@ -2176,7 +2176,7 @@ for _, val in ipairs({
         "e_select-9.2."..tn,
         select,
         {
-            1, "datatype mismatch"})
+            1, "Only positive numbers are allowed in the LIMIT clause"})
 end
 
 -- EVIDENCE-OF: R-03014-26414 If the LIMIT expression evaluates to a
@@ -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
@@ -2230,7 +2230,7 @@ for _, val in ipairs({
     test:do_catchsql_test(
         "e_select-9.7."..tn,
         select, {
-            1, "datatype mismatch"
+            1, "Only positive numbers are allowed in the OFFSET clause"
         })
 
 end
@@ -2270,9 +2270,7 @@ 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"}},
     })
 
 -- EVIDENCE-OF: R-19509-40356 Instead of a separate OFFSET clause, the
@@ -2293,9 +2291,8 @@ test:do_select_tests(
         {"7", "SELECT b FROM f1 ORDER BY a LIMIT '1'||'5', 3 ", {"p", "q", "r"}},
         {"8", "SELECT b FROM f1 ORDER BY a LIMIT 20, 10 ", {"u", "v", "w", "x", "y", "z"}},
         {"9", "SELECT a FROM f1 ORDER BY a DESC LIMIT 18+4, 100 ", {4, 3, 2, 1}},
-        {"10", "SELECT b FROM f1 ORDER BY a LIMIT -1, 5 ", {"a", "b", "c", "d", "e"}},
-        {"11", "SELECT b FROM f1 ORDER BY a LIMIT -500, 5 ", {"a", "b", "c", "d", "e"}},
-        {"12", "SELECT b FROM f1 ORDER BY a LIMIT 0, 5 ", {"a", "b", "c", "d", "e"}},
+        {"10", "SELECT b FROM f1 ORDER BY a LIMIT 0, 5 ", {"a", "b", "c", "d", "e"}},
+        {"11", "SELECT b FROM f1 ORDER BY a LIMIT 500-500, 5 ", {"a", "b", "c", "d", "e"}},
     })
 
 test:finish_test()
diff --git a/test/sql-tap/eqp.test.lua b/test/sql-tap/eqp.test.lua
index b52dff033..7f37f4ae6 100755
--- a/test/sql-tap/eqp.test.lua
+++ b/test/sql-tap/eqp.test.lua
@@ -140,9 +140,9 @@ test:do_eqp_test(
         -- <1.9>
         {3, 0, 0, "SCAN TABLE T3"},
         {1, 0, 0, "COMPOUND SUBQUERIES 2 AND 3 USING TEMP B-TREE (EXCEPT)"},
-        {0, 0, 1, "SCAN SUBQUERY 1"},
-        {0, 1, 0, "SCAN TABLE T3"},
-        
+        {0, 0, 0,"SCAN TABLE T3"},
+        {0, 1, 1,"SCAN SUBQUERY 1"},
+
         -- </1.9>
     })
 
diff --git a/test/sql-tap/limit.test.lua b/test/sql-tap/limit.test.lua
index 062ba4e38..a94a8d10f 100755
--- a/test/sql-tap/limit.test.lua
+++ b/test/sql-tap/limit.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(103)
+test:plan(111)
 
 --!./tcltestrunner.lua
 -- 2001 November 6
@@ -78,56 +78,75 @@ test:do_execsql_test(
         -- </limit-1.2.2>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "limit-1.2.3",
     [[
         SELECT x FROM t1 ORDER BY x+1 LIMIT 5 OFFSET -2
     ]], {
-        -- <limit-1.2.3>
-        0, 1, 2, 3, 4
-        -- </limit-1.2.3>
+        -- <limit-1.2.13>
+        1 ,"Only positive numbers are allowed in the OFFSET clause"
+        -- </limit-1.2.13>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "limit-1.2.4",
     [[
         SELECT x FROM t1 ORDER BY x+1 LIMIT 2, -5
     ]], {
         -- <limit-1.2.4>
-        2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
-        20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
+        1, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-1.2.4>
     })
 
 test:do_execsql_test(
     "limit-1.2.5",
     [[
-        SELECT x FROM t1 ORDER BY x+1 LIMIT -2, 5
+        SELECT x FROM t1 ORDER BY x+1 LIMIT 2, 1000
     ]], {
-        -- <limit-1.2.5>
-        0, 1, 2, 3, 4
-        -- </limit-1.2.5>
-    })
+    -- <limit-1.2.5>
+    2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
+    20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
+    -- </limit-1.2.5>
+})
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "limit-1.2.6",
     [[
-        SELECT x FROM t1 ORDER BY x+1 LIMIT -2, -5
+        SELECT x FROM t1 ORDER BY x+1 LIMIT -2, 5
     ]], {
         -- <limit-1.2.6>
-        0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
-        20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31
+        1, "Only positive numbers are allowed in the OFFSET clause"
         -- </limit-1.2.6>
     })
 
 test:do_execsql_test(
     "limit-1.2.7",
+    [[
+        SELECT x FROM t1 ORDER BY x+1 LIMIT 0, 5
+    ]], {
+    -- <limit-1.2.7>
+    0, 1, 2, 3, 4
+    -- </limit-1.2.7>
+})
+
+test:do_catchsql_test(
+    "limit-1.2.8",
+    [[
+        SELECT x FROM t1 ORDER BY x+1 LIMIT -2, -5
+    ]], {
+        -- <limit-1.2.8>
+        1, "Only positive numbers are allowed in the LIMIT clause"
+        -- </limit-1.2.8>
+    })
+
+test:do_execsql_test(
+    "limit-1.2.9",
     [[
         SELECT x FROM t1 ORDER BY x LIMIT 2, 5
     ]], {
-        -- <limit-1.2.7>
+        -- <limit-1.2.9>
         2, 3, 4, 5, 6
-        -- </limit-1.2.7>
+        -- </limit-1.2.9>
     })
 
 test:do_execsql_test(
@@ -359,56 +378,96 @@ test:do_execsql_test(
         -- </limit-6.1>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "limit-6.2",
     [[
         SELECT * FROM t6 LIMIT -1 OFFSET -1;
     ]], {
         -- <limit-6.2>
-        1, 2, 3, 4
+        1, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-6.2>
     })
 
-test:do_execsql_test(
-    "limit-6.3",
+test:do_catchsql_test(
+    "limit-6.3.1",
     [[
         SELECT * FROM t6 LIMIT 2 OFFSET -123;
     ]], {
         -- <limit-6.3>
-        1, 2
+        1, "Only positive numbers are allowed in the OFFSET clause"
         -- </limit-6.3>
     })
 
 test:do_execsql_test(
-    "limit-6.4",
+    "limit-6.3.2",
+    [[
+        SELECT * FROM t6 LIMIT 2 OFFSET 0;
+    ]], {
+    -- <limit-6.3>
+    1, 2
+    -- </limit-6.3>
+})
+
+test:do_catchsql_test(
+    "limit-6.4.1",
     [[
         SELECT * FROM t6 LIMIT -432 OFFSET 2;
     ]], {
         -- <limit-6.4>
-        3, 4
+        1, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-6.4>
     })
 
 test:do_execsql_test(
-    "limit-6.5",
+    "limit-6.4.2",
+    [[
+        SELECT * FROM t6 LIMIT 1000 OFFSET 2;
+    ]], {
+    -- <limit-6.4>
+    3, 4
+    -- </limit-6.4>
+})
+
+test:do_catchsql_test(
+    "limit-6.5.1",
     [[
         SELECT * FROM t6 LIMIT -1
     ]], {
         -- <limit-6.5>
-        1, 2, 3, 4
+        1, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-6.5>
     })
 
 test:do_execsql_test(
-    "limit-6.6",
+    "limit-6.5.2",
+    [[
+        SELECT * FROM t6 LIMIT '12'
+    ]], {
+    -- <limit-6.5>
+    1, 2, 3, 4
+    -- </limit-6.5>
+})
+
+test:do_catchsql_test(
+    "limit-6.6.1",
     [[
         SELECT * FROM t6 LIMIT -1 OFFSET 1
     ]], {
         -- <limit-6.6>
-        2, 3, 4
+        1, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-6.6>
     })
 
+test:do_execsql_test(
+    "limit-6.6.2",
+    [[
+        SELECT * FROM t6 LIMIT 111 OFFSET 1
+    ]], {
+    -- <limit-6.6>
+    2, 3, 4
+    -- </limit-6.6>
+})
+
 test:do_execsql_test(
     "limit-6.7",
     [[
@@ -660,13 +719,13 @@ test:do_test(
 test:do_test(
     "limit-10.3",
     function()
-        local limit = -1
+        local limit = 111
         return test:execsql("SELECT x FROM t1 WHERE x<10 LIMIT "..limit)
     end, {
-        -- <limit-10.3>
-        9, 8, 7, 6, 5, 4, 3, 2, 1, 0
-        -- </limit-10.3>
-    })
+    -- <limit-10.3.2>
+    9, 8, 7, 6, 5, 4, 3, 2, 1, 0
+    -- </limit-10.3.2>
+})
 
 test:do_test(
     "limit-10.4",
@@ -677,7 +736,7 @@ test:do_test(
         end)}
     end, {
         -- <limit-10.4>
-        0, "datatype mismatch"
+        0, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-10.4>
     })
 
@@ -690,7 +749,7 @@ test:do_test(
         end)}
     end, {
         -- <limit-10.5>
-        0, "datatype mismatch"
+        0, "Only positive numbers are allowed in the LIMIT clause"
         -- </limit-10.5>
     })
 
@@ -1259,24 +1318,44 @@ test:do_execsql_test(
         -- </limit-14.4>
     })
 
-test:do_execsql_test(
-    "limit-14.6",
+test:do_catchsql_test(
+    "limit-14.6.1",
     [[
         SELECT 123 LIMIT -1 OFFSET 0
     ]], {
-        -- <limit-14.6>
-        123
-        -- </limit-14.6>
+        -- <limit-14.6.1>
+        1, "Only positive numbers are allowed in the LIMIT clause"
+        -- </limit-14.6.1>
     })
 
 test:do_execsql_test(
-    "limit-14.7",
+    "limit-14.6.2",
+    [[
+        SELECT 123 LIMIT 21 OFFSET 0
+    ]], {
+    -- <limit-14.6.2>
+    123
+    -- </limit-14.6.2>
+})
+
+test:do_catchsql_test(
+    "limit-14.7.1",
     [[
         SELECT 123 LIMIT -1 OFFSET 1
     ]], {
-        -- <limit-14.7>
-        
-        -- </limit-14.7>
+        -- <limit-14.7.1>
+        1, "Only positive numbers are allowed in the LIMIT clause"
+        -- </limit-14.7.1>
     })
 
+test:do_execsql_test(
+    "limit-14.7.2",
+    [[
+        SELECT 123 LIMIT 111 OFFSET 1
+    ]], {
+    -- <limit-14.7.2>
+
+    -- </limit-14.7.2>
+})
+
 test:finish_test()
diff --git a/test/sql-tap/orderby8.test.lua b/test/sql-tap/orderby8.test.lua
index 63ec6da1c..55b944230 100755
--- a/test/sql-tap/orderby8.test.lua
+++ b/test/sql-tap/orderby8.test.lua
@@ -44,7 +44,7 @@ for i=1,200 do
     rs = rs..", x+"..i
     test:do_execsql_test(
         "1."..i,
-        "SELECT x FROM (SELECT "..rs.." FROM t1 ORDER BY x LIMIT -1)",
+        "SELECT x FROM (SELECT "..rs.." FROM t1 ORDER BY x LIMIT 100)",
         {
             1, 2, 3, 4, 5, 6, 7, 8, 9
     })
diff --git a/test/sql-tap/select4.test.lua b/test/sql-tap/select4.test.lua
index ebe8cd4ca..49d5bf154 100755
--- a/test/sql-tap/select4.test.lua
+++ b/test/sql-tap/select4.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(103)
+test:plan(105)
 
 --!./tcltestrunner.lua
 -- 2001 September 15
@@ -984,25 +984,43 @@ test:do_execsql_test(
         -- </select4-10.3>
     })
 
-test:do_execsql_test(
-    "select4-10.4",
+test:do_catchsql_test(
+    "select4-10.4.1",
     [[
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1
     ]], {
-        -- <select4-10.4>
-        0, 1, 2, 3, 4, 5
-        -- </select4-10.4>
+        -- <select4-10.4.1>
+    1,"Only positive numbers are allowed in the LIMIT clause"
+        -- </select4-10.4.1>
     })
-
 test:do_execsql_test(
-    "select4-10.5",
+    "select4-10.4.2",
+    [[
+        SELECT DISTINCT log FROM t1 ORDER BY log LIMIT 111
+    ]], {
+    -- <select4-10.4.2>
+    0, 1, 2, 3, 4, 5
+    -- </select4-10.4.2>
+})
+
+test:do_catchsql_test(
+    "select4-10.5.1",
     [[
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1 OFFSET 2
     ]], {
-        -- <select4-10.5>
-        2, 3, 4, 5
-        -- </select4-10.5>
+        -- <select4-10.5.1>
+        1,"Only positive numbers are allowed in the LIMIT clause"
+        -- </select4-10.5.1>
     })
+test:do_execsql_test(
+    "select4-10.5.2",
+    [[
+        SELECT DISTINCT log FROM t1 ORDER BY log LIMIT 111 OFFSET 2
+    ]], {
+    -- <select4-10.5.2>
+    2, 3, 4, 5
+    -- </select4-10.5.2>
+})
 
 test:do_execsql_test(
     "select4-10.6",
diff --git a/test/sql-tap/select6.test.lua b/test/sql-tap/select6.test.lua
index 9a0fe6efb..7f174f04f 100755
--- a/test/sql-tap/select6.test.lua
+++ b/test/sql-tap/select6.test.lua
@@ -842,7 +842,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select6-9.7",
     [[
-        SELECT x FROM (SELECT x FROM t1 LIMIT -1) LIMIT 3;
+        SELECT x FROM (SELECT x FROM t1 LIMIT 101) LIMIT 3;
     ]], {
         -- <select6-9.7>
         1, 2, 3
@@ -852,7 +852,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select6-9.8",
     [[
-        SELECT x FROM (SELECT x FROM t1 LIMIT -1);
+        SELECT x FROM (SELECT x FROM t1 LIMIT 101);
     ]], {
         -- <select6-9.8>
         1, 2, 3, 4
@@ -862,7 +862,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select6-9.9",
     [[
-        SELECT x FROM (SELECT x FROM t1 LIMIT -1 OFFSET 1);
+        SELECT x FROM (SELECT x FROM t1 LIMIT 10-1 OFFSET 1);
     ]], {
         -- <select6-9.9>
         2, 3, 4
@@ -872,7 +872,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select6-9.10",
     [[
-        SELECT x, y FROM (SELECT x, (SELECT 10+x) y FROM t1 LIMIT -1 OFFSET 1);
+        SELECT x, y FROM (SELECT x, (SELECT 10+x) y FROM t1 LIMIT 9-1 OFFSET 1);
     ]], {
         -- <select6-9.10>
         2, 12, 3, 13, 4, 14
@@ -882,7 +882,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select6-9.11",
     [[
-        SELECT x, y FROM (SELECT x, (SELECT 10)+x y FROM t1 LIMIT -1 OFFSET 1);
+        SELECT x, y FROM (SELECT x, (SELECT 10)+x y FROM t1 LIMIT 7-1 OFFSET 1);
     ]], {
         -- <select6-9.11>
         2, 12, 3, 13, 4, 14
diff --git a/test/sql-tap/select8.test.lua b/test/sql-tap/select8.test.lua
index b67d0a194..7b2bec51d 100755
--- a/test/sql-tap/select8.test.lua
+++ b/test/sql-tap/select8.test.lua
@@ -69,7 +69,7 @@ test:do_execsql_test(
         SELECT DISTINCT artist,sum(timesplayed) AS total      
         FROM songs      
         GROUP BY LOWER(artist)      
-        LIMIT -1 OFFSET 2
+        LIMIT 1000 OFFSET 2
     ]], subrange(result, 5, #result))
 
 test:finish_test()
diff --git a/test/sql-tap/subquery2.test.lua b/test/sql-tap/subquery2.test.lua
index 240911f4d..58c5de2f6 100755
--- a/test/sql-tap/subquery2.test.lua
+++ b/test/sql-tap/subquery2.test.lua
@@ -132,7 +132,7 @@ test:do_execsql_test(
     2.2,
     [[
         SELECT * 
-        FROM (SELECT * FROM t4 ORDER BY a LIMIT -1 OFFSET 1) 
+        FROM (SELECT * FROM t4 ORDER BY a LIMIT 1000000 OFFSET 1)
         LIMIT (SELECT a FROM t5)
     ]], {
         -- <2.2>
diff --git a/test/sql-tap/with1.test.lua b/test/sql-tap/with1.test.lua
index 97585c13b..4b56c5a15 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,6 @@ 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)
 -- #--------------------------------------------------------------------------
 -- # Test the ORDER BY clause on recursive tables.
 -- #
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 9ace282d5..9b0c08274 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -125,7 +125,7 @@ cn:execute('   ;')
 - error: 'Failed to execute SQL statement: syntax error: empty request'
 ...
 --
--- Parmaeters bindig.
+-- Parameters binding.
 --
 cn:execute('select * from test where id = ?', {1})
 ---
@@ -139,6 +139,61 @@ cn:execute('select * from test where id = ?', {1})
   rows:
   - [1, 2, '3']
 ...
+cn:execute('select * from test limit ?', {2})
+---
+- metadata:
+  - name: ID
+    type: INTEGER
+  - name: A
+    type: NUMERIC
+  - name: B
+    type: TEXT
+  rows:
+  - [1, 2, '3']
+  - [7, 8.5, '9']
+...
+cn:execute('select * from test limit ?', {-2})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    LIMIT clause'
+...
+cn:execute('select * from test limit ?', {2.7})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    LIMIT clause'
+...
+cn:execute('select * from test limit ?', {'Hello'})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    LIMIT clause'
+...
+cn:execute('select * from test limit 1 offset ?', {2})
+---
+- metadata:
+  - name: ID
+    type: INTEGER
+  - name: A
+    type: NUMERIC
+  - name: B
+    type: TEXT
+  rows:
+  - [10, 11, null]
+...
+cn:execute('select * from test limit 1 offset ?', {-2})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    OFFSET clause'
+...
+cn:execute('select * from test limit 1 offset ?', {2.7})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    OFFSET clause'
+...
+cn:execute('select * from test limit 1 offset ?', {'Hello'})
+---
+- error: 'Failed to execute SQL statement: Only positive numbers are allowed in the
+    OFFSET clause'
+...
 parameters = {}
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 664090368..919ddcb3c 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -45,10 +45,20 @@ cn:execute('')
 cn:execute('   ;')
 
 --
--- Parmaeters bindig.
+-- Parameters binding.
 --
 
 cn:execute('select * from test where id = ?', {1})
+cn:execute('select * from test limit ?', {2})
+cn:execute('select * from test limit ?', {-2})
+cn:execute('select * from test limit ?', {2.7})
+cn:execute('select * from test limit ?', {'Hello'})
+
+cn:execute('select * from test limit 1 offset ?', {2})
+cn:execute('select * from test limit 1 offset ?', {-2})
+cn:execute('select * from test limit 1 offset ?', {2.7})
+cn:execute('select * from test limit 1 offset ?', {'Hello'})
+
 parameters = {}
 parameters[1] = {}
 parameters[1][':value'] = 1
-- 
2.17.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause
  2019-02-05  8:56 [tarantool-patches] [PATCH v2] sql: prohibit negative values under LIMIT clause Stanislav Zudin
@ 2019-02-05 15:15 ` n.pettik
  2019-02-05 19:51   ` Stanislav Zudin
  2019-02-07  8:53 ` Kirill Yukhin
  1 sibling, 1 reply; 5+ messages in thread
From: n.pettik @ 2019-02-05 15:15 UTC (permalink / raw)
  To: tarantool-patches; +Cc: szudin


> If LIMIT or OFFSET expressions can be casted to the
> negative integer value VDBE returns an error.
> If expression in the LIMIT clause can't be converted
> into integer without data loss the VDBE instead of
> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
> message "Only positive numbers are allowed in the
> LIMIT clause". The same for OFFSET clause.
> 
> Closes #3467

Fix please commit message subject: now not only negative
values are rejected, but everything except for positive integers.

Below a few minor comments. Don’t send new version of patch,
just answer to this letter with provided fixes.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 02ee225f1..a8ed3c346 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 	Vdbe *v = 0;
> 	int iLimit = 0;
> 	int iOffset;
> -	int n;
> +	const char *negativeLimitError = "Only positive numbers are allowed "
> +					 "in the LIMIT clause”;

Nit: now this variable is used only once, so I suggest to inline it
or at least move closer to its usage. SQLite uses very ancient
way of declaring variables at the beginning of function
(I guess it is due to C 89 standard).

One more nit:

select * from t1 limit 0.5;
---
- error: Only positive numbers are allowed in the LIMIT clause
…

However, 0.5 is a positive number. Lets re-phrase error message a bit.
Like: Only positive integers are allowed in the LIMIT clause

> 	if (p->iLimit)
> 		return;
> 
> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 		p->iLimit = iLimit = ++pParse->nMem;
> 		v = sqlite3GetVdbe(pParse);
> 		assert(v != 0);
> -		if (sqlite3ExprIsInteger(p->pLimit, &n)) {
> -			sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit);
> -			VdbeComment((v, "LIMIT counter"));
> -			if (n == 0) {
> -				sqlite3VdbeGoto(v, iBreak);
> -			} else if (n >= 0
> -				   && p->nSelectRow > sqlite3LogEst((u64) n)) {
> -				p->nSelectRow = sqlite3LogEst((u64) n);
> -				p->selFlags |= SF_FixedLimit;
> -			}
> -		} else {
> -			sqlite3ExprCode(pParse, p->pLimit, iLimit);
> -			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
> -			VdbeCoverage(v);
> -			VdbeComment((v, "LIMIT counter"));
> -			sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
> -			VdbeCoverage(v);
> -		}
> +		int lPosLimitValue = sqlite3VdbeMakeLabel(v);
> +		int labelHalt = sqlite3VdbeMakeLabel(v);

Could you please rename these two variables according to our
code style (as I pointed in previous review)? For example:

int IPosLimitValue -> positive_limit_label; 
int labelHalt -> int halt_label;

The same for code concerning LIMIT clause.

(Yep, it is kind of messing code styles, but we decided to use
our codestyle in freshly added/refactored code).

> +		sqlite3ExprCode(pParse, p->pLimit, iLimit);
> +		sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt);
> +		/* 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 */
> +		sqlite3VdbeResolveLabel(v, labelHalt);
> +		sqlite3VdbeAddOp4(v, OP_Halt,
> +				  SQL_TARANTOOL_ERROR,
> +				  0, 0,
> +				  negativeLimitError,
> +				  P4_STATIC);
> +
> +		sqlite3VdbeResolveLabel(v, lPosLimitValue);
> +		sqlite3ReleaseTempReg(pParse, r1);
> +		VdbeCoverage(v);
> +		VdbeComment((v, "LIMIT counter"));
> +		sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
> +		VdbeCoverage(v);
> +
> 
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 664090368..919ddcb3c 100644
> cn:execute('select * from test where id = ?', {1})

Add a short comment like:
gh-3467: allow only positive integers under limit clause.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause
  2019-02-05 15:15 ` [tarantool-patches] " n.pettik
@ 2019-02-05 19:51   ` Stanislav Zudin
  2019-02-06 13:25     ` n.pettik
  0 siblings, 1 reply; 5+ messages in thread
From: Stanislav Zudin @ 2019-02-05 19:51 UTC (permalink / raw)
  To: tarantool-patches, n.pettik

The remarks are applied in the branch:
https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-limits

Issue: https://github.com/tarantool/tarantool/issues/3467


On 05.02.2019 18:15, n.pettik wrote:
>> If LIMIT or OFFSET expressions can be casted to the
>> negative integer value VDBE returns an error.
>> If expression in the LIMIT clause can't be converted
>> into integer without data loss the VDBE instead of
>> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
>> message "Only positive numbers are allowed in the
>> LIMIT clause". The same for OFFSET clause.
>>
>> Closes #3467
> Fix please commit message subject: now not only negative
> values are rejected, but everything except for positive integers.
>
> Below a few minor comments. Don’t send new version of patch,
> just answer to this letter with provided fixes.
>
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index 02ee225f1..a8ed3c346 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>> 	Vdbe *v = 0;
>> 	int iLimit = 0;
>> 	int iOffset;
>> -	int n;
>> +	const char *negativeLimitError = "Only positive numbers are allowed "
>> +					 "in the LIMIT clause”;
> Nit: now this variable is used only once, so I suggest to inline it
> or at least move closer to its usage. SQLite uses very ancient
> way of declaring variables at the beginning of function
> (I guess it is due to C 89 standard).
>
> One more nit:
>
> select * from t1 limit 0.5;
> ---
> - error: Only positive numbers are allowed in the LIMIT clause
> …
>
> However, 0.5 is a positive number. Lets re-phrase error message a bit.
> Like: Only positive integers are allowed in the LIMIT clause
>
>> 	if (p->iLimit)
>> 		return;
>>
>> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>> 		p->iLimit = iLimit = ++pParse->nMem;
>> 		v = sqlite3GetVdbe(pParse);
>> 		assert(v != 0);
>> -		if (sqlite3ExprIsInteger(p->pLimit, &n)) {
>> -			sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit);
>> -			VdbeComment((v, "LIMIT counter"));
>> -			if (n == 0) {
>> -				sqlite3VdbeGoto(v, iBreak);
>> -			} else if (n >= 0
>> -				   && p->nSelectRow > sqlite3LogEst((u64) n)) {
>> -				p->nSelectRow = sqlite3LogEst((u64) n);
>> -				p->selFlags |= SF_FixedLimit;
>> -			}
>> -		} else {
>> -			sqlite3ExprCode(pParse, p->pLimit, iLimit);
>> -			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
>> -			VdbeCoverage(v);
>> -			VdbeComment((v, "LIMIT counter"));
>> -			sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
>> -			VdbeCoverage(v);
>> -		}
>> +		int lPosLimitValue = sqlite3VdbeMakeLabel(v);
>> +		int labelHalt = sqlite3VdbeMakeLabel(v);
> Could you please rename these two variables according to our
> code style (as I pointed in previous review)? For example:
>
> int IPosLimitValue -> positive_limit_label;
> int labelHalt -> int halt_label;
>
> The same for code concerning LIMIT clause.
>
> (Yep, it is kind of messing code styles, but we decided to use
> our codestyle in freshly added/refactored code).
>
>> +		sqlite3ExprCode(pParse, p->pLimit, iLimit);
>> +		sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt);
>> +		/* 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 */
>> +		sqlite3VdbeResolveLabel(v, labelHalt);
>> +		sqlite3VdbeAddOp4(v, OP_Halt,
>> +				  SQL_TARANTOOL_ERROR,
>> +				  0, 0,
>> +				  negativeLimitError,
>> +				  P4_STATIC);
>> +
>> +		sqlite3VdbeResolveLabel(v, lPosLimitValue);
>> +		sqlite3ReleaseTempReg(pParse, r1);
>> +		VdbeCoverage(v);
>> +		VdbeComment((v, "LIMIT counter"));
>> +		sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
>> +		VdbeCoverage(v);
>> +
>>
>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
>> index 664090368..919ddcb3c 100644
>> cn:execute('select * from test where id = ?', {1})
> Add a short comment like:
> gh-3467: allow only positive integers under limit clause.
>
>> +cn:execute('select * from test limit ?', {2})
>> +cn:execute('select * from test limit ?', {-2})
>> +cn:execute('select * from test limit ?', {2.7})
>> +cn:execute('select * from test limit ?', {'Hello'})
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause
  2019-02-05 19:51   ` Stanislav Zudin
@ 2019-02-06 13:25     ` n.pettik
  0 siblings, 0 replies; 5+ messages in thread
From: n.pettik @ 2019-02-06 13:25 UTC (permalink / raw)
  To: tarantool-patches; +Cc: szudin



> On 5 Feb 2019, at 22:51, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> The remarks are applied in the branch:
> https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-limits

Ok, but next time please attach diff (between old and new version)
right to the letter (but not send it as version++).

LGTM.

> Issue: https://github.com/tarantool/tarantool/issues/3467
> 
> On 05.02.2019 18:15, n.pettik wrote:
>>> If LIMIT or OFFSET expressions can be casted to the
>>> negative integer value VDBE returns an error.
>>> If expression in the LIMIT clause can't be converted
>>> into integer without data loss the VDBE instead of
>>> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
>>> message "Only positive numbers are allowed in the
>>> LIMIT clause". The same for OFFSET clause.
>>> 
>>> Closes #3467
>> Fix please commit message subject: now not only negative
>> values are rejected, but everything except for positive integers.
>> 
>> Below a few minor comments. Don’t send new version of patch,
>> just answer to this letter with provided fixes.
>> 
>>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>>> index 02ee225f1..a8ed3c346 100644
>>> --- a/src/box/sql/select.c
>>> +++ b/src/box/sql/select.c
>>> @@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>>> 	Vdbe *v = 0;
>>> 	int iLimit = 0;
>>> 	int iOffset;
>>> -	int n;
>>> +	const char *negativeLimitError = "Only positive numbers are allowed "
>>> +					 "in the LIMIT clause”;
>> Nit: now this variable is used only once, so I suggest to inline it
>> or at least move closer to its usage. SQLite uses very ancient
>> way of declaring variables at the beginning of function
>> (I guess it is due to C 89 standard).
>> 
>> One more nit:
>> 
>> select * from t1 limit 0.5;
>> ---
>> - error: Only positive numbers are allowed in the LIMIT clause
>> …
>> 
>> However, 0.5 is a positive number. Lets re-phrase error message a bit.
>> Like: Only positive integers are allowed in the LIMIT clause
>> 
>>> 	if (p->iLimit)
>>> 		return;
>>> 
>>> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>>> 		p->iLimit = iLimit = ++pParse->nMem;
>>> 		v = sqlite3GetVdbe(pParse);
>>> 		assert(v != 0);
>>> -		if (sqlite3ExprIsInteger(p->pLimit, &n)) {
>>> -			sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit);
>>> -			VdbeComment((v, "LIMIT counter"));
>>> -			if (n == 0) {
>>> -				sqlite3VdbeGoto(v, iBreak);
>>> -			} else if (n >= 0
>>> -				   && p->nSelectRow > sqlite3LogEst((u64) n)) {
>>> -				p->nSelectRow = sqlite3LogEst((u64) n);
>>> -				p->selFlags |= SF_FixedLimit;
>>> -			}
>>> -		} else {
>>> -			sqlite3ExprCode(pParse, p->pLimit, iLimit);
>>> -			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
>>> -			VdbeCoverage(v);
>>> -			VdbeComment((v, "LIMIT counter"));
>>> -			sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
>>> -			VdbeCoverage(v);
>>> -		}
>>> +		int lPosLimitValue = sqlite3VdbeMakeLabel(v);
>>> +		int labelHalt = sqlite3VdbeMakeLabel(v);
>> Could you please rename these two variables according to our
>> code style (as I pointed in previous review)? For example:
>> 
>> int IPosLimitValue -> positive_limit_label;
>> int labelHalt -> int halt_label;
>> 
>> The same for code concerning LIMIT clause.
>> 
>> (Yep, it is kind of messing code styles, but we decided to use
>> our codestyle in freshly added/refactored code).
>> 
>>> +		sqlite3ExprCode(pParse, p->pLimit, iLimit);
>>> +		sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt);
>>> +		/* 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 */
>>> +		sqlite3VdbeResolveLabel(v, labelHalt);
>>> +		sqlite3VdbeAddOp4(v, OP_Halt,
>>> +				  SQL_TARANTOOL_ERROR,
>>> +				  0, 0,
>>> +				  negativeLimitError,
>>> +				  P4_STATIC);
>>> +
>>> +		sqlite3VdbeResolveLabel(v, lPosLimitValue);
>>> +		sqlite3ReleaseTempReg(pParse, r1);
>>> +		VdbeCoverage(v);
>>> +		VdbeComment((v, "LIMIT counter"));
>>> +		sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
>>> +		VdbeCoverage(v);
>>> +
>>> 
>>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
>>> index 664090368..919ddcb3c 100644
>>> cn:execute('select * from test where id = ?', {1})
>> Add a short comment like:
>> gh-3467: allow only positive integers under limit clause.
>> 
>>> +cn:execute('select * from test limit ?', {2})
>>> +cn:execute('select * from test limit ?', {-2})
>>> +cn:execute('select * from test limit ?', {2.7})
>>> +cn:execute('select * from test limit ?', {'Hello'})
>> 
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause
  2019-02-05  8:56 [tarantool-patches] [PATCH v2] sql: prohibit negative values under LIMIT clause Stanislav Zudin
  2019-02-05 15:15 ` [tarantool-patches] " n.pettik
@ 2019-02-07  8:53 ` Kirill Yukhin
  1 sibling, 0 replies; 5+ messages in thread
From: Kirill Yukhin @ 2019-02-07  8:53 UTC (permalink / raw)
  To: tarantool-patches; +Cc: korablev, Stanislav Zudin

Hello,

On 05 Feb 11:56, Stanislav Zudin wrote:
> If LIMIT or OFFSET expressions can be casted to the
> negative integer value VDBE returns an error.
> If expression in the LIMIT clause can't be converted
> into integer without data loss the VDBE instead of
> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
> message "Only positive numbers are allowed in the
> LIMIT clause". The same for OFFSET clause.
> 
> Closes #3467
> ---
> Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3467-prohibit-negative-limits
> Issue: https://github.com/tarantool/tarantool/issues/3467

I've checked your patch into 2.1 branch.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-02-07  8:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-05  8:56 [tarantool-patches] [PATCH v2] sql: prohibit negative values under LIMIT clause Stanislav Zudin
2019-02-05 15:15 ` [tarantool-patches] " n.pettik
2019-02-05 19:51   ` Stanislav Zudin
2019-02-06 13:25     ` n.pettik
2019-02-07  8:53 ` Kirill Yukhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox