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

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

[Roman Khabibov]

Print error if object after serialization is the same.

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

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..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.'
+ | ...
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..d3c76ef0c
--- /dev/null
+++ b/test/app/gh-3228-serializer-look-for-recursion.test.lua
@@ -0,0 +1,8 @@
+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})
+setmetatable({}, {__serialize = function(a, b, c) return a, b, c end})
-- 
2.24.3 (Apple Git-128)

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

[Igor Munkin]

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

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

[roman]

Hi! Thanks for the review.

On 23.11.2020 23:28, Igor Munkin 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.

Done.

>> 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.

Added.

>> + | ...
> <snipped>
>
>> -- 
>> 2.24.3 (Apple Git-128)
>>
commit a40f5623b31e547cc0c273f2224484a591002e31
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 23fbdd4..d12f367 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 0000000..aaa3a4f
--- /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) return a ~= b 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 0000000..2f757af
--- /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) return a ~= b end})
+setmetatable(table, {__serialize = function(a) return a end})

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

[Roman Khabibov]

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})

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

[Sergey Ostanevich]

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})
> 

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

[Igor Munkin]

Sergos,

On 08.12.20, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> My biggest concern was how would you check the recursion appears. You

Well, I see the issue is reported for "identity" serializer (considering
documentation request and the error message), and such recursion can be
fixed the way Roma did. You're talking about a general case of infinite
recursions, but I believe there is no other approach to handle this
situation the way different from one Python does (I mentioned it in my
previous review[1]). By the way, other runtimes such as Perl starts
spamming with the corresponding warning after the recursion hits the
"soft" limit, but the result is the same: stack overflow. Thoughts?

> 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
> 

<snipped>

> 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018524.html

-- 
Best regards,
IM

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

[Roman Khabibov]

Hi!

> On Dec 8, 2020, at 22:25, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergos,
> 
> On 08.12.20, Sergey Ostanevich wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> 
>> My biggest concern was how would you check the recursion appears. You
> 
> Well, I see the issue is reported for "identity" serializer (considering
> documentation request and the error message), and such recursion can be
> fixed the way Roma did. You're talking about a general case of infinite
> recursions, but I believe there is no other approach to handle this
> situation the way different from one Python does (I mentioned it in my
> previous review[1]). By the way, other runtimes such as Perl starts
> spamming with the corresponding warning after the recursion hits the
> "soft" limit, but the result is the same: stack overflow. Thoughts?
Let’s just add ‘hard’ limit? Only it is not clear how to calculate its
value. Just empirically?

>> 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
>> 
> 
> <snipped>
> 
>> 
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018524.html
> 
> -- 
> Best regards,
> IM