[Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
Mergen Imeev
imeevma at tarantool.org
Thu Sep 2 12:35:54 MSK 2021
Hi! Thank you for the review! My answer below.
On Thu, Sep 02, 2021 at 12:15:50PM +0300, Timur Safin wrote:
> You intentionally increase memory usage here (before we had a
> chance to release memory for empty literals, nowadays you leave
> 32-bytes hanging longer. And why 32, and not, say, 8 bytes -
> which is lowest allocation granularity).
I did this because we already have case when 32 bytes or more allocated. You
can see this in sqlVdbeMemGrow().
> I don't like it in a longer-term, but as a short term dirty
> work-around - that's ok. We will reshake
I agree that this should be rethought, but I disagree that this is dirty
work-around - it works according the rules of most cases, when allocated memory
is used. This is so because in most cases we use sqlVdbeMemGrow(), which
allocates no less than 32 bytes.
>
> LGTM
>
> Timur
>
>
> > -----Original Message-----
> > From: imeevma at tarantool.org <imeevma at tarantool.org>
> > Sent: Thursday, September 2, 2021 11:43 AM
> > To: tsafin at tarantool.org
> > Cc: tarantool-patches at dev.tarantool.org
> > Subject: [PATCH v1 1/1] 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
> > ---
> > https://github.com/tarantool/tarantool/issues/6157
> > https://github.com/tarantool/tarantool/issues/6399
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-6157-fix-
> > error-on-empty-str
> >
> > .../gh-6157-fix-error-on-copy-empty-str.md | 4 ++++
> > src/box/sql/func.c | 6 ++++--
> > src/box/sql/mem.c | 3 ++-
> > src/box/sql/vdbe.c | 3 ++-
> > src/box/sql/vdbeapi.c | 3 ++-
> > src/box/sql/vdbeaux.c | 8 +++++--
> > test/sql-tap/engine.cfg | 1 +
> > ...h-6157-unnecessary-free-on-string.test.lua | 21
> > +++++++++++++++++++
> > 8 files changed, 42 insertions(+), 7 deletions(-)
> > create mode 100644 changelogs/unreleased/gh-6157-fix-error-on-copy-
> > empty-str.md
> > create mode 100755 test/sql-tap/gh-6157-unnecessary-free-on-
> > string.test.lua
> >
> > 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 24de26548..48755a017 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 77df0e4cc..115940227 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()
> > --
> > 2.25.1
>
>
More information about the Tarantool-patches
mailing list