Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
@ 2021-09-02  8:43 Mergen Imeev via Tarantool-patches
  2021-09-02  9:15 ` Timur Safin via Tarantool-patches
  2021-09-02  9:17 ` Timur Safin via Tarantool-patches
  0 siblings, 2 replies; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-02  8:43 UTC (permalink / raw)
  To: tsafin; +Cc: tarantool-patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-09-02  8:43 [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy() Mergen Imeev via Tarantool-patches
@ 2021-09-02  9:15 ` Timur Safin via Tarantool-patches
  2021-09-02  9:35   ` Mergen Imeev via Tarantool-patches
  2021-09-02  9:17 ` Timur Safin via Tarantool-patches
  1 sibling, 1 reply; 12+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-09-02  9:15 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

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

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-09-02  8:43 [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy() Mergen Imeev via Tarantool-patches
  2021-09-02  9:15 ` Timur Safin via Tarantool-patches
@ 2021-09-02  9:17 ` Timur Safin via Tarantool-patches
  1 sibling, 0 replies; 12+ messages in thread
From: Timur Safin via Tarantool-patches @ 2021-09-02  9:17 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches


> 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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-09-02  9:15 ` Timur Safin via Tarantool-patches
@ 2021-09-02  9:35   ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-09-02  9:35 UTC (permalink / raw)
  To: Timur Safin; +Cc: tarantool-patches

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@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
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-31  8:55           ` Mergen Imeev via Tarantool-patches
@ 2021-09-01 21:34             ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-01 21:34 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

LGTM.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-30 21:32         ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-31  8:55           ` Mergen Imeev via Tarantool-patches
  2021-09-01 21:34             ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-31  8:55 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: 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 <imeevma@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()

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-30  5:57       ` Mergen Imeev via Tarantool-patches
@ 2021-08-30 21:32         ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-31  8:55           ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-30 21:32 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

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.

>>> 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?

> 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.

	t = {}
	for i = 1, 129 do table.insert(t, '') end

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-27 21:44     ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-30  5:57       ` Mergen Imeev via Tarantool-patches
  2021-08-30 21:32         ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-30  5:57 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

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.

> > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> > index 2d7800b17..beb8cee04 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) {
> > +				sqlDbFree(sql_get(), pRet);
> 
> 2. Shouldn't you use sqlValueFree()?
> 
True, fixed.

> > +				return NULL;
> > +			}
> >  			return pRet;
> >  		}
> >  	}
> > 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.

