[tarantool-patches] [PATCH v1 1/1] sql: disallow division by zero

[tarantool-patches] [PATCH v1 1/1] sql: disallow division by zero

[Kirill Shcherbatov]

Dissallowed division by zero in SQL to follow ANSI standard.
This also affects some unobvious scenarios when divider indirectly
casts to zero like
SELECT 'word1' / 'word2';

Closes #2135.
---
https://github.com/tarantool/tarantool/compare/kshch/gh-2135-dissallow-div-by-zero
https://github.com/tarantool/tarantool/issues/2135

 src/box/sql/vdbe.c              | 19 ++++++++++----
 test/sql-tap/e_expr.test.lua    | 55 ++++++++++++++++++++++++++++-------------
 test/sql-tap/randexpr1.test.lua | 11 +++------
 3 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a4d376..8a200b0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1590,13 +1590,15 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		case OP_Subtract:  if (sqlite3SubInt64(&iB,iA)) goto fp_math;  break;
 		case OP_Multiply:  if (sqlite3MulInt64(&iB,iA)) goto fp_math;  break;
 		case OP_Divide: {
-			if (iA==0) goto arithmetic_result_is_null;
+			if (iA == 0)
+				goto division_by_zero;
 			if (iA==-1 && iB==SMALLEST_INT64) goto fp_math;
 			iB /= iA;
 			break;
 		}
 		default: {
-			if (iA==0) goto arithmetic_result_is_null;
+			if (iA == 0)
+				goto division_by_zero;
 			if (iA==-1) iA = 1;
 			iB %= iA;
 			break;
@@ -1615,14 +1617,16 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		case OP_Multiply:    rB *= rA;       break;
 		case OP_Divide: {
 			/* (double)0 In case of SQLITE_OMIT_FLOATING_POINT... */
-			if (rA==(double)0) goto arithmetic_result_is_null;
+			if (rA == (double)0)
+				goto division_by_zero;
 			rB /= rA;
 			break;
 		}
 		default: {
 			iA = (i64)rA;
 			iB = (i64)rB;
-			if (iA==0) goto arithmetic_result_is_null;
+			if (iA == 0)
+				goto division_by_zero;
 			if (iA==-1) iA = 1;
 			rB = (double)(iB % iA);
 			break;
@@ -1644,9 +1648,14 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 	}
 	break;
 
-			arithmetic_result_is_null:
+arithmetic_result_is_null:
 	sqlite3VdbeMemSetNull(pOut);
 	break;
+
+division_by_zero:
+	diag_set(ClientError, ER_SQL_EXECUTE, "division by zero");
+	rc = SQL_TARANTOOL_ERROR;
+	goto abort_due_to_error;
 }
 
 /* Opcode: CollSeq P1 * * P4
diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
index d617f09..d7b8820 100755
--- a/test/sql-tap/e_expr.test.lua
+++ b/test/sql-tap/e_expr.test.lua
@@ -165,11 +165,20 @@ for _, op1 in ipairs(oplist) do
             local sql1 = string.format("SELECT %s %s %s %s %s", A, op1, B, op2, C)
             local sql2 = string.format("SELECT (%s %s %s) %s %s", A, op1, B, op2, C)
             local sql3 = string.format("SELECT %s %s (%s %s %s)", A, op1, B, op2, C)
-            local a2 = test:execsql(sql2)
-            local a3 = test:execsql(sql3)
-            test:do_execsql_test(
-                testname,
-                sql1, (opprec[op2] < opprec[op1]) and a3 or a2)
+            local a2 = test:catchsql(sql2)
+            local a3 = test:catchsql(sql3)
+            local res = opprec[op2] < opprec[op1] and a3 or a2
+
+            if res[1] ~= 0 then
+                --
+                -- gh-2135: Division by zero is forbiden.
+                --
+                test:do_catchsql_test(
+                    testname, sql1,
+                    {1, "Failed to execute SQL statement: division by zero"})
+            else
+                test:do_execsql_test(testname, sql1, res[2])
+            end
 
             if (a2 ~= a3) then
                 untested[op1..","..op2] = nil
@@ -460,18 +469,30 @@ literals = {
 for _, op in ipairs(oplist) do
     for n1, rhs in ipairs(literals) do
         for n2, lhs in ipairs(literals) do
-            local t = test:execsql(string.format(" SELECT typeof(%s %s %s) ", lhs, op, rhs))[1]
-            test:do_test(
-                string.format("e_expr-7.%s.%s.%s", opname[op], n1, n2),
-                function()
-                    --print("\n op "..op.." t "..t)
-                    return (((op == "||") and ((t == "text") or
-                            (t == "null"))) or
-                            ((op ~= "||") and (((t == "integer") or
-                                    (t == "real")) or
-                                    (t == "null")))) and 1 or 0
-                end, 1)
-
+            local res = test:catchsql(string.format(" SELECT typeof(%s %s %s) ", lhs, op, rhs))
+            local testname = string.format("e_expr-7.%s.%s.%s", opname[op], n1, n2)
+            if res[1] ~= 0 then
+                --
+                -- gh-2135: Division by zero is forbiden.
+                --
+                test:do_test(
+                    testname,
+                    function()
+                        return res[2] == "Failed to execute SQL statement: division by zero"
+                    end, true)
+            else
+                local t = res[2][1]
+                test:do_test(
+                    testname,
+                    function()
+                        --print("\n op "..op.." t "..t)
+                        return (((op == "||") and ((t == "text") or
+                                (t == "null"))) or
+                                ((op ~= "||") and (((t == "integer") or
+                                        (t == "real")) or
+                                        (t == "null")))) and 1 or 0
+                    end, 1)
+            end
         end
     end
 end
diff --git a/test/sql-tap/randexpr1.test.lua b/test/sql-tap/randexpr1.test.lua
index 5665fff..7074b18 100755
--- a/test/sql-tap/randexpr1.test.lua
+++ b/test/sql-tap/randexpr1.test.lua
@@ -16534,15 +16534,10 @@ test:do_test(
         -- </randexpr-2.1649>
     })
 
-test:do_test(
+test:do_catchsql_test(
     "randexpr-2.1650",
-    function()
-        return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  -t1.a when t1.d then  -13 else 17 end))))-t1.c) from t1 union select ~case cast(avg(t1.c) AS integer) when  -~ -count(*)*max((t1.f))+count(*) then (min(d)) else max(a) end-count(distinct t1.a)+ -count(*)-count(*) from t1) then d when b not between t1.f and t1.e then a else 11 end+t1.a FROM t1 WHERE NOT ((exists(select 1 from t1 where case when exists(select 1 from t1 where case when (t1.e+case (t1.b) when f then case t1.c when 11 |  -13-coalesce((select max(d) from t1 where c not in ( -t1.e,t1.c,17) and t1.c<17),e)*17+t1.a then t1.b else 11 end else d end)+t1.f not in (d,13,d) then f else t1.d end not between (c) and f) then t1.e else f end-t1.c in (t1.b,t1.a,f))))"
-    end, {
-        -- <randexpr-2.1650>
-        800
-        -- </randexpr-2.1650>
-    })
+    "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  -t1.a when t1.d then  -13 else 17 end))))-t1.c) from t1 union select ~case cast(avg(t1.c) AS integer) when  -~ -count(*)*max((t1.f))+count(*) then (min(d)) else max(a) end-count(distinct t1.a)+ -count(*)-count(*) from t1) then d when b not between t1.f and t1.e then a else 11 end+t1.a FROM t1 WHERE NOT ((exists(select 1 from t1 where case when exists(select 1 from t1 where case when (t1.e+case (t1.b) when f then case t1.c when 11 |  -13-coalesce((select max(d) from t1 where c not in ( -t1.e,t1.c,17) and t1.c<17),e)*17+t1.a then t1.b else 11 end else d end)+t1.f not in (d,13,d) then f else t1.d end not between (c) and f) then t1.e else f end-t1.c in (t1.b,t1.a,f))))",
+    {1, "Failed to execute SQL statement: division by zero"})
 
 test:do_test(
     "randexpr-2.1651",
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow division by zero

[Vladislav Shpilevoy]

Hello. Thanks for the patch!

See 3 comments below.

On 12/07/2018 14:16, Kirill Shcherbatov wrote:
> Dissallowed division by zero in SQL to follow ANSI standard.

1. Typo.

> This also affects some unobvious scenarios when divider indirectly
> casts to zero like
> SELECT 'word1' / 'word2';
> 
> Closes #2135.
> ---
> https://github.com/tarantool/tarantool/compare/kshch/gh-2135-dissallow-div-by-zero
> https://github.com/tarantool/tarantool/issues/2135
> 
> diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
> index d617f09..d7b8820 100755
> --- a/test/sql-tap/e_expr.test.lua
> +++ b/test/sql-tap/e_expr.test.lua
> @@ -460,18 +469,30 @@ literals = {
>   for _, op in ipairs(oplist) do
>       for n1, rhs in ipairs(literals) do
>           for n2, lhs in ipairs(literals) do
> -            local t = test:execsql(string.format(" SELECT typeof(%s %s %s) ", lhs, op, rhs))[1]
> -            test:do_test(
> -                string.format("e_expr-7.%s.%s.%s", opname[op], n1, n2),
> -                function()
> -                    --print("\n op "..op.." t "..t)
> -                    return (((op == "||") and ((t == "text") or
> -                            (t == "null"))) or
> -                            ((op ~= "||") and (((t == "integer") or
> -                                    (t == "real")) or
> -                                    (t == "null")))) and 1 or 0
> -                end, 1)
> -
> +            local res = test:catchsql(string.format(" SELECT typeof(%s %s %s) ", lhs, op, rhs))
> +            local testname = string.format("e_expr-7.%s.%s.%s", opname[op], n1, n2)
> +            if res[1] ~= 0 then
> +                --
> +                -- gh-2135: Division by zero is forbiden.
> +                --
> +                test:do_test(
> +                    testname,
> +                    function()
> +                        return res[2] == "Failed to execute SQL statement: division by zero"
> +                    end, true)
> +            else
> +                local t = res[2][1]
> +                test:do_test(
> +                    testname,
> +                    function()
> +                        --print("\n op "..op.." t "..t)

2. Garbage comment.

> +                        return (((op == "||") and ((t == "text") or
> +                                (t == "null"))) or
> +                                ((op ~= "||") and (((t == "integer") or
> +                                        (t == "real")) or
> +                                        (t == "null")))) and 1 or 0
> +                    end, 1)
> +            end
>           end
>       end
>   end
> diff --git a/test/sql-tap/randexpr1.test.lua b/test/sql-tap/randexpr1.test.lua
> index 5665fff..7074b18 100755
> --- a/test/sql-tap/randexpr1.test.lua
> +++ b/test/sql-tap/randexpr1.test.lua
> @@ -16534,15 +16534,10 @@ test:do_test(
>           -- </randexpr-2.1649>
>       })
>   
> -test:do_test(
> +test:do_catchsql_test(
>       "randexpr-2.1650",
> -    function()
> -        return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  -t1.a when t1.d then  -13 else 17 end))))-t1.c) from t1 union select ~case cast(avg(t1.c) AS integer) when  -~ -count(*)*max((t1.f))+count(*) then (min(d)) else max(a) end-count(distinct t1.a)+ -count(*)-count(*) from t1) then d when b not between t1.f and t1.e then a else 11 end+t1.a FROM t1 WHERE NOT ((exists(select 1 from t1 where case when exists(select 1 from t1 where case when (t1.e+case (t1.b) when f then case t1.c when 11 |  -13-coalesce((select max(d) from t1 where c not in ( -t1.e,t1.c,17) and t1.c<17),e)*17+t1.a then t1.b else 11 end else d end)+t1.f not in (d,13,d) then f else t1.d end not between (c) and f) then t1.e else f end-t1.c in (t1.b,t1.a,f))))"
> -    end, {
> -        -- <randexpr-2.1650>
> -        800
> -        -- </randexpr-2.1650>
> -    })
> +    "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  -t1.a when t1.d then  -13 else 17 end))))-t1.c) from t1 union select ~case cast(avg(t1.c) AS integer) when  -~ -count(*)*max((t1.f))+count(*) then (min(d)) else max(a) end-count(distinct t1.a)+ -count(*)-count(*) from t1) then d when b not between t1.f and t1.e then a else 11 end+t1.a FROM t1 WHERE NOT ((exists(select 1 from t1 where case when exists(select 1 from t1 where case when (t1.e+case (t1.b) when f then case t1.c when 11 |  -13-coalesce((select max(d) from t1 where c not in ( -t1.e,t1.c,17) and t1.c<17),e)*17+t1.a then t1.b else 11 end else d end)+t1.f not in (d,13,d) then f else t1.d end not between (c) and f) then t1.e else f end-t1.c in (t1.b,t1.a,f))))",
> +    {1, "Failed to execute SQL statement: division by zero"})

3. Such huge test for division by zero is overkill. Please, fix the test to
avoid division by zero. Obviously it was not about divisions.

>   
>   test:do_test(
>       "randexpr-2.1651",
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow division by zero

[Kirill Shcherbatov]

Hi! Thank you for review.

> 1. Typo.
Fixed.

> 2. Garbage comment.Dropped. It wasn't mine.

> 3. Such huge test for division by zero is overkill. Please, fix the test to
> avoid division by zero. Obviously it was not about divisions.
Ok, I've changed one sign to fix this test case:
return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  .....
return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)*abs(+case  .....
Test result didn't change.

[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow division by zero

[Vladislav Shpilevoy]

Thanks for the fixes!

LGTM.

On 13/07/2018 14:49, Kirill Shcherbatov wrote:
> Hi! Thank you for review.
> 
>> 1. Typo.
> Fixed.
> 
>> 2. Garbage comment.Dropped. It wasn't mine.
> 
>> 3. Such huge test for division by zero is overkill. Please, fix the test to
>> avoid division by zero. Obviously it was not about divisions.
> Ok, I've changed one sign to fix this test case:
> return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)/abs(+case  .....
> return test:execsql "SELECT f+case when 17-t1.f in (select ~count(distinct (abs((abs(c)/abs(c)))/abs((abs(11)*abs(+case  .....
> Test result didn't change.
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow division by zero

[n.pettik]

LGTM as well.

[tarantool-patches] Re: [PATCH v1 1/1] sql: disallow division by zero

[Kirill Yukhin]

Hello,
On 12 июл 14:16, Kirill Shcherbatov wrote:
> Dissallowed division by zero in SQL to follow ANSI standard.
> This also affects some unobvious scenarios when divider indirectly
> casts to zero like
> SELECT 'word1' / 'word2';
> 
> Closes #2135.
> ---
> https://github.com/tarantool/tarantool/compare/kshch/gh-2135-dissallow-div-by-zero
> https://github.com/tarantool/tarantool/issues/2135
I've checked the patch into 2.0 branch.

--
Thanks, Kirill Yukhin