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!

On 30.08.2021 07:57, Mergen Imeev wrote:
> Thank you for the review! My answers, diff and new patch below.
> 
> On Fri, Aug 27, 2021 at 11:44:23PM +0200, Vladislav Shpilevoy wrote:
>> 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?
>>
> True, thank you. Fixed.

You didn't add it to the changelog. You can use the same file for both
tickets I think.

>>> 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.
>>
> Fixed. Increased number of expressions to 129 and reworked the test-file a bit.

If you say it depends on 128, why did the test fail without the patch even
before you made it use 129 cells?

> 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..e2d25d7d4
> --- /dev/null
> +++ b/test/sql-tap/gh-6157-unnecessary-free-on-string.test.lua
> @@ -0,0 +1,29 @@
> +#!/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 s = "NULLIF(SUBSTR('123', 1, 0), NULL)"
> +for i = 1, 7 do s = s..', '..s end
> +s = "SELECT "..s..', '.."NULLIF(SUBSTR('123', 1, 0), NULL)"
> +-- The "s" variable contains 129 expressions.
> +
> +test:do_execsql_test(
> +    "gh-6157", s, {
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> +        "", "", ""

Lua has loops. You can use them to avoid copy-paste.

	t = {}
	for i = 1, 129 do table.insert(t, '') end