> > +--
> > +local s = "NULLIF(SUBSTR('123', 1, 0), NULL)"
> > +for i = 1, 5 do s = s..', '..s end
> > +local res = box.execute("SELECT "..s).rows[1]
> > +
> > +test:plan(1)
> > +test:is(#res, 32, 'wrong error in mem_copy() was set')
> > +os.exit(test:check() and 0 or 1)


Diff:

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index beb8cee04..8148ed8b0 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2321,7 +2321,7 @@ sqlVdbeGetBoundValue(struct Vdbe *v, int iVar)
 			if (pRet == NULL)
 				return NULL;
 			if (mem_copy(pRet, pMem) != 0) {
-				sqlDbFree(sql_get(), pRet);
+				sqlValueFree(pRet);
 				return NULL;
 			}
 			return pRet;
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 e0c09a325..e2d25d7d4 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
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
-local tap = require('tap')
-local test = tap.test('test wrong error in mem_copy()')
+local test = require("sqltester")
+test:plan(1)
 
 --
 -- Make sure there is no assert due to an incorrectly set error in mem_copy().
@@ -10,9 +10,20 @@ local test = tap.test('test wrong error in mem_copy()')
 -- STATIC and no memory will be allocated.
 --
 local s = "NULLIF(SUBSTR('123', 1, 0), NULL)"
-for i = 1, 5 do s = s..', '..s end
-local res = box.execute("SELECT "..s).rows[1]
+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:plan(1)
-test:is(#res, 32, 'wrong error in mem_copy() was set')
-os.exit(test:check() and 0 or 1)
+test:do_execsql_test(
+    "gh-6157", s, {
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", ""
+    })
+
+test:finish_test()


New patch:

commit a27b5651b67b54ddf9d403847263235162fbf489
Author: Mergen Imeev <imeevma@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..e5f747414
--- /dev/null
+++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
@@ -0,0 +1,5 @@
+## bugfix/sql
+
+* Now, when copying an empty string, an error will not be set
+  unnecessarily (gh-6157).
+
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..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, {
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
+        "", "", ""
+    })
+
+test:finish_test()

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-27 15:22   ` Mergen Imeev via Tarantool-patches
@ 2021-08-27 21:44     ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-30  5:57       ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-27 21:44 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

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?

> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 2d7800b17..beb8cee04 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) {
> +				sqlDbFree(sql_get(), pRet);

2. Shouldn't you use sqlValueFree()?

> +				return NULL;
> +			}
>  			return pRet;
>  		}
>  	}
> 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.

> +--
> +local s = "NULLIF(SUBSTR('123', 1, 0), NULL)"
> +for i = 1, 5 do s = s..', '..s end
> +local res = box.execute("SELECT "..s).rows[1]
> +
> +test:plan(1)
> +test:is(#res, 32, 'wrong error in mem_copy() was set')
> +os.exit(test:check() and 0 or 1)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-26 20:21 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-27 15:22   ` Mergen Imeev via Tarantool-patches
  2021-08-27 21:44     ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-27 15:22 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Thank you for the review! My answers and new patch below. I didn't include diff
since I reworked most psrt of the patch.

On Thu, Aug 26, 2021 at 10:21:01PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
> > diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.c b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
> > new file mode 100644
> > index 000000000..ce928d494
> > --- /dev/null
> > +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
> > @@ -0,0 +1,10 @@
> > +#include "msgpuck.h"
> > +#include "module.h"
> > +
> > +int
> > +f(box_function_ctx_t* ctx, const char* args, const char* args_end)
> 
> 1. '*' must be right before variable name, not after the types.
> 
Dropped this funcion.

> > +{
> > +	char res[16];
> > +	char *end = mp_encode_str(res, "stub", strlen("stub"));
> > +	return box_return_mp(ctx, res, end);
> > +}
> 
> 2. Why do you need a C function? Can't you reproduce it with Lua?
> 
Dropped.

> > 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..326570aea
> > --- /dev/null
> > +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> > @@ -0,0 +1,38 @@
> > +#!/usr/bin/env tarantool
> > +local build_path = os.getenv("BUILDDIR")
> > +package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath
> > +
> > +local test = require("sqltester")
> > +test:plan(1)
> > +
> > +box.schema.func.create("gh-6157.f", {
> > +    language = "C",
> > +    param_list = {"string"},
> > +    returns = "string",
> > +    exports = {"SQL"}
> > +})
> > +
> > +box.execute([[CREATE TABLE ts(s STRING PRIMARY KEY);]])
> > +box.execute([[INSERT INTO ts VALUES ('');]])
> > +box.execute([[CREATE TABLE ti(i INT PRIMARY KEY);]])
> > +for i = 1, 100 do
> > +    box.execute([[INSERT INTO ti VALUES(]]..i..[[);]])
> 
> 3. Why do you need a space to reproduce this bug? It does not seem related.
As I understood, there is two bugs: first - seems like lookaside is leaking, but
not sure if this is a bug. Created a new issue - #6401. The second is the one
fixed in this patch. I reworked this test and now it is a lot simpler.

Also, I added checks for the result of mem_copy() since before we couldn't be
sure where exactly these errors appear, since only part of mem_copy() was
executed, but error was set improperly.

New patch:


commit c7e823516c5fdf91ff9b04fc3a0642e3ced9ad3e
Author: Mergen Imeev <imeevma@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

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..e5f747414
--- /dev/null
+++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
@@ -0,0 +1,5 @@
+## bugfix/sql
+
+* Now, when copying an empty string, an error will not be set
+  unnecessarily (gh-6157).
+
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..beb8cee04 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) {
+				sqlDbFree(sql_get(), 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..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.
+--
+local s = "NULLIF(SUBSTR('123', 1, 0), NULL)"
+for i = 1, 5 do s = s..', '..s end
+local res = box.execute("SELECT "..s).rows[1]
+
+test:plan(1)
+test:is(#res, 32, 'wrong error in mem_copy() was set')
+os.exit(test:check() and 0 or 1)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
  2021-08-26 11:09 Mergen Imeev via Tarantool-patches
@ 2021-08-26 20:21 ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-27 15:22   ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-26 20:21 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.c b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
> new file mode 100644
> index 000000000..ce928d494
> --- /dev/null
> +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
> @@ -0,0 +1,10 @@
> +#include "msgpuck.h"
> +#include "module.h"
> +
> +int
> +f(box_function_ctx_t* ctx, const char* args, const char* args_end)

1. '*' must be right before variable name, not after the types.

> +{
> +	char res[16];
> +	char *end = mp_encode_str(res, "stub", strlen("stub"));
> +	return box_return_mp(ctx, res, end);
> +}

2. Why do you need a C function? Can't you reproduce it with 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..326570aea
> --- /dev/null
> +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> @@ -0,0 +1,38 @@
> +#!/usr/bin/env tarantool
> +local build_path = os.getenv("BUILDDIR")
> +package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath
> +
> +local test = require("sqltester")
> +test:plan(1)
> +
> +box.schema.func.create("gh-6157.f", {
> +    language = "C",
> +    param_list = {"string"},
> +    returns = "string",
> +    exports = {"SQL"}
> +})
> +
> +box.execute([[CREATE TABLE ts(s STRING PRIMARY KEY);]])
> +box.execute([[INSERT INTO ts VALUES ('');]])
> +box.execute([[CREATE TABLE ti(i INT PRIMARY KEY);]])
> +for i = 1, 100 do
> +    box.execute([[INSERT INTO ti VALUES(]]..i..[[);]])

3. Why do you need a space to reproduce this bug? It does not seem related.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
@ 2021-08-26 11:09 Mergen Imeev via Tarantool-patches
  2021-08-26 20:21 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 12+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-08-26 11:09 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

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
---
https://github.com/tarantool/tarantool/issues/6157
https://github.com/tarantool/tarantool/tree/imeevma/gh-6157-fix-error-on-empty-str

 .../gh-6157-fix-error-on-copy-empty-str.md    |  5 +++
 src/box/sql/mem.c                             |  3 +-
 test/sql-tap/CMakeLists.txt                   |  1 +
 test/sql-tap/engine.cfg                       |  1 +
 .../gh-6157-unnecessary-free-on-string.c      | 10 +++++
 ...h-6157-unnecessary-free-on-string.test.lua | 38 +++++++++++++++++++
 6 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
 create mode 100644 test/sql-tap/gh-6157-unnecessary-free-on-string.c
 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..e5f747414
--- /dev/null
+++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
@@ -0,0 +1,5 @@
+## bugfix/sql
+
+* Now, when copying an empty string, an error will not be set
+  unnecessarily (gh-6157).
+
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 0aca76112..c91fc8396 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/test/sql-tap/CMakeLists.txt b/test/sql-tap/CMakeLists.txt
index 87f23b2f7..2e215032b 100644
--- a/test/sql-tap/CMakeLists.txt
+++ b/test/sql-tap/CMakeLists.txt
@@ -3,3 +3,4 @@ build_module(gh-5938-wrong-string-length gh-5938-wrong-string-length.c)
 build_module(gh-6024-funcs-return-bin gh-6024-funcs-return-bin.c)
 build_module(sql_uuid sql_uuid.c)
 build_module(decimal decimal.c)
+build_module(gh-6157 gh-6157-unnecessary-free-on-string.c)
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 35754f769..f6f7752af 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": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.c b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
new file mode 100644
index 000000000..ce928d494
--- /dev/null
+++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.c
@@ -0,0 +1,10 @@
+#include "msgpuck.h"
+#include "module.h"
+
+int
+f(box_function_ctx_t* ctx, const char* args, const char* args_end)
+{
+	char res[16];
+	char *end = mp_encode_str(res, "stub", strlen("stub"));
+	return box_return_mp(ctx, res, end);
+}
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..326570aea
--- /dev/null
+++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
@@ -0,0 +1,38 @@
+#!/usr/bin/env tarantool
+local build_path = os.getenv("BUILDDIR")
+package.cpath = build_path..'/test/sql-tap/?.so;'..build_path..'/test/sql-tap/?.dylib;'..package.cpath
+
+local test = require("sqltester")
+test:plan(1)
+
+box.schema.func.create("gh-6157.f", {
+    language = "C",
+    param_list = {"string"},
+    returns = "string",
+    exports = {"SQL"}
+})
+
+box.execute([[CREATE TABLE ts(s STRING PRIMARY KEY);]])
+box.execute([[INSERT INTO ts VALUES ('');]])
+box.execute([[CREATE TABLE ti(i INT PRIMARY KEY);]])
+for i = 1, 100 do
+    box.execute([[INSERT INTO ti VALUES(]]..i..[[);]])
+end
+
+test:do_execsql_test(
+    "gh-6157",
+    [[
+        SELECT COUNT("gh-6157.f"('')), (SELECT s FROM ts WHERE s = '') FROM ti;
+        SELECT COUNT("gh-6157.f"('')), (SELECT s FROM ts WHERE s = '') FROM ti;
+        SELECT COUNT("gh-6157.f"('')), (SELECT s FROM ts WHERE s = '') FROM ti;
+        SELECT COUNT("gh-6157.f"('')), (SELECT s FROM ts WHERE s = '') FROM ti;
+        SELECT COUNT("gh-6157.f"('')), (SELECT s FROM ts WHERE s = '') FROM ti;
+    ]], {
+        100, ""
+    })
+
+box.space._func.index['name']:delete("gh-6157.f")
+box.execute([[DROP TABLE ts;]])
+box.execute([[DROP TABLE ti;]])
+
+test:finish_test()
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-09-02  9:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02  8:43 [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy() Mergen Imeev via Tarantool-patches
2021-09-02  9:15 ` Timur Safin via Tarantool-patches
2021-09-02  9:35   ` Mergen Imeev via Tarantool-patches
2021-09-02  9:17 ` Timur Safin via Tarantool-patches
  -- strict thread matches above, loose matches on Subject: below --
2021-08-26 11:09 Mergen Imeev via Tarantool-patches
2021-08-26 20:21 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-27 15:22   ` Mergen Imeev via Tarantool-patches
2021-08-27 21:44     ` Vladislav Shpilevoy via Tarantool-patches
2021-08-30  5:57       ` Mergen Imeev via Tarantool-patches
2021-08-30 21:32         ` Vladislav Shpilevoy via Tarantool-patches
2021-08-31  8:55           ` Mergen Imeev via Tarantool-patches
2021-09-01 21:34             ` Vladislav Shpilevoy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git