[tarantool-patches] Re: [PATCH v1 2/3] sql: fix sql_vdbe_mem_alloc_region result memory

[n.pettik]

> The function sql_vdbe_mem_alloc_region that constructing the
> value of Vdbe Mem object used to change only flags responsible
> for it's type.
> It is also required to grind old flags, as their combination may
> be invalid.
> In a typical Vdbe scenario, OP_MakeRecord and OP_RowData make
> memory release with sqlite3VdbeMemRelease and allocation
> on region with sql_vdbe_mem_alloc_region call. An integrity
> assert based on sqlite3VdbeCheckMemInvariants would fire here
> because of contradictory combination of flags
> MEM_Static | (MEM_Blob | MEM_Ephem).
> 
> Needed for #3850
> ---
> src/box/sql/vdbeaux.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index fc805e3aa..d477662a4 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -3231,7 +3231,8 @@ sql_vdbe_mem_alloc_region(Mem *vdbe_mem, uint32_t size)
> 	vdbe_mem->z = region_alloc(&fiber()->gc, size);
> 	if (vdbe_mem->z == NULL)
> 		return SQLITE_NOMEM;
> -	MemSetTypeFlag(vdbe_mem, MEM_Blob | MEM_Ephem);
> +	vdbe_mem->flags = MEM_Ephem | MEM_Blob;
> +	assert(sqlite3VdbeCheckMemInvariants(vdbe_mem));
> 	return SQLITE_OK;
> }


I’ve changed your commit message a bit:

Function sql_vdbe_mem_alloc_region() that constructs the value of Vdbe
Mem object used to change only type related flags.  However, it is also
required to erase other flags (for instance flags related to allocation
policy: static, dynamic etc), since their combination may be invalid.
In a typical Vdbe scenario, OP_MakeRecord and OP_RowData release memory
with sqlite3VdbeMemRelease() and allocate on region with
sql_vdbe_mem_alloc_region(). An integrity assert based on
sqlite3VdbeCheckMemInvariants() would fire here due to incompatible
combination of flags: MEM_Static | (MEM_Blob | MEM_Ephem).

Read it and in case it looks OK to you, please apply.

LGTM.