[Tarantool-patches] [PATCH v3 2/2] sql: add '\0' to the BLOB when it is cast to INTEGER

[imeevma@tarantool.org]

Hi! Thank you for review. My answers and new patch below.

On 3/11/20 7:15 PM, Nikita Pettik wrote:
> On 22 Feb 11:27, Mergen Imeev wrote:
>> Hi! Thank you for review. I changed a test once again.
>> Diff below.
>>
>> On Thu, Feb 20, 2020 at 10:58:21PM +0300, Nikita Pettik wrote:
>>> On 13 Feb 11:16, imeevma@tarantool.org wrote:
>>> So now you insert 0x33 instead of 1 to integer field. But how does it
>>> affect test? I failed to understand. In both cases you fetch and operate
>>> on blob, meanwhile integer field doesn't seem to be involved.
>>>
>> As I wrote in the last letter, we have a way to make sure
>> that with the first case everything will be in order,
>> without creating a duplicate of this binary value.
>> Obviously, that method will definitely not affect
>> performance. But it can lead to the part of the value that
>> looks like X'333300' being decoded as 33. See the example
>> from the last letter.
>>
>>
>> Diff:
>>
>> diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
>> index 86c0fee..74844e0 100755
>> --- a/test/sql-tap/cast.test.lua
>> +++ b/test/sql-tap/cast.test.lua
>> @@ -891,13 +891,15 @@ test:do_execsql_test(
>>  
>>  --
>>  -- In some cases, the absence of '\0' could lead to an incorrect
>> --- result. Make sure this does not happen now.
>> +-- result. For example, in this case, part of the value is as
>> +-- follows: X'333300', which can be decoded as the number 33. Make
>> +-- sure this does not happen now.
>>  --
>>  test:do_execsql_test(
>>      "cast-6.2",
>>      [[
>> -        CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT);
>> -        INSERT INTO t VALUES (X'33', 0x33);
>> +        CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT, u INT);
>> +        INSERT INTO t VALUES (X'33', 0x33, 0x00);
>
> Still don't understand the purpose of creating separate table and so on.
I removed this test, though I still do not think that there is
something wrong with it.

> Again: next/prev fields don't affect content of field 'A': blob is
> stored in msgpack alongside with its length, so OP_Column can't decode
> more/less bytes than indicated in msgpack.
In new commit-message you can see example (also in previous
letters), from which it can be seen, that second and third field
affects result of the CAST().

>
> What is more, found that your implementation relies on tt_cstr() which
> uses static buffer which in turn restricted by 3 * 4096 bytes. So users
> may get wrong results of cast with ease. Example:
>
> long_str = string.rep('0', 15000) 
> long_str = long_str..'123'
> box.execute(string.format("insert into test values(2, '%s')", long_str)) 
> box.execute("select cast(s as INTEGER) from test")
>
> Result is 0 meanwhile should lead to error.
>
Fixed.


New patch:

From 0450959d670b1a466f7bc2c3dccb6a5f6ca0a0b8 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 25 Mar 2020 13:34:20 +0300
Subject: [PATCH] sql: add '\0' to the BLOB when it is cast to INTEGER

Prior to this patch, due to the absence of the '\0' character at
the end of the BLOB, it was possible to get an error or incorrect
result when using CAST() from BLOB to INTEGER or UNSIGNED. This
has now been fixed, but the maximum length of a BLOB that could be
cast to INTEGER or UNSIGNED was limited to 12287 bytes.

Examples of wrong CAST() from BLOB to INTEGER:

Error during CAST():
tarantool> box.execute("CREATE TABLE t1 (a VARBINARY PRIMARY KEY);")
---
- row_count: 1
...

tarantool> box.execute("INSERT INTO t1 VALUES (X'33'), (X'372020202020');")
---
- row_count: 2
...

tarantool> box.execute("SELECT a, CAST(a AS INTEGER) FROM t1;")
---
- null
- 'Type mismatch: can not convert varbinary to integer'
...

Wrong result:
tarantool> box.execute("CREATE TABLE t2 (a VARBINARY PRIMARY KEY, i INT, u INT);")
---
- row_count: 1
...

tarantool> box.execute("INSERT INTO t2 VALUES (X'33', 0x33, 0x00);")
---
- row_count: 1
...

tarantool> box.execute("SELECT a, CAST(a AS INTEGER) FROM t2;")
---
- metadata:
  - name: A
    type: varbinary
  - name: CAST(a AS INTEGER)
    type: integer
  rows:
  - ['3', 33]
...

Closes #4766

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index f908e9c..c556b98 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -467,14 +467,21 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length)
 	if (*z == '-')
 		*is_neg = true;
 
+	/*
+	 * BLOB data may not end with '\0'. Because of this, the
+	 * strtoll() and strtoull() functions may return an
+	 * incorrect result. To fix this, let's copy the value for
+	 * decoding into static memory and add '\0' to it.
+	 */
+	if (length > SMALL_STATIC_SIZE - 1)
+		return -1;
+	const char *str_value = tt_cstr(z, length);
 	char *end = NULL;
 	errno = 0;
-	if (*z == '-') {
-		*is_neg = true;
-		*val = strtoll(z, &end, 10);
+	if (*is_neg) {
+		*val = strtoll(str_value, &end, 10);
 	} else {
-		*is_neg = false;
-		uint64_t u_val = strtoull(z, &end, 10);
+		uint64_t u_val = strtoull(str_value, &end, 10);
 		*val = u_val;
 	}
 	/* Overflow and underflow errors. */
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index fb0790d..42fdf81 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(79)
+test:plan(82)
 
 --!./tcltestrunner.lua
 -- 2005 June 25
@@ -871,4 +871,50 @@ test:do_execsql_test(
         -- </cast-5.1>
     })
 
+--
+-- gh-4766: 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(
+    "cast-6.1",
+    [[
+        CREATE TABLE t (a VARBINARY PRIMARY KEY);
+        INSERT INTO t VALUES (X'33'), (X'372020202020');
+        SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t;
+        DROP TABLE t;
+    ]], {
+        -- <cast-6.1>
+        '3', 3, 3, 3, '7     ', 7, 7, 7
+        -- </cast-6.1>
+    })
+
+--
+-- Make sure that BLOB longer than 12287 bytes cannot be cast to
+-- INTEGER.
+--
+long_str = string.rep('0', 12284)
+test:do_execsql_test(
+    "cast-6.2",
+    "SELECT CAST('" .. long_str .. "123'" .. " AS INTEGER);", {
+        -- <cast-6.2>
+        123
+        -- </cast-6.2>
+    })
+
+test:do_catchsql_test(
+    "cast-6.3",
+    "SELECT CAST('" .. long_str .. "1234'" .. " AS INTEGER);", {
+        -- <cast-6.3>
+        1, "Type mismatch: can not convert 000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "000000000000000000000000000000000000000000000"
+        -- </cast-6.3>
+    })
+
 test:finish_test()