Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

> From: Timur Safin <tsafin@tarantool.org>
> Subject: RE: [PATCH v1 1/1] sql: fix error on copy empty string in
> mem_copy()
> 
> 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 don't like it in a longer-term, but as a short term dirty
> work-around - that's ok. We will reshake

(Incomplete wording) We will reshake all allocation related 
functionality in SQL subsystem very soon.



> 
> LGTM
> 
> Timur
> 
> 
> > -----Original Message-----
> > From: imeevma@tarantool.org <imeevma@tarantool.org>
> > Sent: Thursday, September 2, 2021 11:43 AM
> > To: tsafin@tarantool.org
> > Cc: tarantool-patches@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