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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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)