[Tarantool-patches] [PATCH v1 1/1] sql: fix error on copy empty string in mem_copy()
Timur Safin
tsafin at tarantool.org
Thu Sep 2 12:15:50 MSK 2021
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 at tarantool.org <imeevma at tarantool.org>
> Sent: Thursday, September 2, 2021 11:43 AM
> To: tsafin at tarantool.org
> Cc: tarantool-patches at dev.tarantool.org
> Subject: [PATCH v1 1/1] sql: fix error on copy empty string in
> mem_copy()
>
> This patch fixes the problem with copying an empty string in
> mem_copy().
> Previously, because the string length was 0, an error was thrown, but
> the diag was not set, which could lead to an error due to an empty
> diag
> or to a double free.
>
> Closes #6157
> Closes #6399
> ---
> https://github.com/tarantool/tarantool/issues/6157
> https://github.com/tarantool/tarantool/issues/6399
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6157-fix-
> error-on-empty-str
>
> .../gh-6157-fix-error-on-copy-empty-str.md | 4 ++++
> src/box/sql/func.c | 6 ++++--
> src/box/sql/mem.c | 3 ++-
> src/box/sql/vdbe.c | 3 ++-
> src/box/sql/vdbeapi.c | 3 ++-
> src/box/sql/vdbeaux.c | 8 +++++--
> test/sql-tap/engine.cfg | 1 +
> ...h-6157-unnecessary-free-on-string.test.lua | 21
> +++++++++++++++++++
> 8 files changed, 42 insertions(+), 7 deletions(-)
> create mode 100644 changelogs/unreleased/gh-6157-fix-error-on-copy-
> empty-str.md
> create mode 100755 test/sql-tap/gh-6157-unnecessary-free-on-
> string.test.lua
>
> diff --git a/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-
> str.md b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
> new file mode 100644
> index 000000000..398b6e31d
> --- /dev/null
> +++ b/changelogs/unreleased/gh-6157-fix-error-on-copy-empty-str.md
> @@ -0,0 +1,4 @@
> +## bugfix/sql
> +
> +* Now, when copying an empty string, an error will not be set
> + unnecessarily (gh-6157, gh-6399).
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index c063552d6..9009f9e4f 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1771,13 +1771,15 @@ minmaxStep(sql_context * context, int
> NotUsed, sql_value ** argv)
> bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
> int cmp = mem_cmp_scalar(pBest, pArg, pColl);
> if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
> - mem_copy(pBest, pArg);
> + if (mem_copy(pBest, pArg) != 0)
> + context->is_aborted = true;
> } else {
> sqlSkipAccumulatorLoad(context);
> }
> } else {
> pBest->db = sql_context_db_handle(context);
> - mem_copy(pBest, pArg);
> + if (mem_copy(pBest, pArg) != 0)
> + context->is_aborted = true;
> }
> }
>
> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 24de26548..48755a017 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -1913,7 +1913,8 @@ mem_copy(struct Mem *to, const struct Mem
> *from)
> assert((to->flags & MEM_Zero) == 0 || to->type == MEM_TYPE_BIN);
> if ((to->flags & MEM_Zero) != 0)
> return sqlVdbeMemExpandBlob(to);
> - to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
> + to->zMalloc = sqlDbRealloc(to->db, to->zMalloc, MAX(32, to->n));
> + assert(to->zMalloc != NULL || sql_get()->mallocFailed != 0);
> if (to->zMalloc == NULL)
> return -1;
> to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 7f86fa7b3..44533fb3e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -978,7 +978,8 @@ case OP_Copy: {
> pOut = &aMem[pOp->p2];
> assert(pOut!=pIn1);
> while( 1) {
> - mem_copy(pOut, pIn1);
> + if (mem_copy(pOut, pIn1) != 0)
> + goto abort_due_to_error;
> REGISTER_TRACE(p, pOp->p2+pOp->p3-n, pOut);
> if ((n--)==0) break;
> pOut++;
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 77df0e4cc..115940227 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -232,7 +232,8 @@ sql_result_text64(sql_context * pCtx,
> void
> sql_result_value(sql_context * pCtx, sql_value * pValue)
> {
> - mem_copy(pCtx->pOut, pValue);
> + if (mem_copy(pCtx->pOut, pValue) != 0)
> + pCtx->is_aborted = true;
> }
>
> void
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 2d7800b17..8148ed8b0 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2318,8 +2318,12 @@ sqlVdbeGetBoundValue(struct Vdbe *v, int iVar)
> Mem *pMem = &v->aVar[iVar - 1];
> if (!mem_is_null(pMem)) {
> sql_value *pRet = sqlValueNew(v->db);
> - if (pRet != NULL)
> - mem_copy(pRet, pMem);
> + if (pRet == NULL)
> + return NULL;
> + if (mem_copy(pRet, pMem) != 0) {
> + sqlValueFree(pRet);
> + return NULL;
> + }
> return pRet;
> }
> }
> diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
> index 587adbed9..c35d1dced 100644
> --- a/test/sql-tap/engine.cfg
> +++ b/test/sql-tap/engine.cfg
> @@ -35,6 +35,7 @@
> "built-in-functions.test.lua": {
> "memtx": {"engine": "memtx"}
> },
> + "gh-6157-unnecessary-free-on-string.test.lua": {},
> "gh-4077-iproto-execute-no-bind.test.lua": {},
> "gh-6375-assert-on-unsupported-ext.test.lua": {},
> "*": {
> diff --git a/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> new file mode 100755
> index 000000000..ebe69906a
> --- /dev/null
> +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> @@ -0,0 +1,21 @@
> +#!/usr/bin/env tarantool
> +local test = require("sqltester")
> +test:plan(1)
> +
> +--
> +-- Make sure there is no assert due to an incorrectly set error in
> mem_copy().
> +-- How this test works: We have 128 mempool cells in SQL
> ("lookaside"), and
> +-- until those 128 cells are filled in, the error cannot be
> reproduced. Also, we
> +-- have to get '' from somewhere because if we just enter it, it
> will be of type
> +-- STATIC and no memory will be allocated.
> +--
> +local query = "NULLIF(SUBSTR('123', 1, 0), NULL)"
> +for _ = 1, 7 do query = query..', '..query end
> +query = "SELECT "..query..", NULLIF(SUBSTR('123', 1, 0), NULL);"
> +-- The "query" variable contains 129 expressions.
> +local result = {}
> +for _ = 1, 129 do table.insert(result, '') end
> +
> +test:do_execsql_test("gh-6157", query, result)
> +
> +test:finish_test()
> --
> 2.25.1
More information about the Tarantool-patches
mailing list