* [Tarantool-patches] [PATCH v3 1/2] sql: fix CAST() from STRING to INTEGER
2020-03-25 11:38 [Tarantool-patches] [PATCH v3 0/2] sql: fix CAST() from BLOB to INTEGER imeevma
@ 2020-03-25 11:38 ` imeevma
2020-03-25 18:10 ` Nikita Pettik
2020-03-25 11:38 ` [Tarantool-patches] [PATCH v3 2/2] sql: add '\0' to the BLOB when it is cast " imeevma
2020-03-25 17:46 ` [Tarantool-patches] [PATCH v3 0/2] sql: fix CAST() from BLOB " Nikita Pettik
2 siblings, 1 reply; 7+ messages in thread
From: imeevma @ 2020-03-25 11:38 UTC (permalink / raw)
To: korablev, tsafin, tarantool-patches
Prior to this patch, STRING, which contains the DOUBLE value,
could be cast to INTEGER. This was done by converting STRING to
DOUBLE and then converting this DOUBLE value to INTEGER. This may
affect the accuracy of CAST(), so it was forbidden.
Before patch:
tarantool> box.execute("SELECT CAST('1.1' as INTEGER);")
---
- metadata:
- name: CAST('1.1' as INTEGER)
type: integer
rows:
- [1]
...
After patch:
tarantool> box.execute("SELECT CAST('1.1' as INTEGER);")
---
- null
- 'Type mismatch: can not convert 1.1 to integer'
...
Part of #4766
---
src/box/sql/vdbe.c | 11 +++++++--
src/box/sql/vdbeInt.h | 1 -
src/box/sql/vdbemem.c | 45 +++++++++++-------------------------
test/sql-tap/e_select1.test.lua | 2 +-
test/sql-tap/intpkey.test.lua | 2 +-
test/sql-tap/join.test.lua | 4 ++--
test/sql-tap/subquery.test.lua | 6 ++---
test/sql-tap/tkt-9a8b09f8e6.test.lua | 4 ++--
test/sql/types.result | 23 ++++++++----------
9 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a..6c0e5bd 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -319,6 +319,8 @@ mem_apply_type(struct Mem *record, enum field_type type)
switch (type) {
case FIELD_TYPE_INTEGER:
case FIELD_TYPE_UNSIGNED:
+ if ((record->flags & (MEM_Bool | MEM_Blob)) != 0)
+ return -1;
if ((record->flags & MEM_UInt) == MEM_UInt)
return 0;
if ((record->flags & MEM_Real) == MEM_Real) {
@@ -331,8 +333,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
mem_set_u64(record, u);
return 0;
}
- if (sqlVdbeMemIntegerify(record) != 0)
- return -1;
+ if ((record->flags & MEM_Str) != 0) {
+ bool is_neg;
+ int64_t i;
+ if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0)
+ return -1;
+ mem_set_int(record, i, is_neg);
+ }
if ((record->flags & MEM_Int) == MEM_Int) {
if (type == FIELD_TYPE_UNSIGNED)
return -1;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 38305ce..2c50b67 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -525,7 +525,6 @@ int sqlVdbeMemMakeWriteable(Mem *);
int sqlVdbeMemStringify(Mem *);
int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg);
-int sqlVdbeMemIntegerify(struct Mem *pMem);
int sqlVdbeRealValue(Mem *, double *);
int
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index aad030d..3bc7303 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -551,35 +551,6 @@ mem_apply_integer_type(Mem *pMem)
}
/*
- * Convert pMem to type integer. Invalidate any prior representations.
- */
-int
-sqlVdbeMemIntegerify(struct Mem *pMem)
-{
- assert(EIGHT_BYTE_ALIGNMENT(pMem));
-
- int64_t i;
- bool is_neg;
- if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) {
- mem_set_int(pMem, i, is_neg);
- return 0;
- }
-
- double d;
- if (sqlVdbeRealValue(pMem, &d) != 0)
- return -1;
- if (d < INT64_MAX && d >= INT64_MIN) {
- mem_set_int(pMem, d, d <= -1);
- return 0;
- }
- if (d >= INT64_MAX && d < UINT64_MAX) {
- mem_set_u64(pMem, d);
- return 0;
- }
- return -1;
-}
-
-/*
* Convert pMem so that it is of type MEM_Real.
* Invalidate any prior representations.
*/
@@ -696,7 +667,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
return -1;
case FIELD_TYPE_INTEGER:
case FIELD_TYPE_UNSIGNED:
- if ((pMem->flags & MEM_Blob) != 0) {
+ if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
bool is_neg;
int64_t val;
if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
@@ -711,8 +682,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
MemSetTypeFlag(pMem, MEM_UInt);
return 0;
}
- if (sqlVdbeMemIntegerify(pMem) != 0)
+ if ((pMem->flags & MEM_Real) != 0) {
+ double d;
+ if (sqlVdbeRealValue(pMem, &d) != 0)
+ return -1;
+ if (d < INT64_MAX && d >= INT64_MIN) {
+ mem_set_int(pMem, d, d <= -1);
+ return 0;
+ }
+ if (d >= INT64_MAX && d < UINT64_MAX) {
+ mem_set_u64(pMem, d);
+ return 0;
+ }
return -1;
+ }
if (type == FIELD_TYPE_UNSIGNED &&
(pMem->flags & MEM_UInt) == 0)
return -1;
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index c1818dd..1d3b964 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -2195,7 +2195,7 @@ test:do_select_tests(
{"1", "SELECT b FROM f1 ORDER BY a LIMIT 0 ", {}},
{"2", "SELECT b FROM f1 ORDER BY a DESC LIMIT 4 ", {"z", "y", "x", "w"}},
{"3", "SELECT b FROM f1 ORDER BY a DESC LIMIT 8 ", {"z", "y", "x", "w", "v", "u", "t", "s"}},
- {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12.0' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
+ {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
})
-- EVIDENCE-OF: R-54935-19057 Or, if the SELECT statement would return
diff --git a/test/sql-tap/intpkey.test.lua b/test/sql-tap/intpkey.test.lua
index bec2670..b6b1866 100755
--- a/test/sql-tap/intpkey.test.lua
+++ b/test/sql-tap/intpkey.test.lua
@@ -788,7 +788,7 @@ test:do_execsql_test(
test:do_execsql_test(
"intpkey-13.2",
[[
- INSERT INTO t1 VALUES('1.0',2,3);
+ INSERT INTO t1 VALUES('1',2,3);
SELECT * FROM t1 WHERE a=1;
]], {
-- <intpkey-13.2>
diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
index 4f014e0..840b780 100755
--- a/test/sql-tap/join.test.lua
+++ b/test/sql-tap/join.test.lua
@@ -1017,7 +1017,7 @@ test:do_test(
return test:execsql [[
CREATE TABLE t1(a TEXT primary key, b TEXT);
CREATE TABLE t2(b INTEGER primary key, a TEXT);
- INSERT INTO t1 VALUES('one', '1.0');
+ INSERT INTO t1 VALUES('one', '1');
INSERT INTO t1 VALUES('two', '2');
INSERT INTO t2 VALUES(1, 'one');
INSERT INTO t2 VALUES(2, 'two');
@@ -1034,7 +1034,7 @@ test:do_execsql_test(
SELECT * FROM t1 NATURAL JOIN t2
]], {
-- <join-11.9>
- "one", "1.0", "two", "2"
+ "one", "1", "two", "2"
-- </join-11.9>
})
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index 6bedf58..15c4c82 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -342,7 +342,7 @@ test:do_execsql_test(
INSERT INTO t3 VALUES(10);
CREATE TABLE t4(x TEXT PRIMARY KEY);
- INSERT INTO t4 VALUES('10.0');
+ INSERT INTO t4 VALUES('10');
]], {
-- <subquery-2.5.1>
@@ -363,7 +363,7 @@ test:do_test(
]]
end, {
-- <subquery-2.5.2>
- "10.0"
+ "10"
-- </subquery-2.5.2>
})
@@ -378,7 +378,7 @@ test:do_test(
]]
end, {
-- <subquery-2.5.3.1>
- "10.0"
+ "10"
-- </subquery-2.5.3.1>
})
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index db0881c..cb5348a 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -196,7 +196,7 @@ test:do_execsql_test(
test:do_execsql_test(
3.4,
[[
- SELECT x FROM t2 WHERE x IN ('1.0');
+ SELECT x FROM t2 WHERE x IN ('1');
]], {
-- <3.4>
1
@@ -236,7 +236,7 @@ test:do_execsql_test(
test:do_execsql_test(
3.8,
[[
- SELECT x FROM t2 WHERE '1.0' IN (x);
+ SELECT x FROM t2 WHERE '1' IN (x);
]], {
-- <3.8>
1
diff --git a/test/sql/types.result b/test/sql/types.result
index 38e4385..54aff46 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -269,11 +269,8 @@ box.space.T1:drop()
--
box.execute("SELECT CAST('1.123' AS INTEGER);")
---
-- metadata:
- - name: CAST('1.123' AS INTEGER)
- type: integer
- rows:
- - [1]
+- null
+- 'Type mismatch: can not convert 1.123 to integer'
...
box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
---
@@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
...
box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
---
-- metadata:
- - name: CAST(f AS INTEGER)
- type: integer
- rows:
- - [0]
- - [1]
- - [3]
+- null
+- 'Type mismatch: can not convert 0.0 to integer'
...
box.space.T1:drop()
---
@@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
...
box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
---
-- null
-- 'Type mismatch: can not convert -1 to unsigned'
+- metadata:
+ - name: CAST(-1.5 AS UNSIGNED)
+ type: unsigned
+ rows:
+ - [-1]
...
box.execute("SELECT CAST(true AS UNSIGNED);")
---
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread* [Tarantool-patches] [PATCH v3 2/2] sql: add '\0' to the BLOB when it is cast to INTEGER
2020-03-25 11:38 [Tarantool-patches] [PATCH v3 0/2] sql: fix CAST() from BLOB to INTEGER imeevma
2020-03-25 11:38 ` [Tarantool-patches] [PATCH v3 1/2] sql: fix CAST() from STRING " imeevma
@ 2020-03-25 11:38 ` imeevma
2020-03-25 18:17 ` Nikita Pettik
2020-03-25 17:46 ` [Tarantool-patches] [PATCH v3 0/2] sql: fix CAST() from BLOB " Nikita Pettik
2 siblings, 1 reply; 7+ messages in thread
From: imeevma @ 2020-03-25 11:38 UTC (permalink / raw)
To: korablev, tsafin, tarantool-patches
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()
^ permalink raw reply [flat|nested] 7+ messages in thread