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

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

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