From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 6ACB76BD2D; Tue, 13 Apr 2021 15:01:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6ACB76BD2D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618315280; bh=JkbY2UGGbgcTQxqmvbc5l6cRR/e/PDHU/hFAFkcHRF8=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=IOYGsz2EOkdiRxe20/E+mfSK0tbbYFMqd17aw33p3Hsdb7g+8Nmz/5Ruopc9AQWZy T5zMKoPT1EptTetImK6oFYxbYVv+0GCxzr/LyeEh4BsYmRopvBIZaKKiHUE+7yCUn5 eeWTA0059LKp9slGDCJNMd8jIY99ozteu0n8XkGc= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 24DCA6BD29 for ; Tue, 13 Apr 2021 15:01:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 24DCA6BD29 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lWHjK-0001uS-3L; Tue, 13 Apr 2021 15:01:18 +0300 Date: Tue, 13 Apr 2021 15:01:16 +0300 To: Vladislav Shpilevoy Message-ID: <20210413120116.GA95443@tarantool.org> References: <3fda2029b186d0cea67ba01269dc3d6e209b3c89.1617984948.git.imeevma@gmail.com> <57dd6994-64f3-7645-f0ec-376751bd0b3e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <57dd6994-64f3-7645-f0ec-376751bd0b3e@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E74809299FB6B3996B874F289A033C44AD400182A05F538085040CFD8D60FABF82BC6C9235A3D2AFFAE9519DFB05B3A23706CDBEA13CFB2456EBC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A85479CDD055C83FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063702DFA59B3C994360EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA6589694FB1B053EFA956A2AB5D511F0893CF6B57BC7E64490618DEB871D839B73339E8FC8737B5C22498424CA1AAF98A6958941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B68424CA1AAF98A6958941B15DA834481F9449624AB7ADAF372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F9FFED5BD9FB4175535872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5B296D54A6EB8CE7B484FFA2C281E41B008BAB117AAE91FC0D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C264B329661203DA891CD84E61FF175E1953EBA71472DE7F064075016E32B241321711E900714DBD1D7E09C32AA3244CD9F2A09C6B87C546841A4799A6C3AF6324AF4FAF06DA24FDFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXFiE4o8+LA3Sg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822B577E28C80AD910FF4A7AEDF87370FBE83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 01/52] sql: enhance vdbe_decode_msgpack_into_mem() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! I'a sorry to ask you so late, but could you look at new version of the patch? It was decided that this patch should be pushed to 2.6 and 2.7 along with master. Only this patch of the patch-set. Diff and new version below. Please note, that I added additional SELECT to the first test since currently sql-tap test system does not support binary result. Also, in the last two tests I created new table for each since this way we could see proper error. If I used space T there then in the last test there would be the same error as in the test 5 due to UUID being before DECIMAL in space format. To remove dependence of their position, I added two new spaces. On Sun, Apr 11, 2021 at 07:42:22PM +0200, Vladislav Shpilevoy wrote: > Hi! Good job on the patch! > > On 09.04.2021 18:51, Mergen Imeev via Tarantool-patches wrote: > > Hi! Thank you for the review! My answer and new patch below. I didn't include > > diffs in answers since due to merge conflicts they are partly useless. > > > > On 30.03.2021 01:57, Vladislav Shpilevoy wrote: > >> Hi! I appreciate the work you did here! > >> > >> Truly huge patch. But it does the important thing which I think > >> should give a huge help in the task of SQL code rework. > >> > >> On 23.03.2021 10:34, Mergen Imeev via Tarantool-patches wrote: > >>> Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not > >>> properly initialized in case msgpack contains MP_EXT, MP_MAP, or > >>> MP_ARRAY fields. Also, it doesn't set field_type. > >> > >> AFAIR, field type wasn't set deliberately. Because we use it only for > >> strictly typed values obtained from formatted space fields. It wasn't > >> applied to plain non-formatted values on purpose. > >> > >> Why do you change that? > > > > I didn't know about that. I thought that all MEMs that contains data should have > > field_type set. However, I tried to understand where did this field come from > > and found that it was added for two purposes: to show difference between NUMBER > > and INTEGER in MEM before DOUBLE was added and to show column name instead of > > type determined from MP-type in typeof(). I believe that both these purposes are > > not needed now and that this field should be removed from struct MEM. I created > > an issue for this: #5957. However, I was prohibited to remove this field for now > > by @tsafin, who believes that this field is actually important. > > I tend to agree, that it must be done separately. Although probably could be done > before this patchset to make it kind of simpler. Anyway, thanks for the > explanation. Diff: diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua new file mode 100755 index 000000000..60978c2b5 --- /dev/null +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua @@ -0,0 +1,99 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(6) + +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111") +local decimal = require("decimal").new(111.111) + +box.schema.create_space('T') +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"}, + {name = "D", type = "decimal"}}) +box.space.T:create_index("primary") +box.space.T:insert({1, uuid, decimal}) + +-- +-- Make sure that there is no segmentation fault on select from field that +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL, +-- so they treated as VARBINARY. +-- +test:do_execsql_test( + "gh-5913-1", + [[ + SELECT i, u, d FROM t; + SELECT i from t; + ]], { + 1 + }) + +box.schema.create_space('T1') +box.space.T1:format({{name = "I", type = "integer"}, + {name = "U", type = "uuid", is_nullable = true}, + {name = "D", type = "decimal", is_nullable = true}}) +box.space.T1:create_index("primary") + +-- +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY, +-- they cannot be inserted from SQL. +-- +test:do_catchsql_test( + "gh-5913-2", + [[ + INSERT INTO t1 SELECT i, u, NULL FROM t; + ]], { + 1, "Type mismatch: can not convert varbinary to uuid" + }) + +test:do_catchsql_test( + "gh-5913-3", + [[ + INSERT INTO t1 SELECT i, NULL, d FROM t; + ]], { + 1, "Type mismatch: can not convert varbinary to decimal" + }) + +-- +-- Still, if UUID or DECIMAL fields does not selected directly, insert is +-- working properly. +-- +test:do_execsql_test( + "gh-5913-4", + [[ + INSERT INTO t1 SELECT * FROM t; + SELECT count() FROM t1; + ]], { + 1 + }) + +box.schema.create_space('TU') +box.space.TU:format({{name = "I", type = "integer"}, + {name = "U", type = "uuid"}}) +box.space.TU:create_index("primary") +box.space.TU:insert({1, uuid}) + +box.schema.create_space('TD') +box.space.TD:format({{name = "I", type = "integer"}, + {name = "D", type = "decimal"}}) +box.space.TD:create_index("primary") +box.space.TD:insert({1, decimal}) + +-- +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an +-- error since after changing value cannot be inserted into the field from SQL. +-- +test:do_catchsql_test( + "gh-5913-5", + [[ + UPDATE tu SET u = u; + ]], { + 1, "Type mismatch: can not convert varbinary to uuid" + }) + +test:do_catchsql_test( + "gh-5913-6", + [[ + UPDATE td SET d = d; + ]], { + 1, "Type mismatch: can not convert varbinary to decimal" + }) + +test:finish_test() New patch: commit d41d8f756f8ab6bfae70ee5787bc4c509ddb844d Author: Mergen Imeev Date: Thu Mar 4 17:17:18 2021 +0300 sql: enhance vdbe_decode_msgpack_into_mem() Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not properly initialized in case msgpack contains MP_EXT, MP_MAP, or MP_ARRAY fields. This patch makes it so that after execution of this function, all created MEMs are properly initialized. Closes #5011 Closes #5704 Closes #5913 Needed for #5818 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 3b3b1f01d..9a4f38bb9 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -846,16 +846,6 @@ vdbe_field_ref_fetch_data(struct vdbe_field_ref *field_ref, uint32_t fieldno) return field_begin; } -static inline enum field_type -vdbe_field_ref_fetch_type(struct vdbe_field_ref *field_ref, uint32_t fieldno) -{ - const struct tuple_field *tf = - vdbe_field_ref_fetch_field(field_ref, fieldno); - if (tf == NULL || tf->type == FIELD_TYPE_ANY) - return field_type_MAX; - return tf->type; -} - /** * Fetch field by fieldno using vdbe_field_ref and store result * in dest_mem. @@ -879,17 +869,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno, if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0) return -1; - /* - * MsgPack map, array or extension (unsupported in sql). - * Wrap it in a blob verbatim. - */ - if (dest_mem->flags == 0) { - dest_mem->z = (char *) data; - dest_mem->n = vdbe_field_ref_fetch_data(field_ref, - fieldno + 1) - data; - dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; - dest_mem->subtype = SQL_SUBTYPE_MSGPACK; - } /* * Add 0 termination (at most for strings) * Not sure why do we check MEM_Ephem @@ -909,7 +888,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno, dest_mem->flags |= MEM_Term; } UPDATE_MAX_BLOBSIZE(dest_mem); - dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno); return 0; } diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 91b64316e..772476377 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -2793,38 +2793,62 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len) { const char *start_buf = buf; switch (mp_typeof(*buf)) { - case MP_ARRAY: - case MP_MAP: - case MP_EXT: - default: { - mem->flags = 0; + case MP_ARRAY: { + mem->z = (char *)buf; + mp_next(&buf); + mem->n = buf - mem->z; + mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; + mem->subtype = SQL_SUBTYPE_MSGPACK; + mem->field_type = FIELD_TYPE_ARRAY; + break; + } + case MP_MAP: { + mem->z = (char *)buf; + mp_next(&buf); + mem->n = buf - mem->z; + mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; + mem->subtype = SQL_SUBTYPE_MSGPACK; + mem->field_type = FIELD_TYPE_MAP; + break; + } + case MP_EXT: { + mem->z = (char *)buf; + mp_next(&buf); + mem->n = buf - mem->z; + mem->flags = MEM_Blob | MEM_Ephem; + mem->field_type = FIELD_TYPE_VARBINARY; break; } case MP_NIL: { mp_decode_nil(&buf); mem->flags = MEM_Null; + mem->field_type = field_type_MAX; break; } case MP_BOOL: { mem->u.b = mp_decode_bool(&buf); mem->flags = MEM_Bool; + mem->field_type = FIELD_TYPE_BOOLEAN; break; } case MP_UINT: { uint64_t v = mp_decode_uint(&buf); mem->u.u = v; mem->flags = MEM_UInt; + mem->field_type = FIELD_TYPE_INTEGER; break; } case MP_INT: { mem->u.i = mp_decode_int(&buf); mem->flags = MEM_Int; + mem->field_type = FIELD_TYPE_INTEGER; break; } case MP_STR: { /* XXX u32->int */ mem->n = (int) mp_decode_strl(&buf); mem->flags = MEM_Str | MEM_Ephem; + mem->field_type = FIELD_TYPE_STRING; install_blob: mem->z = (char *)buf; buf += mem->n; @@ -2834,18 +2858,33 @@ install_blob: /* XXX u32->int */ mem->n = (int) mp_decode_binl(&buf); mem->flags = MEM_Blob | MEM_Ephem; + mem->field_type = FIELD_TYPE_VARBINARY; goto install_blob; } case MP_FLOAT: { mem->u.r = mp_decode_float(&buf); - mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real; + if (sqlIsNaN(mem->u.r)) { + mem->flags = MEM_Null; + mem->field_type = FIELD_TYPE_DOUBLE; + } else { + mem->flags = MEM_Real; + mem->field_type = FIELD_TYPE_DOUBLE; + } break; } case MP_DOUBLE: { mem->u.r = mp_decode_double(&buf); - mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real; + if (sqlIsNaN(mem->u.r)) { + mem->flags = MEM_Null; + mem->field_type = FIELD_TYPE_DOUBLE; + } else { + mem->flags = MEM_Real; + mem->field_type = FIELD_TYPE_DOUBLE; + } break; } + default: + unreachable(); } *len = (uint32_t)(buf - start_buf); return 0; @@ -2868,15 +2907,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def, /* Information about the rec pMem->z = 0; 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; - mp_next(&zParse); - pMem->n = zParse - pMem->z; - pMem->flags = MEM_Blob | MEM_Ephem; - } else { - zParse += sz; - } + assert(sz != 0); + zParse += sz; pMem++; } } diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua new file mode 100755 index 000000000..60978c2b5 --- /dev/null +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua @@ -0,0 +1,99 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(6) + +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111") +local decimal = require("decimal").new(111.111) + +box.schema.create_space('T') +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"}, + {name = "D", type = "decimal"}}) +box.space.T:create_index("primary") +box.space.T:insert({1, uuid, decimal}) + +-- +-- Make sure that there is no segmentation fault on select from field that +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL, +-- so they treated as VARBINARY. +-- +test:do_execsql_test( + "gh-5913-1", + [[ + SELECT i, u, d FROM t; + SELECT i from t; + ]], { + 1 + }) + +box.schema.create_space('T1') +box.space.T1:format({{name = "I", type = "integer"}, + {name = "U", type = "uuid", is_nullable = true}, + {name = "D", type = "decimal", is_nullable = true}}) +box.space.T1:create_index("primary") + +-- +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY, +-- they cannot be inserted from SQL. +-- +test:do_catchsql_test( + "gh-5913-2", + [[ + INSERT INTO t1 SELECT i, u, NULL FROM t; + ]], { + 1, "Type mismatch: can not convert varbinary to uuid" + }) + +test:do_catchsql_test( + "gh-5913-3", + [[ + INSERT INTO t1 SELECT i, NULL, d FROM t; + ]], { + 1, "Type mismatch: can not convert varbinary to decimal" + }) + +-- +-- Still, if UUID or DECIMAL fields does not selected directly, insert is +-- working properly. +-- +test:do_execsql_test( + "gh-5913-4", + [[ + INSERT INTO t1 SELECT * FROM t; + SELECT count() FROM t1; + ]], { + 1 + }) + +box.schema.create_space('TU') +box.space.TU:format({{name = "I", type = "integer"}, + {name = "U", type = "uuid"}}) +box.space.TU:create_index("primary") +box.space.TU:insert({1, uuid}) + +box.schema.create_space('TD') +box.space.TD:format({{name = "I", type = "integer"}, + {name = "D", type = "decimal"}}) +box.space.TD:create_index("primary") +box.space.TD:insert({1, decimal}) + +-- +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an +-- error since after changing value cannot be inserted into the field from SQL. +-- +test:do_catchsql_test( + "gh-5913-5", + [[ + UPDATE tu SET u = u; + ]], { + 1, "Type mismatch: can not convert varbinary to uuid" + }) + +test:do_catchsql_test( + "gh-5913-6", + [[ + UPDATE td SET d = d; + ]], { + 1, "Type mismatch: can not convert varbinary to decimal" + }) + +test:finish_test()