* [tarantool-patches] [PATCH 1/4] sql: raise an error if int is overflowed during math operations
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
@ 2019-02-20 11:57 ` Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 2/4] sql: raise an integer overflow error during CAST Nikita Pettik
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Nikita Pettik @ 2019-02-20 11:57 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Before this patch, if integer was overflowed during math operations
(OP_Add, OP_Subtract, OP_Multiply, OP_Divide), it would be implicitly
converted and stored as floating point number. This is obviously wrong
way to handle integer overflow errors. Instead, let's raise
corresponding error.
Part of #3735
---
src/box/sql/vdbe.c | 13 +++++++-----
test/sql/integer-overflow.result | 42 ++++++++++++++++++++++++++++++++++++++
test/sql/integer-overflow.test.lua | 16 +++++++++++++++
3 files changed, 66 insertions(+), 5 deletions(-)
create mode 100644 test/sql/integer-overflow.result
create mode 100644 test/sql/integer-overflow.test.lua
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c370b81ec..d9797b22b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1614,13 +1614,13 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */
iB = pIn2->u.i;
bIntint = 1;
switch( pOp->opcode) {
- case OP_Add: if (sqlAddInt64(&iB,iA)) goto fp_math; break;
- case OP_Subtract: if (sqlSubInt64(&iB,iA)) goto fp_math; break;
- case OP_Multiply: if (sqlMulInt64(&iB,iA)) goto fp_math; break;
+ case OP_Add: if (sqlAddInt64(&iB,iA)) goto integer_overflow; break;
+ case OP_Subtract: if (sqlSubInt64(&iB,iA)) goto integer_overflow; break;
+ case OP_Multiply: if (sqlMulInt64(&iB,iA)) goto integer_overflow; break;
case OP_Divide: {
if (iA == 0)
goto division_by_zero;
- if (iA==-1 && iB==SMALLEST_INT64) goto fp_math;
+ if (iA==-1 && iB==SMALLEST_INT64) goto integer_overflow;
iB /= iA;
break;
}
@@ -1636,7 +1636,6 @@ case OP_Remainder: { /* same as TK_REM, in1, in2, out3 */
MemSetTypeFlag(pOut, MEM_Int);
} else {
bIntint = 0;
- fp_math:
if (sqlVdbeRealValue(pIn1, &rA) != 0) {
diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
sql_value_text(pIn1), "numeric");
@@ -1694,6 +1693,10 @@ division_by_zero:
diag_set(ClientError, ER_SQL_EXECUTE, "division by zero");
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
+integer_overflow:
+ diag_set(ClientError, ER_SQL_EXECUTE, "integer is overflowed");
+ rc = SQL_TARANTOOL_ERROR;
+ goto abort_due_to_error;
}
/* Opcode: CollSeq P1 * * P4
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
new file mode 100644
index 000000000..fedcd4290
--- /dev/null
+++ b/test/sql/integer-overflow.result
@@ -0,0 +1,42 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+...
+-- gh-3735: make sure that integer overflows errors are
+-- handled during VDBE execution.
+--
+box.sql.execute('SELECT (2147483647 * 2147483647 * 2147483647);')
+---
+- error: 'Failed to execute SQL statement: integer is overflowed'
+...
+box.sql.execute('SELECT (-9223372036854775808 / -1);')
+---
+- error: 'Failed to execute SQL statement: integer is overflowed'
+...
+box.sql.execute('SELECT (-9223372036854775808 - 1);')
+---
+- error: 'Failed to execute SQL statement: integer is overflowed'
+...
+box.sql.execute('SELECT (9223372036854775807 + 1);')
+---
+- error: 'Failed to execute SQL statement: integer is overflowed'
+...
+-- Literals are checked right after parsing.
+--
+box.sql.execute('SELECT 9223372036854775808;')
+---
+- error: 'oversized integer: 9223372036854775808'
+...
+box.sql.execute('SELECT -9223372036854775809;')
+---
+- error: 'oversized integer: -9223372036854775809'
+...
+box.sql.execute('SELECT 9223372036854775808 - 1;')
+---
+- error: 'oversized integer: 9223372036854775808'
+...
diff --git a/test/sql/integer-overflow.test.lua b/test/sql/integer-overflow.test.lua
new file mode 100644
index 000000000..49ba5cd8f
--- /dev/null
+++ b/test/sql/integer-overflow.test.lua
@@ -0,0 +1,16 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+
+-- gh-3735: make sure that integer overflows errors are
+-- handled during VDBE execution.
+--
+box.sql.execute('SELECT (2147483647 * 2147483647 * 2147483647);')
+box.sql.execute('SELECT (-9223372036854775808 / -1);')
+box.sql.execute('SELECT (-9223372036854775808 - 1);')
+box.sql.execute('SELECT (9223372036854775807 + 1);')
+-- Literals are checked right after parsing.
+--
+box.sql.execute('SELECT 9223372036854775808;')
+box.sql.execute('SELECT -9223372036854775809;')
+box.sql.execute('SELECT 9223372036854775808 - 1;')
--
2.15.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [tarantool-patches] [PATCH 2/4] sql: raise an integer overflow error during CAST
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 1/4] sql: raise an error if int is overflowed during math operations Nikita Pettik
@ 2019-02-20 11:57 ` Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 3/4] sql: refactor sqlVdbeMsgpackGet() Nikita Pettik
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Nikita Pettik @ 2019-02-20 11:57 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Before this patch, if integer overflow occurred during casting to
integer, it was simply ignored. As a result, wrong results might take
place. Lets check possible overflows before CAST conversion, and if it
happens, raise an appropriate error.
Part of #3735
---
src/box/sql/vdbemem.c | 4 +++-
test/sql/integer-overflow.result | 16 ++++++++++++++++
test/sql/integer-overflow.test.lua | 10 ++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index e9ace33d2..a816936f0 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -532,7 +532,9 @@ sqlVdbeMemIntegerify(Mem * pMem, bool is_forced)
MemSetTypeFlag(pMem, MEM_Int);
return 0;
} else if ((pMem->flags & MEM_Real) != 0 && is_forced) {
- pMem->u.i = (int) pMem->u.r;
+ if (pMem->u.r >= INT64_MAX || pMem->u.r < INT64_MIN)
+ return -1;
+ pMem->u.i = (int64_t) pMem->u.r;
MemSetTypeFlag(pMem, MEM_Int);
return 0;
}
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
index fedcd4290..762ebbf29 100644
--- a/test/sql/integer-overflow.result
+++ b/test/sql/integer-overflow.result
@@ -40,3 +40,19 @@ box.sql.execute('SELECT 9223372036854775808 - 1;')
---
- error: 'oversized integer: 9223372036854775808'
...
+-- Test that CAST may also leads to overflow.
+--
+box.sql.execute('SELECT CAST(\'9223372036854775808\' AS INTEGER);')
+---
+- error: 'Type mismatch: can not convert 9223372036854775808 to integer'
+...
+-- Due to inexact represantation of large integers in terms of
+-- floating point numbers, numerics with value < INT64_MAX
+-- have INT64_MAX + 1 value in integer representation:
+-- float 9223372036854775800 -> int (9223372036854775808),
+-- with error due to conversion = 8.
+--
+box.sql.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
+---
+- error: 'Type mismatch: can not convert 9.22337203685478e+18 to integer'
+...
diff --git a/test/sql/integer-overflow.test.lua b/test/sql/integer-overflow.test.lua
index 49ba5cd8f..ec7eb433e 100644
--- a/test/sql/integer-overflow.test.lua
+++ b/test/sql/integer-overflow.test.lua
@@ -14,3 +14,13 @@ box.sql.execute('SELECT (9223372036854775807 + 1);')
box.sql.execute('SELECT 9223372036854775808;')
box.sql.execute('SELECT -9223372036854775809;')
box.sql.execute('SELECT 9223372036854775808 - 1;')
+-- Test that CAST may also leads to overflow.
+--
+box.sql.execute('SELECT CAST(\'9223372036854775808\' AS INTEGER);')
+-- Due to inexact represantation of large integers in terms of
+-- floating point numbers, numerics with value < INT64_MAX
+-- have INT64_MAX + 1 value in integer representation:
+-- float 9223372036854775800 -> int (9223372036854775808),
+-- with error due to conversion = 8.
+--
+box.sql.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
--
2.15.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [tarantool-patches] [PATCH 3/4] sql: refactor sqlVdbeMsgpackGet()
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 1/4] sql: raise an error if int is overflowed during math operations Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 2/4] sql: raise an integer overflow error during CAST Nikita Pettik
@ 2019-02-20 11:57 ` Nikita Pettik
2019-02-20 11:57 ` [tarantool-patches] [PATCH 4/4] sql: raise integer overflow error during msgpack decode Nikita Pettik
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Nikita Pettik @ 2019-02-20 11:57 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Tarantool allows to hold in INTEGER field values in range
[INT64_MAX, UINT64_MAX], which is obviously larger than common int64_t
range. In this regard, if value of integer field in range
[INT64_MAX, UINT64_MAX] is presented in tuple (e.g. after insertion from
Lua interface), then after decoding msgpack (during processing SQL
query) its value won't fit into int64_t (which in turn is basic type
used to hold integers inside VDBE memory). Now if this happens, instead
of raising an overflow error, value is stored as floating point number
(with precise loss, obviously). Such approach is considered to be messy
and we are going to raise "integer overflow" error instead. To make this
happen, lets firstly refactor sqlVdbeMsgpackGet() to make it return
error code to indicate that something went wrong and move length of
decoded part to output parameters. Codestyle fixes are included as well.
Needed for #3735
---
src/box/sql/vdbe.c | 5 +-
src/box/sql/vdbeInt.h | 13 ++++-
src/box/sql/vdbeaux.c | 132 ++++++++++++++++++++++++--------------------------
src/box/sql/vdbemem.c | 4 +-
4 files changed, 81 insertions(+), 73 deletions(-)
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d9797b22b..d38f61774 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2737,8 +2737,9 @@ case OP_Column: {
if (VdbeMemDynamic(pDest)) {
sqlVdbeMemSetNull(pDest);
}
-
- sqlVdbeMsgpackGet(zData+aOffset[p2], pDest);
+ uint32_t unused;
+ vdbe_decode_msgpack_into_mem((const char *)(zData + aOffset[p2]),
+ pDest, &unused);
/* MsgPack map, array or extension (unsupported in sql).
* Wrap it in a blob verbatim.
*/
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index d003d6bb3..49f665bb0 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -547,7 +547,18 @@ int sqlVdbeCompareMsgpack(const char **key1,
*/
int sqlVdbeRecordCompareMsgpack(const void *key1,
struct UnpackedRecord *key2);
-u32 sqlVdbeMsgpackGet(const unsigned char *buf, Mem * pMem);
+
+/**
+ * Decode msgpack and save value into VDBE memory cell.
+ *
+ * @param buf Buffer to deserialize msgpack from.
+ * @param mem Memory cell to write value into.
+ * @param len[out] Length of decoded part.
+ * @retval Return code: < 0 in case of error.
+ * @retval 0 on success.
+ */
+int
+vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len);
struct mpstream;
struct region;
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index b831b52ad..ba9b96645 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3688,79 +3688,74 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
return key2->default_rc;
}
-u32
-sqlVdbeMsgpackGet(const unsigned char *buf, /* Buffer to deserialize from */
- Mem * pMem) /* Memory cell to write value into */
+int
+vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
{
- const char *zParse = (const char *)buf;
- switch (mp_typeof(*zParse)) {
+ const char *start_buf = buf;
+ switch (mp_typeof(*buf)) {
case MP_ARRAY:
case MP_MAP:
case MP_EXT:
- default:{
- pMem->flags = 0;
- return 0;
- }
- case MP_NIL:{
- mp_decode_nil((const char **)&zParse); /* Still need to promote zParse. */
- pMem->flags = MEM_Null;
- break;
- }
- case MP_BOOL:{
- assert((unsigned char)*zParse == 0xc2
- || (unsigned char)*zParse == 0xc3);
- pMem->u.i = (unsigned char)*zParse - 0xc2;
- pMem->flags = MEM_Int;
- break;
- }
- case MP_UINT:{
- uint64_t v = mp_decode_uint(&zParse);
- if (v > INT64_MAX) {
- /*
- * If the value exceeds i64 range, convert to double (lossy).
- */
- pMem->u.r = v;
- pMem->flags = MEM_Real;
- } else {
- pMem->u.i = v;
- pMem->flags = MEM_Int;
- }
- break;
- }
- case MP_INT:{
- pMem->u.i = mp_decode_int(&zParse);
- pMem->flags = MEM_Int;
- break;
- }
- case MP_STR:{
- /* XXX u32->int */
- pMem->n = (int)mp_decode_strl((const char **)&zParse);
- pMem->flags = MEM_Str | MEM_Ephem;
- install_blob:
- pMem->z = (char *)zParse;
- zParse += pMem->n;
- break;
- }
- case MP_BIN:{
- /* XXX u32->int */
- pMem->n = (int)mp_decode_binl((const char **)&zParse);
- pMem->flags = MEM_Blob | MEM_Ephem;
- goto install_blob;
- }
- case MP_FLOAT:{
- pMem->u.r = mp_decode_float(&zParse);
- pMem->flags =
- sqlIsNaN(pMem->u.r) ? MEM_Null : MEM_Real;
- break;
- }
- case MP_DOUBLE:{
- pMem->u.r = mp_decode_double(&zParse);
- pMem->flags =
- sqlIsNaN(pMem->u.r) ? MEM_Null : MEM_Real;
- break;
+ default: {
+ mem->flags = 0;
+ break;
+ }
+ case MP_NIL: {
+ mp_decode_nil(&buf);
+ mem->flags = MEM_Null;
+ break;
+ }
+ case MP_BOOL: {
+ assert((unsigned char)*buf == 0xc2 ||
+ (unsigned char)*buf == 0xc3);
+ mem->u.i = (unsigned char)*buf - 0xc2;
+ mem->flags = MEM_Int;
+ break;
+ }
+ case MP_UINT: {
+ uint64_t v = mp_decode_uint(&buf);
+ if (v > INT64_MAX) {
+ mem->u.r = v;
+ mem->flags = MEM_Real;
+ } else {
+ mem->u.i = v;
+ mem->flags = MEM_Int;
}
+ break;
+ }
+ case MP_INT: {
+ mem->u.i = mp_decode_int(&buf);
+ mem->flags = MEM_Int;
+ break;
+ }
+ case MP_STR: {
+ /* XXX u32->int */
+ mem->n = (int) mp_decode_strl(&buf);
+ mem->flags = MEM_Str | MEM_Ephem;
+install_blob:
+ mem->z = (char *)buf;
+ buf += mem->n;
+ break;
+ }
+ case MP_BIN: {
+ /* XXX u32->int */
+ mem->n = (int) mp_decode_binl(&buf);
+ mem->flags = MEM_Blob | MEM_Ephem;
+ goto install_blob;
}
- return (u32) (zParse - (const char *)buf);
+ case MP_FLOAT: {
+ mem->u.r = mp_decode_float(&buf);
+ mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+ break;
+ }
+ case MP_DOUBLE: {
+ mem->u.r = mp_decode_double(&buf);
+ mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real;
+ break;
+ }
+ }
+ *len = (uint32_t)(buf - start_buf);
+ return 0;
}
void
@@ -3778,7 +3773,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def, /* Information about the rec
while (n--) {
pMem->szMalloc = 0;
pMem->z = 0;
- u32 sz = sqlVdbeMsgpackGet((u8 *) zParse, pMem);
+ uint32_t sz = 0;
+ vdbe_decode_msgpack_into_mem(zParse, pMem, &sz);
if (sz == 0) {
/* MsgPack array, map or ext. Treat as blob. */
pMem->z = (char *)zParse;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index a816936f0..19802f746 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1634,8 +1634,8 @@ sql_stat4_column(struct sql *db, const char *record, uint32_t col_num,
return -1;
}
}
- sqlVdbeMsgpackGet((const unsigned char *) a, mem);
- return 0;
+ uint32_t unused;
+ return vdbe_decode_msgpack_into_mem(a, mem, &unused);
}
/*
--
2.15.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [tarantool-patches] [PATCH 4/4] sql: raise integer overflow error during msgpack decode
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
` (2 preceding siblings ...)
2019-02-20 11:57 ` [tarantool-patches] [PATCH 3/4] sql: refactor sqlVdbeMsgpackGet() Nikita Pettik
@ 2019-02-20 11:57 ` Nikita Pettik
2019-02-20 18:25 ` [tarantool-patches] " Konstantin Osipov
2019-02-22 18:30 ` [tarantool-patches] Re: [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Vladislav Shpilevoy
2019-02-25 11:58 ` Kirill Yukhin
5 siblings, 1 reply; 10+ messages in thread
From: Nikita Pettik @ 2019-02-20 11:57 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Since previous commit allows us to raise an error during msgpack decode
inside VDBE, lets do this if decoded integer is out of
[INT64_MIN, INT64_MAX] range and set "integer is overflowed" diagnostic
message.
Closes #3735
Workaround for #3810
---
src/box/sql/vdbe.c | 7 +++++--
src/box/sql/vdbeaux.c | 10 +++++-----
test/sql/integer-overflow.result | 18 ++++++++++++++++++
test/sql/integer-overflow.test.lua | 8 ++++++++
4 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d38f61774..dd3797fc0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2738,8 +2738,11 @@ case OP_Column: {
sqlVdbeMemSetNull(pDest);
}
uint32_t unused;
- vdbe_decode_msgpack_into_mem((const char *)(zData + aOffset[p2]),
- pDest, &unused);
+ if (vdbe_decode_msgpack_into_mem((const char *)(zData + aOffset[p2]),
+ pDest, &unused) != 0) {
+ rc = SQL_TARANTOOL_ERROR;
+ goto abort_due_to_error;
+ }
/* MsgPack map, array or extension (unsupported in sql).
* Wrap it in a blob verbatim.
*/
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index ba9b96645..4df58f20c 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3715,12 +3715,12 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len)
case MP_UINT: {
uint64_t v = mp_decode_uint(&buf);
if (v > INT64_MAX) {
- mem->u.r = v;
- mem->flags = MEM_Real;
- } else {
- mem->u.i = v;
- mem->flags = MEM_Int;
+ diag_set(ClientError, ER_SQL_EXECUTE,
+ "integer is overflowed");
+ return -1;
}
+ mem->u.i = v;
+ mem->flags = MEM_Int;
break;
}
case MP_INT: {
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
index 762ebbf29..4754c046c 100644
--- a/test/sql/integer-overflow.result
+++ b/test/sql/integer-overflow.result
@@ -56,3 +56,21 @@ box.sql.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
---
- error: 'Type mismatch: can not convert 9.22337203685478e+18 to integer'
...
+-- gh-3810: make sure that if space contains integers in range
+-- [INT64_MAX, UINT64_MAX], they are handled inside SQL in a
+-- proper way, which now means that an error is raised.
+--
+box.sql.execute('CREATE TABLE t (id INT PRIMARY KEY);')
+---
+...
+box.space.T:insert({9223372036854775809})
+---
+- [9223372036854775808]
+...
+box.sql.execute('SELECT * FROM t;')
+---
+- error: 'Failed to execute SQL statement: integer is overflowed'
+...
+box.space.T:drop()
+---
+...
diff --git a/test/sql/integer-overflow.test.lua b/test/sql/integer-overflow.test.lua
index ec7eb433e..45fc209fd 100644
--- a/test/sql/integer-overflow.test.lua
+++ b/test/sql/integer-overflow.test.lua
@@ -24,3 +24,11 @@ box.sql.execute('SELECT CAST(\'9223372036854775808\' AS INTEGER);')
-- with error due to conversion = 8.
--
box.sql.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
+-- gh-3810: make sure that if space contains integers in range
+-- [INT64_MAX, UINT64_MAX], they are handled inside SQL in a
+-- proper way, which now means that an error is raised.
+--
+box.sql.execute('CREATE TABLE t (id INT PRIMARY KEY);')
+box.space.T:insert({9223372036854775809})
+box.sql.execute('SELECT * FROM t;')
+box.space.T:drop()
--
2.15.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [tarantool-patches] Re: [PATCH 4/4] sql: raise integer overflow error during msgpack decode
2019-02-20 11:57 ` [tarantool-patches] [PATCH 4/4] sql: raise integer overflow error during msgpack decode Nikita Pettik
@ 2019-02-20 18:25 ` Konstantin Osipov
2019-02-20 18:39 ` n.pettik
0 siblings, 1 reply; 10+ messages in thread
From: Konstantin Osipov @ 2019-02-20 18:25 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
* Nikita Pettik <korablev@tarantool.org> [19/02/20 15:12]:
> Since previous commit allows us to raise an error during msgpack decode
> inside VDBE, lets do this if decoded integer is out of
> [INT64_MIN, INT64_MAX] range and set "integer is overflowed" diagnostic
> message.
This looks OK as interim approach to close 3735, but can't be
accepted as a production-level fix. If SQL can't handle uint64_t
range, SQL tables should not be able to store values from this
range, i.e. there should be an implicitly created constraint on
SQL tables. Better yet is to make sure SQL can handle entire
tarantool range by changing vdbe to emit uint64-range-aware
instructions.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: raise integer overflow error during msgpack decode
2019-02-20 18:25 ` [tarantool-patches] " Konstantin Osipov
@ 2019-02-20 18:39 ` n.pettik
2019-02-20 18:46 ` Konstantin Osipov
0 siblings, 1 reply; 10+ messages in thread
From: n.pettik @ 2019-02-20 18:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: Konstantin Osipov, Vladislav Shpilevoy
> On 20 Feb 2019, at 21:25, Konstantin Osipov <kostja@tarantool.org> wrote:
>
> * Nikita Pettik <korablev@tarantool.org> [19/02/20 15:12]:
>> Since previous commit allows us to raise an error during msgpack decode
>> inside VDBE, lets do this if decoded integer is out of
>> [INT64_MIN, INT64_MAX] range and set "integer is overflowed" diagnostic
>> message.
>
> This looks OK as interim approach to close 3735, but can't be
> accepted as a production-level fix. If SQL can't handle uint64_t
> range, SQL tables should not be able to store values from this
> range, i.e. there should be an implicitly created constraint on
> SQL tables. Better yet is to make sure SQL can handle entire
> tarantool range by changing vdbe to emit uint64-range-aware
> instructions.
It is to be implemented in scope of #3810 issue. Without this patch
operations on integers values in range [INT64_MAX, UIN64_MAX]
lead to unpredictable results. So, I guess you won’t argue that
error raising as at least temporary fix is significantly better than
misleading results.
> --
> Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
> http://tarantool.io - www.twitter.com/kostja_osipov
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: raise integer overflow error during msgpack decode
2019-02-20 18:39 ` n.pettik
@ 2019-02-20 18:46 ` Konstantin Osipov
0 siblings, 0 replies; 10+ messages in thread
From: Konstantin Osipov @ 2019-02-20 18:46 UTC (permalink / raw)
To: n.pettik; +Cc: tarantool-patches, Vladislav Shpilevoy
* n.pettik <korablev@tarantool.org> [19/02/20 21:41]:
>
> It is to be implemented in scope of #3810 issue. Without this patch
> operations on integers values in range [INT64_MAX, UIN64_MAX]
> lead to unpredictable results. So, I guess you won’t argue that
> error raising as at least temporary fix is significantly better than
> misleading results.
I agree.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Fix integer overflow behaviour during VDBE execution
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
` (3 preceding siblings ...)
2019-02-20 11:57 ` [tarantool-patches] [PATCH 4/4] sql: raise integer overflow error during msgpack decode Nikita Pettik
@ 2019-02-22 18:30 ` Vladislav Shpilevoy
2019-02-25 11:58 ` Kirill Yukhin
5 siblings, 0 replies; 10+ messages in thread
From: Vladislav Shpilevoy @ 2019-02-22 18:30 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik, Kirill Yukhin
LGTM.
On 20/02/2019 14:57, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3735-integer-overflow
> Issue: https://github.com/tarantool/tarantool/issues/3735
>
> Current patch-set fixes integer overflow behaviour during VDBE
> execution. Originally, SQLite acts quite misleading in such
> situations: intead of raising error, it simply treats overflowed
> integers as floating point numbers. Lets fix it everywhere it is
> possible (math and CAST operators, validate decoded msgpack).
>
> Nikita Pettik (4):
> sql: raise an error if int is overflowed during math operations
> sql: raise an integer overflow error during CAST
> sql: refactor sqlVdbeMsgpackGet()
> sql: raise integer overflow error during msgpack decode
>
> src/box/sql/vdbe.c | 21 ++++--
> src/box/sql/vdbeInt.h | 13 +++-
> src/box/sql/vdbeaux.c | 132 ++++++++++++++++++-------------------
> src/box/sql/vdbemem.c | 8 ++-
> test/sql/integer-overflow.result | 76 +++++++++++++++++++++
> test/sql/integer-overflow.test.lua | 34 ++++++++++
> 6 files changed, 205 insertions(+), 79 deletions(-)
> create mode 100644 test/sql/integer-overflow.result
> create mode 100644 test/sql/integer-overflow.test.lua
>
> --
> 2.15.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [tarantool-patches] Re: [PATCH 0/4] Fix integer overflow behaviour during VDBE execution
2019-02-20 11:57 [tarantool-patches] [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Nikita Pettik
` (4 preceding siblings ...)
2019-02-22 18:30 ` [tarantool-patches] Re: [PATCH 0/4] Fix integer overflow behaviour during VDBE execution Vladislav Shpilevoy
@ 2019-02-25 11:58 ` Kirill Yukhin
5 siblings, 0 replies; 10+ messages in thread
From: Kirill Yukhin @ 2019-02-25 11:58 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Hello,
On 20 Feb 14:57, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3735-integer-overflow
> Issue: https://github.com/tarantool/tarantool/issues/3735
>
> Current patch-set fixes integer overflow behaviour during VDBE
> execution. Originally, SQLite acts quite misleading in such
> situations: intead of raising error, it simply treats overflowed
> integers as floating point numbers. Lets fix it everywhere it is
> possible (math and CAST operators, validate decoded msgpack).
I've checked your patches into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 10+ messages in thread