Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: <imeevma@tarantool.org>
Cc: <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
Date: Thu, 2 Sep 2021 12:15:50 +0300	[thread overview]
Message-ID: <069e01d79fdb$1fac2ab0$5f048010$@tarantool.org> (raw)
In-Reply-To: <9beadd2600ef349c5bf5142716784526423b317c.1630572128.git.imeevma@gmail.com>

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



  reply	other threads:[~2021-09-02  9:16 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02  8:43 Mergen Imeev via Tarantool-patches
2021-09-02  9:15 ` Timur Safin via Tarantool-patches [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='069e01d79fdb$1fac2ab0$5f048010$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox