Re: [Tarantool-patches] [PATCH] serializer: check for recursive serialization

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch!

My biggest concern was how would you check the recursion appears. You just check if the result is equivalent
to the argument. To me it is not enough, obviously. I tried this on your branch and…


tarantool> setmetatable({},{__serialize = function(_) return {_} end})
Segmentation fault (core dumped)


Regards,
Sergos


> On 2 Dec 2020, at 03:53, Roman Khabibov <roman.habibov@tarantool.org> wrote:
> 
> Thanks for the LGTM.
> 
> SergOs, could you, please, look through the patch?
> 
>> On Nov 23, 2020, at 23:28, Igor Munkin <imun@tarantool.org> wrote:
>> 
>> Roma,
>> 
>> Thanks for the patch! This version looks much better than the previous one,
>> but I still have a couple of nits. Otherwise LGTM.
>> 
>> On 17.11.20, Roman Khabibov wrote:
>>> Print error if object after serialization is the same.
>> 
>> I believe we need a doc request to update __serialize description, since
>> its behaviour is restricted with the introduced constraint now.
>> 
>>> 
>>> Closes #3228
>>> ---
>>> 
>>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/serialize-check
>>> Issue: https://github.com/tarantool/tarantool/issues/3228
>>> 
>>> @ChangeLog:
>>> * Fix bug with bus error when __serialize function generates infinite recursion (gh-3228).
>>> 
>>> src/lua/utils.c                               |  5 +++++
>>> ...-3228-serializer-look-for-recursion.result | 19 +++++++++++++++++++
>>> ...228-serializer-look-for-recursion.test.lua |  8 ++++++++
>>> 3 files changed, 32 insertions(+)
>>> create mode 100644 test/app/gh-3228-serializer-look-for-recursion.result
>>> create mode 100644 test/app/gh-3228-serializer-look-for-recursion.test.lua
>>> 
>> 
>> <snipped>
>> 
>>> diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result
>>> new file mode 100644
>>> index 000000000..cd86ab06a
>>> --- /dev/null
>>> +++ b/test/app/gh-3228-serializer-look-for-recursion.result
>>> @@ -0,0 +1,19 @@
>>> +-- test-run result file version 2
>>> +test_run = require('test_run').new()
>>> + | ---
>>> + | ...
>>> +
>>> +--
>>> +-- gh-3228: Check the error message in the case of a __serialize
>>> +-- function generating infinite recursion.
>>> +--
>>> +setmetatable({}, {__serialize = function(a) return a end})
>>> + | ---
>>> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize
>>> + |     function. It can''t return the same value.'
>>> + | ...
>>> +setmetatable({}, {__serialize = function(a, b, c) return a, b, c end})
>>> + | ---
>>> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize
>>> + |     function. It can''t return the same value.'
>> 
>> Hm, AFAICS the custom serializer accepts a single argument (i.e. "self")
>> and a single return value is expected (considering the code you were
>> around to). Hence, the latter check is the same as the first one and
>> checks literally nothing. By the way, I guess it's worth to check that
>> __eq metamethod is ignored when the object itself is compared with its
>> "serialized" value. Just to be sure it won't be broken unintentionally
>> in future.
>> 
>>> + | ...
>> 
>> <snipped>
>> 
>>> -- 
>>> 2.24.3 (Apple Git-128)
>>> 
>> 
>> -- 
>> Best regards,
>> IM
> 
> commit 0eebee84ac425fc028f07920352ad2f9ec8be1e1 (HEAD -> romanhabibov/serialize-check, origin/romanhabibov/serialize-check)
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Thu Oct 8 18:22:24 2020 +0300
> 
>    serializer: check for recursive serialization
> 
>    Print error if object after serialization is the same.
> 
>    Closes #3228
> 
>    @TarantoolBot documnet
>    Title: __serialize parameter
>    If __serialize parameter is function, then this function
>    can't return the value passed to it. Such functions
>    generates recursions, so this is forbidden.
> 
>    Example:
>    ```
>    tarantool> setmetatable({},{__serialize = function(_) return _ end})
>    ---
>    - error: 'console: an exception occurred when formatting the output: Bad __serialize
>        function. It can''t return the same value.'
>    ...
>    ```
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 23fbdd4ad..d12f3675a 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -508,6 +508,11 @@ lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
> 			diag_set(LuajitError, lua_tostring(L, -1));
> 			return -1;
> 		}
> +		if (lua_rawequal(L, -2, -1) == 1) {
> +			diag_set(LuajitError, "Bad __serialize function. It "
> +				 "can't return the same value.");
> +			return -1;
> +		}
> 		if (luaL_tofield(L, cfg, NULL, -1, field) != 0)
> 			return -1;
> 		lua_replace(L, idx);
> diff --git a/test/app/gh-3228-serializer-look-for-recursion.result b/test/app/gh-3228-serializer-look-for-recursion.result
> new file mode 100644
> index 000000000..e55c2796b
> --- /dev/null
> +++ b/test/app/gh-3228-serializer-look-for-recursion.result
> @@ -0,0 +1,26 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +
> +--
> +-- gh-3228: Check the error message in the case of a __serialize
> +-- function generating infinite recursion.
> +--
> +setmetatable({}, {__serialize = function(a) return a end})
> + | ---
> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize
> + |     function. It can''t return the same value.'
> + | ...
> +
> +--
> +--Check that __eq metamethod is ignored.
> +--
> +local table = setmetatable({}, {__eq = function(a, b) error('__eq is called') end})
> + | ---
> + | ...
> +setmetatable(table, {__serialize = function(a) return a end})
> + | ---
> + | - error: 'console: an exception occurred when formatting the output: Bad __serialize
> + |     function. It can''t return the same value.'
> + | ...
> diff --git a/test/app/gh-3228-serializer-look-for-recursion.test.lua b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> new file mode 100644
> index 000000000..01268f026
> --- /dev/null
> +++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua
> @@ -0,0 +1,13 @@
> +test_run = require('test_run').new()
> +
> +--
> +-- gh-3228: Check the error message in the case of a __serialize
> +-- function generating infinite recursion.
> +--
> +setmetatable({}, {__serialize = function(a) return a end})
> +
> +--
> +--Check that __eq metamethod is ignored.
> +--
> +local table = setmetatable({}, {__eq = function(a, b) error('__eq is called') end})
> +setmetatable(table, {__serialize = function(a) return a end})
>