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 DF8026F3D0; Wed, 1 Sep 2021 11:44:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DF8026F3D0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1630485893; bh=cJ7bW9zwCuctcS6NoikFGcGwJLLQrxycBjRZDJzw3aM=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Wl8gbEU7vz7vY6Dp3xgNE/UO572kYGhyJsLtG1bbb8qVPHUDtgT4EfxxiGyXY08yt Dcwrg0Ww1uSPJdah2XVQmqj1GpVp3Qh7UGwRD2WmcLASKTpvxkN0JUR/DmMQNnKvAO rLZ4SMdvnqHPcGBDQDp812wNDXZBlOjFqrZB40z4= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [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 0FBC86F3D0 for ; Wed, 1 Sep 2021 11:44:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0FBC86F3D0 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mLLrX-0003oT-7N; Wed, 01 Sep 2021 11:44:51 +0300 Date: Wed, 1 Sep 2021 11:44:50 +0300 To: Timur Safin Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210901084450.GA111802@tarantool.org> References: <9ec7b38b0979cb2e9ac6cb6b8f2e405c313a67f9.1630305008.git.imeevma@gmail.com> <017001d79e9e$f9d5f8d0$ed81ea70$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <017001d79e9e$f9d5f8d0$ed81ea70$@tarantool.org> X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9EE8D69D3379D80C4F2809F464D146B007F92475D298E3533182A05F538085040515FBB441F00F148A851B7A9C1D151579BD6A61F6790A545BAA7EE4034F2EA20 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78C6616F30072131EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374C29A2D8B98EBF198638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8063BF9F01ED65D6F0131E3392EE54FC1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A500B394417443DBACB119A90337EC1D9EF0B3286B8CB17C8BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756888F6138A55F47E410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34524B2D44961BF9C69E49FB41D9E69DC4C6D9A5E202E966E7BA5D91F731B6C6BC621E8E65889E02651D7E09C32AA3244C54B4C7D3D62045936C08676AD6F25ACE69B6CAE0477E908D729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKvNUqyKacB2CrmY74M/Azg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DF0361D58B95F9466A7419741D6E0FD7683D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix a segfault in hex() on receiving zeroblob 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thank you for the review. My answers below. On Tue, Aug 31, 2021 at 10:32:46PM +0300, Timur Safin wrote: > I may miss something obvious, but prior version of a code > with pBlob and n was much shorter, compacter and more readable. > I'm curious, why do you prefer to always use argv[0]->n and > argv[0]->z instead? > If we talk about the old function, then it really looks simpler. However, it did not work correctly and also made some unnecessary changes to the arguments. You can compare to the fixed version of old function on this branch: imeevma/gh-6113-fix-hex-segfault-2.8 (which I also sent you for review). You will see much less difference there. > Also, it seems to me we better to limit the number of bytes customer > may request to allocate from HEX()? What about to check against SQL_LIMIT_LENGTH? > This check is performed in the OP_BuiltinFunction opcode. > Thanks, > Timur > > > -----Original Message----- > > From: imeevma@tarantool.org > > Sent: Monday, August 30, 2021 9:31 AM > > To: tsafin@tarantool.org > > Cc: tarantool-patches@dev.tarantool.org > > Subject: [PATCH v1 1/1] sql: fix a segfault in hex() on receiving > > zeroblob > > > > This patch fixes a segmentation fault when zeroblob is received by > > the > > SQL built-in HEX() function. > > > > Closes #6113 > > --- > > https://github.com/tarantool/tarantool/issues/6113 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-6113-fix-hex- > > segfault-2.10 > > > > .../gh-6113-fix-segfault-in-hex-func.md | 5 ++ > > src/box/sql/func.c | 75 ++++++++++------- > > -- > > test/sql-tap/engine.cfg | 1 + > > ...gh-6113-assert-in-hex-on-zeroblob.test.lua | 13 ++++ > > 4 files changed, 58 insertions(+), 36 deletions(-) > > create mode 100644 changelogs/unreleased/gh-6113-fix-segfault-in- > > hex-func.md > > create mode 100755 test/sql-tap/gh-6113-assert-in-hex-on- > > zeroblob.test.lua > > > > diff --git a/changelogs/unreleased/gh-6113-fix-segfault-in-hex- > > func.md b/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md > > new file mode 100644 > > index 000000000..c59be4d96 > > --- /dev/null > > +++ b/changelogs/unreleased/gh-6113-fix-segfault-in-hex-func.md > > @@ -0,0 +1,5 @@ > > +## bugfix/sql > > + > > +* The HEX() SQL built-in function now does not throw an assert on > > receiving > > + varbinary values that consist of zero-bytes (gh-6113). > > + > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index c063552d6..fa2a2c245 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > @@ -53,6 +53,44 @@ > > static struct mh_strnptr_t *built_in_functions = NULL; > > static struct func_sql_builtin **functions; > > > > +/** Array for converting from half-bytes into ASCII hex digits. */ > > +static const char hexdigits[] = { > > + '0', '1', '2', '3', '4', '5', '6', '7', > > + '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' > > +}; > > + > > +/** Implementation of the HEX() SQL built-in function. */ > > +static void > > +func_hex(struct sql_context *ctx, int argc, struct Mem **argv) > > +{ > > + assert(argc == 1); > > + (void)argc; > > + if (argv[0]->type == MEM_TYPE_NULL) > > + return mem_set_null(ctx->pOut); > > + > > + assert(argv[0]->type == MEM_TYPE_BIN && argv[0]->n >= 0); > > + assert((argv[0]->flags & MEM_Zero) == 0 || argv[0]->u.nZero >= > > 0); > > + uint32_t size = 2 * argv[0]->n; > > + if ((argv[0]->flags & MEM_Zero) != 0) > > + size += 2 * argv[0]->u.nZero; > > + if (size == 0) > > + return mem_set_str0_static(ctx->pOut, ""); > > + > > + char *str = sqlDbMallocRawNN(sql_get(), size); > > + if (str == NULL) { > > + ctx->is_aborted = true; > > + return; > > + } > > + for (int i = 0; i < argv[0]->n; ++i) { > > + char c = argv[0]->z[i]; > > + str[2 * i] = hexdigits[(c >> 4) & 0xf]; > > + str[2 * i + 1] = hexdigits[c & 0xf]; > > + } > > + if ((argv[0]->flags & MEM_Zero) != 0) > > + memset(&str[2 * argv[0]->n], '0', 2 * argv[0]->u.nZero); > > + mem_set_str_allocated(ctx->pOut, str, size); > > +} > > + > > static const unsigned char * > > mem_as_ustr(struct Mem *mem) > > { > > @@ -1072,14 +1110,6 @@ sql_func_version(struct sql_context *context, > > sql_result_text(context, tarantool_version(), -1, SQL_STATIC); > > } > > > > -/* Array for converting from half-bytes (nybbles) into ASCII hex > > - * digits. > > - */ > > -static const char hexdigits[] = { > > - '0', '1', '2', '3', '4', '5', '6', '7', > > - '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' > > -}; > > - > > /* > > * Implementation of the QUOTE() function. This function takes a > > single > > * argument. If the argument is numeric, the return value is the > > same as > > @@ -1233,33 +1263,6 @@ charFunc(sql_context * context, int argc, > > sql_value ** argv) > > sql_result_text64(context, (char *)z, zOut - z, sql_free); > > } > > > > -/* > > - * The hex() function. Interpret the argument as a blob. Return > > - * a hexadecimal rendering as text. > > - */ > > -static void > > -hexFunc(sql_context * context, int argc, sql_value ** argv) > > -{ > > - int i, n; > > - const unsigned char *pBlob; > > - char *zHex, *z; > > - assert(argc == 1); > > - UNUSED_PARAMETER(argc); > > - pBlob = mem_as_bin(argv[0]); > > - n = mem_len_unsafe(argv[0]); > > - assert(pBlob == mem_as_bin(argv[0])); /* No encoding change */ > > - z = zHex = contextMalloc(context, ((i64) n) * 2 + 1); > > - if (zHex) { > > - for (i = 0; i < n; i++, pBlob++) { > > - unsigned char c = *pBlob; > > - *(z++) = hexdigits[(c >> 4) & 0xf]; > > - *(z++) = hexdigits[c & 0xf]; > > - } > > - *z = 0; > > - sql_result_text(context, zHex, n * 2, sql_free); > > - } > > -} > > - > > /* > > * The zeroblob(N) function returns a zero-filled blob of size N > > bytes. > > */ > > @@ -2034,7 +2037,7 @@ static struct sql_func_definition definitions[] > > = { > > {"GROUP_CONCAT", 2, {FIELD_TYPE_VARBINARY, > > FIELD_TYPE_VARBINARY}, > > FIELD_TYPE_VARBINARY, groupConcatStep, groupConcatFinalize}, > > > > - {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, hexFunc, > > NULL}, > > + {"HEX", 1, {FIELD_TYPE_VARBINARY}, FIELD_TYPE_STRING, func_hex, > > NULL}, > > {"IFNULL", 2, {FIELD_TYPE_ANY, FIELD_TYPE_ANY}, > > FIELD_TYPE_SCALAR, > > sql_builtin_stub, NULL}, > > > > diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg > > index 587adbed9..5ff0219fc 100644 > > --- a/test/sql-tap/engine.cfg > > +++ b/test/sql-tap/engine.cfg > > @@ -35,6 +35,7 @@ > > "built-in-functions.test.lua": { > > "memtx": {"engine": "memtx"} > > }, > > + "gh-6113-assert-in-hex-on-zeroblob.test.lua": {}, > > "gh-4077-iproto-execute-no-bind.test.lua": {}, > > "gh-6375-assert-on-unsupported-ext.test.lua": {}, > > "*": { > > diff --git a/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua > > b/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua > > new file mode 100755 > > index 000000000..91a29a5b4 > > --- /dev/null > > +++ b/test/sql-tap/gh-6113-assert-in-hex-on-zeroblob.test.lua > > @@ -0,0 +1,13 @@ > > +#!/usr/bin/env tarantool > > +local test = require("sqltester") > > +test:plan(1) > > + > > +test:do_execsql_test( > > + "gh-6113", > > + [[ > > + SELECT hex(zeroblob(0)), hex(zeroblob(10)); > > + ]], { > > + '', '00000000000000000000' > > + }) > > + > > +test:finish_test() > > -- > > 2.25.1 > >