Tarantool development patches archive
 help / color / mirror / Atom feed
From: Roman Khabibov <roman.habibov@tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] serilaizer: check for recursive serialization
Date: Thu, 1 Oct 2020 00:49:04 +0300	[thread overview]
Message-ID: <62DACB5E-6673-4DE8-A3B0-0E23FC546059@tarantool.org> (raw)
In-Reply-To: <20200916072916.GL18920@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})

  reply	other threads:[~2020-09-30 21:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-10 12:01 Roman Khabibov
2020-07-10 12:29 ` Cyrill Gorcunov
2020-07-14  9:45   ` Igor Munkin
2020-07-14 10:40     ` Cyrill Gorcunov
2020-09-14 14:43       ` Roman Khabibov
2020-09-14 16:06         ` Cyrill Gorcunov
2020-09-16  7:29         ` Igor Munkin
2020-09-30 21:49           ` Roman Khabibov [this message]
2020-10-01 14:40             ` Igor Munkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=62DACB5E-6673-4DE8-A3B0-0E23FC546059@tarantool.org \
    --to=roman.habibov@tarantool.org \
    --cc=imun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] serilaizer: check for recursive serialization' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox