[tarantool-patches] Re: [PATCH v1 1/1] sql: fix double mem release on Lua function

[Nikita Pettik]

On 18 Sep 15:48, Kirill Shcherbatov wrote:
> An region's integrity assertion used to be triggered with
> recursive SQL request calling Lua function. This happened because
> of invalid usage of region_truncate routine: the preparatory
> region_used call used to be called in a wrong place, before
> function call itself that may allocate and release memory on
> region by it's own.
> 
> Closes #4504
> ---

We've discussed verbally real origins of this issue. I've also found
crashing query due to the same reason: inside OP_RowData region is
used to carry blob values without explicit tracking of its lifetime.
As a result, if box.space:select() is called between OP_RowData and
next access to the allocated on region blob, the last results in crash.
Investigating usages during query compilation, I come up with following
example:

WITH RECURSIVE w AS ( SELECT s1 FROM ts UNION ALL SELECT s1+1  FROM w WHERE s1 < 4) SELECT _c(s1) FROM w;

Substituting region with malloc fixes this problem.

> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4504-func-recursion-region
> Issue: https://github.com/tarantool/tarantool/issues/4504
> 
>  src/box/sql/vdbe.c          |  4 ++--
>  test/box/function1.result   | 35 +++++++++++++++++++++++++++++++++++
>  test/box/function1.test.lua | 20 ++++++++++++++++++++
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 8c4acdee2..9580d944e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1842,14 +1842,14 @@ case OP_Function: {
>  	struct Mem *argv = &aMem[pOp->p2];
>  	struct port args, ret;
>  
> -	struct region *region = &fiber()->gc;
> -	size_t region_svp = region_used(region);
>  	port_vdbemem_create(&args, (struct sql_value *)argv, argc);
>  	if (func_call(func, &args, &ret) != 0)
>  		goto abort_due_to_error;
>  
>  	pOut = vdbe_prepare_null_out(p, pOp->p3);
>  	uint32_t size;
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
>  	struct Mem *mem = (struct Mem *)port_get_vdbemem(&ret, &size);
>  	if (mem != NULL && size > 0)
>  		*pOut = mem[0];
> diff --git a/test/box/function1.result b/test/box/function1.result
> index a41ca4e3c..0838f2774 100644
> --- a/test/box/function1.result
> +++ b/test/box/function1.result
> @@ -1032,3 +1032,38 @@ box.func['test'].is_multikey == true
>  box.func['test']:drop()
>  ---
>  ...
> +--
> +-- gh-4504: Region integrity assertion with recursive sql request
> +--          involving Lua function.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +box.schema.func.create('_C', {
> +	language = 'LUA', returns = 'string',
> +	body = [[function (i)
> +			print(i)
> +			local m = box.space._space:select()
> +			return 0
> +		end
> +		]],
> +	param_list = {'integer'}, exports = {'LUA', 'SQL'},
> +	is_sandboxed = false, is_deterministic = true})
> +test_run:cmd("setopt delimiter ''");
> +---
> +...
> +box.execute([[WITH RECURSIVE x AS (SELECT 0 AS q, 1 UNION ALL SELECT q+1, _c(q) FROM x WHERE q < 1) SELECT * FROM x;]])
> +---
> +- metadata:
> +  - name: Q
> +    type: integer
> +  - name: '1'
> +    type: integer
> +  rows:
> +  - [0, 1]
> +  - [1, 0]
> +...
> +box.func._C:drop()
> +---
> +...
> diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua
> index e576cbb6f..b0b77fc06 100644
> --- a/test/box/function1.test.lua
> +++ b/test/box/function1.test.lua
> @@ -368,3 +368,23 @@ box.schema.func.drop("SUM")
>  box.schema.func.create('test', {body = "function(tuple) return tuple end", is_deterministic = true, opts = {is_multikey = true}})
>  box.func['test'].is_multikey == true
>  box.func['test']:drop()
> +
> +--
> +-- gh-4504: Region integrity assertion with recursive sql request
> +--          involving Lua function.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +box.schema.func.create('_C', {
> +	language = 'LUA', returns = 'string',
> +	body = [[function (i)
> +			print(i)
> +			local m = box.space._space:select()
> +			return 0
> +		end
> +		]],
> +	param_list = {'integer'}, exports = {'LUA', 'SQL'},
> +	is_sandboxed = false, is_deterministic = true})
> +test_run:cmd("setopt delimiter ''");
> +
> +box.execute([[WITH RECURSIVE x AS (SELECT 0 AS q, 1 UNION ALL SELECT q+1, _c(q) FROM x WHERE q < 1) SELECT * FROM x;]])
> +box.func._C:drop()
> -- 
> 2.23.0
> 
>