From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7A13F21BC1 for ; Tue, 18 Dec 2018 09:30:40 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dWTs63EEPSix for ; Tue, 18 Dec 2018 09:30:40 -0500 (EST) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B063E21B9C for ; Tue, 18 Dec 2018 09:30:39 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] sql: fix bug with BLOB TRIM() when X'00' in char set References: <20181215105741.28464-1-roman.habibov@tarantool.org> From: Roman Message-ID: <3b2ab896-d36f-3d19-e740-34d9db5d91f0@tarantool.org> Date: Tue, 18 Dec 2018 17:30:36 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, "n.pettik" Hi! Thanks for review. > Nit: ‘… after its’ -> ‘… after it’. > > Nit: inculding -> including. > > Nit: ‘are then ignored’ -> ‘are ignored’. > > Nit: I wouldn’t mix first and second conditions: > ‘If the char set contains …, all characters are ignored...' Fixed. > You should put links to the branch and issue below delimiter ---, not above. Sorry. > Lets start from putting here comments which explain > what is going on here (how you algorithm/approach works). > Then, lets choose more suitable names for variables: > length, sizeInChar and nChar sound very similar > (if you can’t come up with better names, then at least > mention in comment what they mean). > >> + int sizeInChar = sqlite3_value_bytes(argv[1]); >> + int length = 0; > What is the difference between sizeInChar, length and nChar? I add comments and renamed vars. Hope, now it's clearer. > We use explicit conditions as a predicate value. In other words: > > while (sizeInChar - length > 0) > > Also, it would be better if you declare and initialise var at once: > > const unsigned char *z = zCharSet; Fixed. >> + zStepBack = z; >> SQLITE_SKIP_UTF8(z); >> + length += z - zStepBack; >> + nChar++; >> } >> if (nChar > 0) { >> azChar = >> @@ -1235,10 +1243,16 @@ trimFunc(sqlite3_context * context, int argc, sqlite3_value ** argv) >> return; >> } >> aLen = (unsigned char *)&azChar[nChar]; >> - for (z = zCharSet, nChar = 0; *z; nChar++) { >> + z = zCharSet; >> + nChar = 0; >> + length = 0; >> + while(sizeInChar - length) { >> azChar[nChar] = (unsigned char *)z; >> + zStepBack = z; >> SQLITE_SKIP_UTF8(z); >> + length += z - zStepBack; >> aLen[nChar] = (u8) (z - azChar[nChar]); >> + nChar++; >> } >> } >> } >> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua >> index 393212968..b7b8e7c4c 100755 >> --- a/test/sql-tap/func.test.lua >> +++ b/test/sql-tap/func.test.lua >> @@ -1,6 +1,6 @@ >> #!/usr/bin/env tarantool >> test = require("sqltester") >> -test:plan(14535) >> +test:plan(14547) >> >> --!./tcltestrunner.lua >> -- 2001 September 15 >> @@ -2100,11 +2100,133 @@ test:do_execsql_test( >> -- >> }) >> >> -- This is to test the deprecated sqlite3_aggregate_count() API. >> -- >> --test:do_test( >> -- "func-23.1", >> --- function() >> +-- function()S > Garbage diff. Sorry. I's an accident. commit 39b2481341cd2e71d67a04bec9aeed0e1189740c Author: Roman Khabibov Date:   Sat Dec 15 13:21:59 2018 +0300     sql: fix bug with BLOB TRIM() when X'00' in char set     The reason for the bug was that X'00' is a terminal symbol. If the char set     contained X'00', all characters are ignored after it (including itself).     Closes #3543 diff --git a/src/box/sql/func.c b/src/box/sql/func.c index 9667aead5..f397e23c1 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1223,9 +1223,19 @@ trimFunc(sqlite3_context * context, int argc, sqlite3_value ** argv)      } else if ((zCharSet = sqlite3_value_text(argv[1])) == 0) {          return;      } else { -        const unsigned char *z; -        for (z = zCharSet, nChar = 0; *z; nChar++) { +        const unsigned char *z = zCharSet; +        int sizeOfCharSet = \ +        sqlite3_value_bytes(argv[1]); /* Size of char set in bytes. */ +        int nProcessedBytes = 0; +        nChar = 0; +        const unsigned char *zStepBack; +        /* Count the number of UTF-8 characters passing through the +        * entire char set, but not up to the '\0' or X'00' character. */ +        while(sizeOfCharSet - nProcessedBytes > 0) { +            zStepBack = z;              SQLITE_SKIP_UTF8(z); +            nProcessedBytes += z - zStepBack; +            nChar++;          }          if (nChar > 0) {              azChar = @@ -1235,10 +1245,18 @@ trimFunc(sqlite3_context * context, int argc, sqlite3_value ** argv)                  return;              }              aLen = (unsigned char *)&azChar[nChar]; -            for (z = zCharSet, nChar = 0; *z; nChar++) { +            z = zCharSet; +            nChar = 0; +            nProcessedBytes = 0; +            /* Similar to the previous cycle. But +            * now write into "azCharSet". */ +            while(sizeOfCharSet - nProcessedBytes > 0) {                  azChar[nChar] = (unsigned char *)z; +                zStepBack = z;                  SQLITE_SKIP_UTF8(z); +                nProcessedBytes += z - zStepBack;                  aLen[nChar] = (u8) (z - azChar[nChar]); +                nChar++;              }          }      } diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua index 393212968..b7de1d955 100755 --- a/test/sql-tap/func.test.lua +++ b/test/sql-tap/func.test.lua @@ -1,6 +1,6 @@  #!/usr/bin/env tarantool  test = require("sqltester") -test:plan(14535) +test:plan(14547)  --!./tcltestrunner.lua  -- 2001 September 15 @@ -2100,6 +2100,128 @@ test:do_execsql_test(          --      }) +-- gh-3543 Check trimming of binary string when X'00' in trimming char set. + +test:do_execsql_test( +    "func-22.23", +    [[ +        SELECT TRIM(X'004100', X'00'); +    ]], { +        -- +        "A" +        -- +    }) + +test:do_execsql_test( +    "func-22.24", +    [[ +        SELECT TRIM(X'004100', X'0000'); +    ]], { +        -- +        "A" +        -- +    }) + +test:do_execsql_test( +    "func-22.25", +    [[ +        SELECT TRIM(X'004100', X'0042'); +    ]], { +        -- +        "A" +        -- +    }) + +test:do_execsql_test( +    "func-22.26", +    [[ +        SELECT TRIM(X'00004100420000', X'00'); +    ]], { +        -- +        "A\0B" +        -- +    }) + +test:do_execsql_test( +    "func-22.27", +    [[ +        SELECT LTRIM(X'004100', X'00'); +    ]], { +        -- +        "A\0" +        -- +    }) + +test:do_execsql_test( +    "func-22.28", +    [[ +        SELECT LTRIM(X'004100', X'0000'); +    ]], { +        -- +        "A\0" +        -- +    }) + +test:do_execsql_test( +    "func-22.29", +    [[ +        SELECT LTRIM(X'004100', X'0042'); +    ]], { +        -- +        "A\0" +        -- +    }) + +test:do_execsql_test( +    "func-22.30", +    [[ +        SELECT LTRIM(X'00004100420000', X'00'); +    ]], { +        -- +        "A\0B\0\0" +        -- +    }) + +test:do_execsql_test( +    "func-22.31", +    [[ +        SELECT RTRIM(X'004100', X'00'); +    ]], { +        -- +        "\0A" +        -- +    }) + +test:do_execsql_test( +    "func-22.32", +    [[ +        SELECT RTRIM(X'004100', X'0000'); +    ]], { +        -- +        "\0A" +        -- +    }) + +test:do_execsql_test( +    "func-22.33", +    [[ +        SELECT RTRIM(X'004100', X'0042'); +    ]], { +        -- +        "\0A" +        -- +    }) + +test:do_execsql_test( +    "func-22.34", +    [[ +        SELECT RTRIM(X'00004100420000', X'00'); +    ]], { +        -- +        "\0\0A\0B" +        -- +    }) +  -- This is to test the deprecated sqlite3_aggregate_count() API.  --  --test:do_test(