[Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
Mergen Imeev
imeevma at tarantool.org
Tue Aug 31 11:55:01 MSK 2021
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 <imeevma at gmail.com>
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()
More information about the Tarantool-patches
mailing list