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 B54B06EC55; Mon, 6 Sep 2021 23:32:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B54B06EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1630960331; bh=pFmkiqMIosBcW0drjLUHK8o010Iy2XwM8QJC9pXS3K8=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=xbYF5xnQGqkJJbPRK8+hjQaFnuP2n4SnvmDggDrk7no/y+FvL0TKOmZ96o1KgSvKA roBLM2jGKAGot01Bc1RqU9nIwcWj7wdOd3+ghLqdwqqu0RsRp1v8ZnHr+63kaHZUo3 Hc1jiItXRZa2PMXFSP8rBDjJf33VAcLsoHkRQ6k0= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 817766EC55 for ; Mon, 6 Sep 2021 23:32:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 817766EC55 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1mNLHk-0003SY-HX; Mon, 06 Sep 2021 23:32:09 +0300 To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org References: <9ec7b38b0979cb2e9ac6cb6b8f2e405c313a67f9.1630305008.git.imeevma@gmail.com> <017001d79e9e$f9d5f8d0$ed81ea70$@tarantool.org> <20210901084450.GA111802@tarantool.org> <73f26e5c-9374-682a-5787-0da49b32953c@tarantool.org> <20210906094528.GA24664@tarantool.org> Message-ID: <7ac838fc-44cf-ccf0-2ca2-c126be437cb2@tarantool.org> Date: Mon, 6 Sep 2021 23:32:06 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210906094528.GA24664@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D96C1EA41D18F4D58D04F3E644628B13AEFE843B3F5982AA182A05F5380850400AE2B150495175EDF93BBE502C2B9E14B6F8429F9AF033EEB96B66615BE4A6A6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FDE19FEC90BA7BD7C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE72DF3470F92485B86EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2A5EB5464CF3D4B593E74685E4252248DCC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8D2DCF9CF1F528DBCF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CB355ED1E20F5346AC0837EA9F3D197644AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C32EC1AE196BA1C124BA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE732FCE54C4D9A645443847C11F186F3C59DAA53EE0834AAEE X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79DB8BF922E402C092C0755FF37656E04DA X-C1DE0DAB: 0D63561A33F958A50E4A3F3DAAC84AAE85C5831419AA9676BE8F86A09B6D5DE1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341C998A3771F04153BB9299F2B8A816F9AB930B7846623795A0F8F2F7828EB1044EF33614DF0E1BFF1D7E09C32AA3244C232FC4910171808DBAA92BABE7939A8530452B15D76AEC14729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj2BdsxQzaueVcYvZW2yBDBw== X-Mailru-Sender: B5B6A6EBBD94DAD86C3E0F9779482A87236B994FC374911AC54EF8998CF1AF8C892BE17A424FB3AC1EC9E4A2C82A33BC8C24925A86E657CE0C70AEE3C9A96FBAB3D7EE8ED63280BE112434F685709FCF0DA7A0AF5A3A8387 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: Safin Timur via Tarantool-patches Reply-To: Safin Timur Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On 06.09.2021 12:45, Mergen Imeev wrote: > Hi! Thank you for the review! My answer below. > > On Fri, Sep 03, 2021 at 10:19:56PM +0300, Safin Timur wrote: >> >> >> On 01.09.2021 11:44, Mergen Imeev wrote: >>> 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. >> >> I meant that newer code was a little bit .. mouthful, with unnecessary code >> substitution and visual noise which harmed readability. Here is an example >> of version which is not using argv[0]->.. wherever we refer to fields. >> >> ---------------------------------------------------- >> /** 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); >> >> int n = argv[0]->n; >> int zero_len = argv[0]->u.nZero; > I believe you cannot use undefined value. That's good question, and wording in standard C is(was) confusing here, and one could get an impression that it's UB to acsess field of union, other than that has been initialized last. It's called type-punning. And always was just implemented as a trickier way for "reinterpret cast" which is compatible with aliasing analysis in compiler. There is correndum in C99 defect report which clarifies that: http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm ``` 78a If the member used to access the contents of a union object is not the same as the member last used to store a value in the object, the appropriate part of the object representation of the value is reinterpreted as an object representation in the new type as described in 6.2.6 (a process sometimes called "type punning"). This might be a trap representation. ``` i.e. it's either cast to asked type, or trap of such cast is impossible (like if target type is double, and value in memory creates invalid floating value, and host hardware programed to raise in such cases). In this case we always get integer value, moreover we use this value below __only__ if argv[0]->flags & MEM_Zero is non-zero, so we reuse this value only in correct circumstances. So, in short, that's ok in code below. > >> assert(argv[0]->type == MEM_TYPE_BIN && n >= 0); >> assert((argv[0]->flags & MEM_Zero) == 0 || zero_len >= 0); >> >> uint32_t size = 2 * n; >> if ((argv[0]->flags & MEM_Zero) != 0) >> size += 2 * zero_len; >> 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 < 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 * n], '0', 2 * zero_len); >> mem_set_str_allocated(ctx->pOut, str, size); >> } >> >> ---------------------------------------------------- >> >> It's more resembling original code (and that was done intentionally). >> > I don't like that you define a variable with an undefined value in some cases. > I would introduce some new variables if there was some complicated logic, > however I don't see the need to do this here since I don't see complex > expressions. I still insist that excessive usage of argv[0]->xxx in every line make this code uglier and less readable. Please, make code less verbose. > >> Also (and I didn't change it in the sample) there is apparent missing check >> for SQL_LIMIT_LENGTH limit which used to be done in contextMalloc() before, >> but now is missing once we use sqlDbMallocRawNN(). I assume we better return >> this check (once again as a proper wrapper which contextMalloc() essentially >> was). >> > This will be verified in VDBE. I think it is better to have such a check > centralized for all functions. Is it already verified at the moment? Or you meant it __will be__ verified eventually in future code. In general, though, we may not assume that the code will always be called in correct context where all bounds checks processed. So assertions should be local, placed where they are assumed to be present. Remember recent discussion elsewhere - assertions may not be too much. Please put them here. (And in any case, they will not hurt performance in release mode, even if there will be coincidentally some duplicates here and 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. >> >> That's nice, so it's not a problem then. Though, assertions help, as I said above... >> >>> >>>> Thanks, >>>> Timur >>>> 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}, >>>>> >> >> Regards, >> Timur