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

[Roman Khabibov <roman.habibov@tarantool.org>]

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