From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 A5C97441856 for ; Wed, 25 Mar 2020 14:38:09 +0300 (MSK) From: imeevma@tarantool.org Date: Wed, 25 Mar 2020 14:38:08 +0300 Message-Id: <0450959d670b1a466f7bc2c3dccb6a5f6ca0a0b8.1585135623.git.imeevma@gmail.com> In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v3 2/2] sql: add '\0' to the BLOB when it is cast to INTEGER List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: korablev@tarantool.org, tsafin@tarantool.org, tarantool-patches@dev.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 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( -- }) +-- +-- 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 + -- + }) + +-- +-- 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);", { + -- + 123 + -- + }) + +test:do_catchsql_test( + "cast-6.3", + "SELECT CAST('" .. long_str .. "1234'" .. " AS INTEGER);", { + -- + 1, "Type mismatch: can not convert 000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "000000000000000000000000000000000000000000000" + -- + }) + test:finish_test()