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

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

Hi! Thanks for the review.

> On Sep 16, 2020, at 10:29, Igor Munkin <imun@tarantool.org> wrote:
> 
> Roma,
> 
> On 14.09.20, Roman Khabibov wrote:
>> Hi, Cyrill and Igor!
>> 
>> I tried to compare the addresses of the previous and the current iteration,
>> if they are equal, then throw "looks like recursion, bad function!”.
> 
> Could you please share your patch?
> 
>> But I got swim tests failing. That is, they use some recursive
>> serializers that do not overflow the stack.
> 
> Is it done intentionally or this behaviour can be changed?
Hm, I think intentionally. Then I need to study the swim code
to fix this. Perhaps we can't do without it :(

>> Therefore, I settled on the idea of introducing a recursion limit.
> 
> Nevertheless, I still propose one of the following:
> * make the limit configurable via box interface
> * introduce a "soft" limit to inform user when recursion occurs (e.g.
>  using log with "WARN" facility) and a "hard" one to stop the instance
> 
> IMHO, the best is to implement both proposals. Thoughts?
I started doing this and thought, why? Why would a user need to
regulate this? In my opinion, the main goal of the patch is to
avoid the "bus error".

> Side note: __tostring Lua metamethod obligues user to yield a string,
> but I see __serialize method doesn't have such restrictions. Otherwise,
> the fix would be brief and clear.

Do you mean to add this check after serialization? Based on the
serializers in swim, __serialize may return an accepted value,
which may not be a string.

>> 
> 
> <snipped>
> 
>> 
> 
> -- 
> Best regards,
> IM

commit fa3131372bdaeb54015b851d2a84afc1e5d2449a
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Mar 10 19:29:10 2020 +0300

    serilaizer: check for recursive serialization
    
    Add a limit to the number of calls to the __serialize function.
    Throw error in case of very deep (most likely endless) recursion.
    
    Closes #3228

diff --git a/src/lua/utils.c b/src/lua/utils.c
index af114b0a2..8d3aa3450 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -52,6 +52,9 @@ static uint32_t CTID_CONST_CHAR_PTR;
 static uint32_t CTID_UUID;
 uint32_t CTID_DECIMAL;
 
+enum {
+	SERIALIZER_CRITICAL_RECURSION_DEPTH = 256
+};
 
 void *
 luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
@@ -492,6 +495,11 @@ static int
 lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
 			int idx, struct luaL_field *field)
 {
+	if (idx > SERIALIZER_CRITICAL_RECURSION_DEPTH) {
+		diag_set(LuajitError, LUAL_SERIALIZE " generates too deep "
+			 "recursion");
+		return -1;
+	}
 	if (luaL_getmetafield(L, idx, LUAL_SERIALIZE) == 0)
 		return 1;
 	if (lua_isfunction(L, -1)) {
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..f105bfae9
--- /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: __serialize generates
+ |     too deep recursion'
+ | ...
+setmetatable({}, {__serialize = function(a, b, c) return a, b, c end})
+ | ---
+ | - error: 'console: an exception occurred when formatting the output: __serialize generates
+ |     too deep recursion'
+ | ...
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})