[Tarantool-patches] [PATCH v2 2/3] sql: disallow explicit cast of VARBINARY to number

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answers, diff and new patch below.

On 26.07.2021 23:12, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 3 comments below.
>
> On 21.07.2021 17:10, Mergen Imeev via Tarantool-patches wrote:
>> This patch removes explicit cast of VARBINARY values to numeric types.
>>
>> Part of #4470
>> Closes #4772
>> Closes #5852
>
> 1. You might need to add a changelog file.
>
I added in next commit, however, if you find it too general please let me know.
I will make it more specific.

>> diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
>> index 3dc49c38e..379fbf09e 100755
>> --- a/test/sql-tap/cast.test.lua
>> +++ b/test/sql-tap/cast.test.lua
>> @@ -867,13 +879,14 @@ test:do_test(
>>  -- gh-4356: Check that result of blob to number cast if of type
>>  -- number.
>
> 2. This comment could mean the test becomes just wrong and can
> be deleted.
>
Removed.

>> diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
>> index 56b11da25..a2877167f 100755
>> --- a/test/sql-tap/numcast.test.lua
>> +++ b/test/sql-tap/numcast.test.lua
>> @@ -149,20 +149,22 @@ test:do_execsql_test(
>>  -- gh-4233: Make sure that NUMBER can contain UNSIGNED, INTEGER
>>  -- and DOUBLE and is not automatically converted to DOUBLE.
>>  --
>
> 3. These tests seem to be quite far from this comment. Perhaps they
> are broken for some time already. Might worth deleting them.
>
I looked at the commit that added these tests and the tests are the same. Still,
I removed ones with VARBINARY since they are useless now.

>> -test:do_execsql_test(
>> +test:do_catchsql_test(
>>      "numcast-3.1",
>>      [[
>>          SELECT CAST(x'3131313131313131313131313131313131313131' AS NUMBER);
>>      ]], {
>> -        11111111111111111111ULL
>> +        1, "Type mismatch: can not convert "..
>> +           "varbinary(x'3131313131313131313131313131313131313131') to number"
>>      })
>>  
>> -test:do_execsql_test(
>> +test:do_catchsql_test(
>>      "numcast-3.2",
>>      [[
>>          SELECT CAST(x'31313131313131313131313131313131313131312E' AS NUMBER);
>>      ]], {
>> -        11111111111111110656
>> +        1, "Type mismatch: can not convert "..
>> +           "varbinary(x'31313131313131313131313131313131313131312E') to number"
>>      })
>>  
>>  test:do_execsql_test(
>>

Diff:

diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 379fbf09e..997298693 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(95)
+test:plan(94)
 
 --!./tcltestrunner.lua
 -- 2005 June 25
@@ -875,21 +875,6 @@ test:do_test(
         -- </cast-4.4>
     })
 
-
--- gh-4356: Check that result of blob to number cast if of type
--- number.
---
-test:do_catchsql_test(
-    "cast-5.1",
-    [[
-        SELECT CAST(x'3138343436373434303733372e33' AS NUMBER)
-    ]], {
-        -- <cast-5.1>
-        1, "Type mismatch: can not convert "..
-           "varbinary(x'3138343436373434303733372E33') to number"
-        -- </cast-5.1>
-    })
-
 -- gh-4470: Make explicit casts work according to our rules.
 
 -- Make sure that explicit cast from BOOLEAN to numeric types throws an error.
diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
index 8f738c760..73d0de642 100755
--- a/test/sql-tap/numcast.test.lua
+++ b/test/sql-tap/numcast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(31)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2013 March 20
@@ -149,24 +149,6 @@ test:do_execsql_test(
 -- gh-4233: Make sure that NUMBER can contain UNSIGNED, INTEGER
 -- and DOUBLE and is not automatically converted to DOUBLE.
 --
-test:do_catchsql_test(
-    "numcast-3.1",
-    [[
-        SELECT CAST(x'3131313131313131313131313131313131313131' AS NUMBER);
-    ]], {
-        1, "Type mismatch: can not convert "..
-           "varbinary(x'3131313131313131313131313131313131313131') to number"
-    })
-
-test:do_catchsql_test(
-    "numcast-3.2",
-    [[
-        SELECT CAST(x'31313131313131313131313131313131313131312E' AS NUMBER);
-    ]], {
-        1, "Type mismatch: can not convert "..
-           "varbinary(x'31313131313131313131313131313131313131312E') to number"
-    })
-
 test:do_execsql_test(
     "numcast-3.3",
     [[


New patch:

commit 8d3502b3cf8b1acbda0ecb76c03284c9edf2f76a
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Mon Jul 19 15:31:21 2021 +0300

    sql: disallow explicit cast of VARBINARY to number
    
    This patch removes explicit cast of VARBINARY values to numeric types.
    
    Part of #4470
    Closes #4772
    Closes #5852

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index aeaa01d94..d8296faa5 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -735,9 +735,9 @@ bin_to_uuid(struct Mem *mem)
 }
 
 static inline int
-bytes_to_int(struct Mem *mem)
+str_to_int(struct Mem *mem)
 {
-   assert((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0);
+   assert(mem->type == MEM_TYPE_STR);
    bool is_neg;
    int64_t i;
    if (sql_atoi64(mem->z, &i, &is_neg, mem->n) != 0)
@@ -747,9 +747,9 @@ bytes_to_int(struct Mem *mem)
 }
 
 static inline int
-bytes_to_uint(struct Mem *mem)
+str_to_uint(struct Mem *mem)
 {
-   assert((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0);
+   assert(mem->type == MEM_TYPE_STR);
    bool is_neg;
    int64_t i;
    if (sql_atoi64(mem->z, &i, &is_neg, mem->n) != 0)
@@ -761,9 +761,9 @@ bytes_to_uint(struct Mem *mem)
 }
 
 static inline int
-bytes_to_double(struct Mem *mem)
+str_to_double(struct Mem *mem)
 {
-   assert((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0);
+   assert(mem->type == MEM_TYPE_STR);
    double d;
    if (sqlAtoF(mem->z, &d, mem->n) == 0)
        return -1;
@@ -905,8 +905,8 @@ mem_to_int(struct Mem *mem)
    assert(mem->type < MEM_TYPE_INVALID);
    if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
        return 0;
-   if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
-       return bytes_to_int(mem);
+   if (mem->type == MEM_TYPE_STR)
+       return str_to_int(mem);
    if (mem->type == MEM_TYPE_DOUBLE)
        return double_to_int(mem);
    return -1;
@@ -919,7 +919,7 @@ mem_to_int_precise(struct Mem *mem)
    if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
        return 0;
    if (mem->type == MEM_TYPE_STR)
-       return bytes_to_int(mem);
+       return str_to_int(mem);
    if (mem->type == MEM_TYPE_DOUBLE)
        return double_to_int_precise(mem);
    return -1;
@@ -934,7 +934,7 @@ mem_to_double(struct Mem *mem)
    if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
        return int_to_double(mem);
    if (mem->type == MEM_TYPE_STR)
-       return bytes_to_double(mem);
+       return str_to_double(mem);
    return -1;
 }
 
@@ -944,10 +944,10 @@ mem_to_number(struct Mem *mem)
    assert(mem->type < MEM_TYPE_INVALID);
    if (mem_is_num(mem))
        return 0;
-   if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0) {
-       if (bytes_to_int(mem) == 0)
+   if (mem->type == MEM_TYPE_STR) {
+       if (str_to_int(mem) == 0)
            return 0;
-       return bytes_to_double(mem);
+       return str_to_double(mem);
    }
    return -1;
 }
@@ -1021,8 +1021,7 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
        case MEM_TYPE_UINT:
            return 0;
        case MEM_TYPE_STR:
-       case MEM_TYPE_BIN:
-           return bytes_to_uint(mem);
+           return str_to_uint(mem);
        case MEM_TYPE_DOUBLE:
            return double_to_int(mem);
        default:
@@ -1158,7 +1157,7 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
        if (mem->type == MEM_TYPE_DOUBLE)
            return double_to_uint_precise(mem);
        if (mem->type == MEM_TYPE_STR)
-           return bytes_to_uint(mem);
+           return str_to_uint(mem);
        return -1;
    case FIELD_TYPE_STRING:
        if ((mem->type & (MEM_TYPE_STR | MEM_TYPE_BIN)) != 0)
@@ -1182,7 +1181,7 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type type)
        if ((mem->type & (MEM_TYPE_INT | MEM_TYPE_UINT)) != 0)
            return 0;
        if (mem->type == MEM_TYPE_STR)
-           return bytes_to_int(mem);
+           return str_to_int(mem);
        if (mem->type == MEM_TYPE_DOUBLE)
            return double_to_int_precise(mem);
        return -1;
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 3dc49c38e..997298693 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(91)
+test:plan(94)
 
 --!./tcltestrunner.lua
 -- 2005 June 25
@@ -565,23 +565,23 @@ test:do_catchsql_test(
         -- </case-1.66>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "case-1.68",
     [[
         SELECT CAST(x'31' AS NUMBER)
     ]], {
         -- <case-1.68>
-        1.0
+        1, "Type mismatch: can not convert varbinary(x'31') to number"
         -- </case-1.68>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "case-1.69",
     [[
         SELECT typeof(CAST(x'31' AS NUMBER))
     ]], {
         -- <case-1.69>
-        "number"
+        1, "Type mismatch: can not convert varbinary(x'31') to number"
         -- </case-1.69>
     })
 
@@ -727,49 +727,61 @@ test:do_execsql_test(
 
 
 if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then
-    test:do_execsql_test(
+    test:do_catchsql_test(
         "cast-3.21",
         [[
             SELECT CAST(x'39323233333732303336383534373734383030' AS integer)
         ]], {
             -- <cast-3.21>
-            9223372036854774800LL
+            1, "Type mismatch: can not convert "..
+               "varbinary(x'39323233333732303336383534373734383030') to integer"
             -- </cast-3.21>
         })
 
-    test:do_execsql_test(
+    test:do_catchsql_test(
         "cast-3.22",
         [[
             SELECT CAST(x'393232333337323033363835343737343830302E' AS NUMBER)
         ]], {
             -- <cast-3.22>
-            9223372036854774784
+            1, "Type mismatch: can not convert "..
+               "varbinary(x'393232333337323033363835343737343830302E') "..
+               "to number"
             -- </cast-3.22>
         })
 
-    test:do_execsql_test(
+    test:do_catchsql_test(
         "cast-3.24",
         [[
             SELECT CAST(CAST(x'39323233333732303336383534373734383030' AS NUMBER)
                         AS integer)
         ]], {
             -- <cast-3.24>
-            9223372036854774800LL
+            1, "Type mismatch: can not convert "..
+               "varbinary(x'39323233333732303336383534373734383030') to number"
             -- </cast-3.24>
         })
 end
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "case-3.25",
     [[
         SELECT CAST(x'31383434363734343037333730393535313631352E' AS NUMBER);
-    ]], { 1.844674407371e+19 } )
+    ]], {
+        1, "Type mismatch: can not convert "..
+           "varbinary(x'31383434363734343037333730393535313631352E') to number"
+    })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "case-3.26",
     [[
         SELECT CAST(x'3138343436373434303733373039353531363135' AS INT);
-    ]], { 18446744073709551615LL } )
+    ]], {
+        -- <cast-3.21>
+        1, "Type mismatch: can not convert "..
+           "varbinary(x'3138343436373434303733373039353531363135') to integer"
+        -- </cast-3.21>
+    })
 
 test:do_execsql_test(
     "case-3.31",
@@ -863,20 +875,6 @@ test:do_test(
         -- </cast-4.4>
     })
 
-
--- gh-4356: Check that result of blob to number cast if of type
--- number.
---
-test:do_execsql_test(
-    "cast-5.1",
-    [[
-        SELECT CAST(x'3138343436373434303733372e33' AS NUMBER)
-    ]], {
-        -- <cast-5.1>
-        184467440737.3
-        -- </cast-5.1>
-    })
-
 -- gh-4470: Make explicit casts work according to our rules.
 
 -- Make sure that explicit cast from BOOLEAN to numeric types throws an error.
@@ -977,4 +975,37 @@ test:do_catchsql_test(
         1, "Type mismatch: can not convert integer(1) to boolean"
     })
 
+-- Make sure that explicit cast from VARBINARY to numeric types throws an error.
+test:do_catchsql_test(
+    "cast-7.1.1",
+    [[
+        SELECT CAST(x'31' AS UNSIGNED);
+    ]], {
+        1, "Type mismatch: can not convert varbinary(x'31') to unsigned"
+    })
+
+test:do_catchsql_test(
+    "cast-7.1.2",
+    [[
+        SELECT CAST(x'31' AS INTEGER);
+    ]], {
+        1, "Type mismatch: can not convert varbinary(x'31') to integer"
+    })
+
+test:do_catchsql_test(
+    "cast-7.1.3",
+    [[
+        SELECT CAST(x'31' AS DOUBLE);
+    ]], {
+        1, "Type mismatch: can not convert varbinary(x'31') to double"
+    })
+
+test:do_catchsql_test(
+    "cast-7.1.4",
+    [[
+        SELECT CAST(x'31' AS NUMBER);
+    ]], {
+        1, "Type mismatch: can not convert varbinary(x'31') to number"
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
deleted file mode 100755
index a8cc0e770..000000000
--- a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
+++ /dev/null
@@ -1,40 +0,0 @@
-#!/usr/bin/env tarantool
-local test = require("sqltester")
-test:plan(3)
-
---
--- Make sure that a blob as part of a tuple can be cast to NUMBER,
--- INTEGER and UNSIGNED. Prior to this patch, an error could
--- appear due to the absence of '\0' at the end of the BLOB.
---
-test:do_execsql_test(
-    "gh-4766-1",
-    [[
-        CREATE TABLE t1 (a VARBINARY PRIMARY KEY);
-        INSERT INTO t1 VALUES (X'33'), (X'372020202020');
-        SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t1;
-    ]], {
-        '3', 3, 3, 3, '7     ', 7, 7, 7
-    })
-
---
--- Make sure that BLOB longer than 12287 bytes cannot be cast to
--- INTEGER.
---
-local long_str = string.rep('0', 12284)
-test:do_execsql_test(
-    "gh-4766-2",
-    "SELECT CAST('" .. long_str .. "123'" .. " AS INTEGER);", {
-        123
-    })
-
-
-test:do_catchsql_test(
-    "gh-4766-3",
-    "SELECT CAST('" .. long_str .. "1234'" .. " AS INTEGER);", {
-        1, "Type mismatch: can not convert string('0000000000000000000000000" ..
-        "0000000000000000000000000000000000000000000000000000000000000000000" ..
-        "000000000000000000000000000000000000...) to integer"
-    })
-
-test:finish_test()
diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
index b3aa64e76..73d0de642 100755
--- a/test/sql-tap/numcast.test.lua
+++ b/test/sql-tap/numcast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(31)
+test:plan(29)
 
 --!./tcltestrunner.lua
 -- 2013 March 20
@@ -149,22 +149,6 @@ test:do_execsql_test(
 -- gh-4233: Make sure that NUMBER can contain UNSIGNED, INTEGER
 -- and DOUBLE and is not automatically converted to DOUBLE.
 --
-test:do_execsql_test(
-    "numcast-3.1",
-    [[
-        SELECT CAST(x'3131313131313131313131313131313131313131' AS NUMBER);
-    ]], {
-        11111111111111111111ULL
-    })
-
-test:do_execsql_test(
-    "numcast-3.2",
-    [[
-        SELECT CAST(x'31313131313131313131313131313131313131312E' AS NUMBER);
-    ]], {
-        11111111111111110656
-    })
-
 test:do_execsql_test(
     "numcast-3.3",
     [[