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 D59896F3FD; Tue, 31 Aug 2021 11:55:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D59896F3FD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1630400105; bh=SdEEg/lIrOLvRGO9uSb/22SdImGHdvdjc6s+1tQ/5yU=; 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=snQ6dkpPOBKObTNttilGM5uMXOmBPEnHNln0pD5G7if+FFWa18Amib9e0S/1Yetnp wR0V/zsv5cNZlsvmLZWYyUdH4YTaKN6wVT0Q8NpxpT5AfusKO4tOVVXaOx/r9AkMhv eKJ2pr0Xoksnprx4Lz5gDAiMiElxlmNhW3NWCid0= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 C9F1F6F3F7 for ; Tue, 31 Aug 2021 11:55:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C9F1F6F3F7 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mKzXq-0006JW-V2; Tue, 31 Aug 2021 11:55:03 +0300 Date: Tue, 31 Aug 2021 11:55:01 +0300 To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Message-ID: <20210831085501.GA30258@tarantool.org> References: <7c4620245d61624883115541490cd94d10626c00.1629976113.git.imeevma@gmail.com> <40a89d61-e252-731c-d419-6c162eecefb4@tarantool.org> <20210827152223.GA435770@tarantool.org> <491db4a6-2fa4-8346-c22f-0924630f7e55@tarantool.org> <20210830055706.GA63349@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F53808504048B3AC60AF4A2E0042B8AA3578521001650A15A7AA06815FE880A43FB49282F1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE795530B80AF2ADB7BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377A9AC615CFEE341F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88C2013B98D11230E51F6BCAC78C0AA5B117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5981141C73B371FCA6DDD199850F87C07807B1246387A74E9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34753B45383CEFF2046AAE1DF3A71668256A259438712EE2B4E190B38360DE76EF1D7DACF7489F99741D7E09C32AA3244CCF1B2ADCEBD406FD0BBD5DC84759521797FE24653F78E668729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKvNUqyKacB0JV4qPKULrzA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DE61FEAB2DD232DE24AC2F3748BBE22EA83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy() 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, diff and new patch below. On Mon, Aug 30, 2021 at 11:32:15PM +0200, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > On 30.08.2021 07:57, Mergen Imeev wrote: > > Thank you for the review! My answers, diff and new patch below. > > > > On Fri, Aug 27, 2021 at 11:44:23PM +0200, Vladislav Shpilevoy wrote: > >> Thanks for the fixes! > >> > >> See 3 comments below. > >> > >>> sql: fix error on copy empty string in mem_copy() > >>> > >>> This patch fixes the problem with copying an empty string in mem_copy(). > >>> Previously, because the string length was 0, an error was thrown, but > >>> the diag was not set, which could lead to an error due to an empty diag > >>> or to a double free. > >>> > >>> Closes #6157 > >> > >> 1. You also need to add closes 6399, don't you? > >> > > True, thank you. Fixed. > > You didn't add it to the changelog. You can use the same file for both > tickets I think. > Fixed. > >>> diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua > >>> new file mode 100755 > >>> index 000000000..e0c09a325 > >>> --- /dev/null > >>> +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua > >>> @@ -0,0 +1,18 @@ > >>> +#!/usr/bin/env tarantool > >>> +local tap = require('tap') > >>> +local test = tap.test('test wrong error in mem_copy()') > >>> + > >>> +-- > >>> +-- Make sure there is no assert due to an incorrectly set error in mem_copy(). > >>> +-- How this test works: We have 128 mempool cells in SQL ("lookaside"), and > >>> +-- until those 128 cells are filled in, the error cannot be reproduced. Also, we > >>> +-- have to get '' from somewhere because if we just enter it, it will be of type > >>> +-- STATIC and no memory will be allocated. > >> > >> 3. You mention 128 cells, but I don't see how 128 or something close is used > >> in this test. > >> > > Fixed. Increased number of expressions to 129 and reworked the test-file a bit. > > If you say it depends on 128, why did the test fail without the patch even > before you made it use 129 cells? > This is because lookaside is used in many places in the SQL code. I think there are two cells used for each one of 129 expressions, but I'm not sure about that. There is quite a lot of mess currently in the memory allocation system in SQL, and it is quite difficult for me to determine the number of cells that will be used. However, 129 expressions will use at least 128 cells (since there are only 128 cells). We have an issue about rewriting the memory allocation system in SQL (#1544), but this is a pretty big issue, and Timur is very eager to be the one to fix it. I don't know when he want to fix it. Still, I plan to do some refactoring in issue #6401 in the next few weeks. > > diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua > > new file mode 100755 > > index 000000000..e2d25d7d4 > > --- /dev/null > > +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua > > @@ -0,0 +1,29 @@ > > +#!/usr/bin/env tarantool > > +local test = require("sqltester") > > +test:plan(1) > > + > > +-- > > +-- Make sure there is no assert due to an incorrectly set error in mem_copy(). > > +-- How this test works: We have 128 mempool cells in SQL ("lookaside"), and > > +-- until those 128 cells are filled in, the error cannot be reproduced. Also, we > > +-- have to get '' from somewhere because if we just enter it, it will be of type > > +-- STATIC and no memory will be allocated. > > +-- > > +local s = "NULLIF(SUBSTR('123', 1, 0), NULL)" > > +for i = 1, 7 do s = s..', '..s end > > +s = "SELECT "..s..', '.."NULLIF(SUBSTR('123', 1, 0), NULL)" > > +-- The "s" variable contains 129 expressions. > > + > > +test:do_execsql_test( > > + "gh-6157", s, { > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", > > + "", "", "" > > Lua has loops. You can use them to avoid copy-paste. > Thanks, fixed. I actually thoughts that this way will be easier to understand, but after applying your suggestion and renaming variables it looks a lot better. > t = {} > for i = 1, 129 do table.insert(t, '') end Diff: diff --git a/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md index e5f747414..398b6e31d 100644 --- a/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md +++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md @@ -1,5 +1,4 @@ ## bugfix/sql * Now, when copying an empty string, an error will not be set - unnecessarily (gh-6157). - + unnecessarily (gh-6157, gh-6399). diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua index e2d25d7d4..ebe69906a 100755 --- a/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua @@ -9,21 +9,13 @@ test:plan(1) -- have to get '' from somewhere because if we just enter it, it will be of type -- STATIC and no memory will be allocated. -- -local s = "NULLIF(SUBSTR('123', 1, 0), NULL)" -for i = 1, 7 do s = s..', '..s end -s = "SELECT "..s..', '.."NULLIF(SUBSTR('123', 1, 0), NULL)" --- The "s" variable contains 129 expressions. +local query = "NULLIF(SUBSTR('123', 1, 0), NULL)" +for _ = 1, 7 do query = query..', '..query end +query = "SELECT "..query..", NULLIF(SUBSTR('123', 1, 0), NULL);" +-- The "query" variable contains 129 expressions. +local result = {} +for _ = 1, 129 do table.insert(result, '') end -test:do_execsql_test( - "gh-6157", s, { - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "" - }) +test:do_execsql_test("gh-6157", query, result) test:finish_test() New patch: commit cb8ce291369ed7f3ca2f6adc5088a67a539b40e7 Author: Mergen Imeev Date: Mon Aug 23 11:54:09 2021 +0300 sql: fix error on copy empty string in mem_copy() This patch fixes the problem with copying an empty string in mem_copy(). Previously, because the string length was 0, an error was thrown, but the diag was not set, which could lead to an error due to an empty diag or to a double free. Closes #6157 Closes #6399 diff --git a/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md new file mode 100644 index 000000000..398b6e31d --- /dev/null +++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md @@ -0,0 +1,4 @@ +## bugfix/sql + +* Now, when copying an empty string, an error will not be set + unnecessarily (gh-6157, gh-6399). diff --git a/src/box/sql/func.c b/src/box/sql/func.c index c063552d6..9009f9e4f 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -1771,13 +1771,15 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) bool is_max = (func->flags & SQL_FUNC_MAX) != 0; int cmp = mem_cmp_scalar(pBest, pArg, pColl); if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { - mem_copy(pBest, pArg); + if (mem_copy(pBest, pArg) != 0) + context->is_aborted = true; } else { sqlSkipAccumulatorLoad(context); } } else { pBest->db = sql_context_db_handle(context); - mem_copy(pBest, pArg); + if (mem_copy(pBest, pArg) != 0) + context->is_aborted = true; } } diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 4c40f15dc..819d9b094 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -1913,7 +1913,8 @@ mem_copy(struct Mem *to, const struct Mem *from) assert((to->flags & MEM_Zero) == 0 || to->type == MEM_TYPE_BIN); if ((to->flags & MEM_Zero) != 0) return sqlVdbeMemExpandBlob(to); - to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n); + to->zMalloc = sqlDbRealloc(to->db, to->zMalloc, MAX(32, to->n)); + assert(to->zMalloc != NULL || sql_get()->mallocFailed != 0); if (to->zMalloc == NULL) return -1; to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7f86fa7b3..44533fb3e 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -978,7 +978,8 @@ case OP_Copy: { pOut = &aMem[pOp->p2]; assert(pOut!=pIn1); while( 1) { - mem_copy(pOut, pIn1); + if (mem_copy(pOut, pIn1) != 0) + goto abort_due_to_error; REGISTER_TRACE(p, pOp->p2+pOp->p3-n, pOut); if ((n--)==0) break; pOut++; diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 8031ee0dc..e44d73a16 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -232,7 +232,8 @@ sql_result_text64(sql_context * pCtx, void sql_result_value(sql_context * pCtx, sql_value * pValue) { - mem_copy(pCtx->pOut, pValue); + if (mem_copy(pCtx->pOut, pValue) != 0) + pCtx->is_aborted = true; } void diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 2d7800b17..8148ed8b0 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -2318,8 +2318,12 @@ sqlVdbeGetBoundValue(struct Vdbe *v, int iVar) Mem *pMem = &v->aVar[iVar - 1]; if (!mem_is_null(pMem)) { sql_value *pRet = sqlValueNew(v->db); - if (pRet != NULL) - mem_copy(pRet, pMem); + if (pRet == NULL) + return NULL; + if (mem_copy(pRet, pMem) != 0) { + sqlValueFree(pRet); + return NULL; + } return pRet; } } diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg index 587adbed9..c35d1dced 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-6157-unnecessary-free-on-string.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-6157-unnecessary-free-on-string.test.lua b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua new file mode 100755 index 000000000..ebe69906a --- /dev/null +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua @@ -0,0 +1,21 @@ +#!/usr/bin/env tarantool +local test = require("sqltester") +test:plan(1) + +-- +-- Make sure there is no assert due to an incorrectly set error in mem_copy(). +-- How this test works: We have 128 mempool cells in SQL ("lookaside"), and +-- until those 128 cells are filled in, the error cannot be reproduced. Also, we +-- have to get '' from somewhere because if we just enter it, it will be of type +-- STATIC and no memory will be allocated. +-- +local query = "NULLIF(SUBSTR('123', 1, 0), NULL)" +for _ = 1, 7 do query = query..', '..query end +query = "SELECT "..query..", NULLIF(SUBSTR('123', 1, 0), NULL);" +-- The "query" variable contains 129 expressions. +local result = {} +for _ = 1, 129 do table.insert(result, '') end + +test:do_execsql_test("gh-6157", query, result) + +test:finish_test()