From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0D6B0469719 for ; Thu, 13 Feb 2020 11:17:02 +0300 (MSK) From: imeevma@tarantool.org Date: Thu, 13 Feb 2020 11:16:57 +0300 Message-Id: <4f7cb05d8161597c0e58520b75af04167ce0b5e6.1581580784.git.imeevma@gmail.com> Subject: [Tarantool-patches] [PATCH v2 1/1] sql: limit blob size during CAST AS INTEGER List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org This patch limits the number of decoded bytes during CAST from BLOB to INTEGER according to the specified BLOB size. Closes #4766 --- https://github.com/tarantool/tarantool/issues/4766 https://github.com/tarantool/tarantool/tree/imeevma/gh-4766-fix-blob-size-for-cast @ChangeLog - added '\0' to the end of VARBINARY when casting to INTEGER or UNSIGNED (gh-4766). src/box/sql/util.c | 13 ++++++++++--- test/sql-tap/cast.test.lua | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 4 deletions(-) Hi! Thank you for review. I send a new version to add a changelog. My answers, diff and new patch below. On 2/11/20 5:50 PM, Nikita Pettik wrote: > On 11 Feb 12:52, imeevma@tarantool.org wrote: >> This patch limits the number of decoded bytes during CAST from >> BLOB to INTEGER according to the specified BLOB size. >> >> Closes #4766 > > Note that issue has no assigned milestone. As far as I remember it is > not recommended to work on such issues (personally I do not care, but > I guess Kirill does). What is more, according to our new policy, each > patch should come with change-log: see 'Support ChangeLogs' chapter in SOP. > Thanks, fixed. Also, 2.2.3 milestone was assigned. >> >> diff --git a/src/box/sql/util.c b/src/box/sql/util.c >> index f908e9c..8f70ce6 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'. This leads to an error >> + * in the strtoll() and strtoull() functions. > > Nit: does strtoll() raise an error, or return wrong result? > Return wrong result. Fixed the comment. >> To fix this, >> + * let's copy the value for decoding into static memory >> + * and add '\0' to it. >> + */ >> + const char *str_value = tt_cstr(z, length); >> char *end = NULL; >> errno = 0; >> - if (*z == '-') { >> + if (*str_value == '-') { >> *is_neg = true; >> - *val = strtoll(z, &end, 10); >> + *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 9c937a0..a579f24 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(85) >> +test:plan(87) >> >> --!./tcltestrunner.lua >> -- 2005 June 25 >> @@ -905,4 +905,34 @@ test:do_execsql_test( >> -- >> }) >> >> +-- >> +-- gh-4766: Make sure that a blob as part of a tuple can be cast >> +-- to NUMBER, INTEGER and UNSIGNED. > > I'd outline the fact that bug appears due to blob may not be terminated > with \0 while stored in space. So make sure that function providing > conversion from blob to numbers does account blob's lenght. > Changed the comment. Added info about '\0'. >> +-- >> +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; >> + ]], { >> + -- >> + '3', 3, 3, 3, '7 ', 7, 7, 7 >> + -- >> + }) >> + >> +test:do_execsql_test( >> + "cast-6.2", >> + [[ >> + CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT); >> + INSERT INTO t VALUES (X'33', 1); >> + SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t; >> + DROP TABLE t; > > What's the (in functional sense) difference between 6.1 and 6.2? > True, in this test it isn't obvious what it should show. I fixed the test. I made this test to show that the '\0' at the end of the BLOB is important. One possible way to solve this problem was to limit the length of the decoded BLOB (without copying and adding '\0'), but this could lead to an incorrect result of this test. I mean this: tarantool> CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT) WITH ENGINE = 'vinyl'; --- - row_count: 1 ... tarantool> INSERT INTO t VALUES (X'33', 0x33); --- - row_count: 1 ... tarantool> SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t; --- - metadata: - name: A type: varbinary - name: CAST(a AS NUMBER) type: number - name: CAST(a AS INTEGER) type: integer - name: CAST(a AS UNSIGNED) type: unsigned rows: - ['3', 33, 33, 33] ... tarantool> DROP TABLE t; --- - row_count: 1 ... >> + ]], { >> + -- >> + '3', 3, 3, 3 >> + -- >> + }) >> + >> test:finish_test() >> -- >> 2.7.4 >> Diff: diff --git a/src/box/sql/util.c b/src/box/sql/util.c index 8f70ce6..b01b1ec 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -468,10 +468,10 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length) *is_neg = true; /* - * BLOB data may not end with '\0'. This leads to an error - * in the strtoll() and strtoull() functions. To fix this, - * let's copy the value for decoding into static memory - * and add '\0' to it. + * 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. */ const char *str_value = tt_cstr(z, length); char *end = NULL; diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua index b1ef27a..86c0fee 100755 --- a/test/sql-tap/cast.test.lua +++ b/test/sql-tap/cast.test.lua @@ -873,7 +873,8 @@ test:do_execsql_test( -- -- gh-4766: Make sure that a blob as part of a tuple can be cast --- to NUMBER, INTEGER and UNSIGNED. +-- 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", @@ -888,11 +889,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. +-- test:do_execsql_test( "cast-6.2", [[ CREATE TABLE t (a VARBINARY PRIMARY KEY, i INT); - INSERT INTO t VALUES (X'33', 1); + INSERT INTO t VALUES (X'33', 0x33); SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t; DROP TABLE t; ]], { New patch: >From 4f7cb05d8161597c0e58520b75af04167ce0b5e6 Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Wed, 5 Feb 2020 16:38:33 +0300 Subject: [PATCH] sql: limit blob size during CAST AS INTEGER This patch limits the number of decoded bytes during CAST from BLOB to INTEGER according to the specified BLOB size. Closes #4766 diff --git a/src/box/sql/util.c b/src/box/sql/util.c index f908e9c..b01b1ec 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. + */ + const char *str_value = tt_cstr(z, length); char *end = NULL; errno = 0; - if (*z == '-') { + if (*str_value == '-') { *is_neg = true; - *val = strtoll(z, &end, 10); + *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..86c0fee 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(81) --!./tcltestrunner.lua -- 2005 June 25 @@ -871,4 +871,39 @@ test:do_execsql_test( -- }) +-- +-- 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; + ]], { + -- + '3', 3, 3, 3, '7 ', 7, 7, 7 + -- + }) + +-- +-- In some cases, the absence of '\0' could lead to an incorrect +-- result. 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); + SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t; + DROP TABLE t; + ]], { + -- + '3', 3, 3, 3 + -- + }) + test:finish_test()