From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A9176469719 for ; Thu, 1 Oct 2020 00:49:07 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <20200916072916.GL18920@tarantool.org> Date: Thu, 1 Oct 2020 00:49:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <62DACB5E-6673-4DE8-A3B0-0E23FC546059@tarantool.org> References: <20200710120109.91675-1-roman.habibov@tarantool.org> <20200710122958.GF1999@grain> <20200714094533.GK5559@tarantool.org> <20200714104027.GF296695@grain> <20200916072916.GL18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] serilaizer: check for recursive serialization List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Sep 16, 2020, at 10:29, Igor Munkin wrote: >=20 > Roma, >=20 > On 14.09.20, Roman Khabibov wrote: >> Hi, Cyrill and Igor! >>=20 >> I tried to compare the addresses of the previous and the current = iteration, >> if they are equal, then throw "looks like recursion, bad = function!=E2=80=9D. >=20 > Could you please share your patch? >=20 >> But I got swim tests failing. That is, they use some recursive >> serializers that do not overflow the stack. >=20 > 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. >=20 > 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 >=20 > 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. >>=20 >=20 > >=20 >>=20 >=20 > --=20 > Best regards, > IM commit fa3131372bdaeb54015b851d2a84afc1e5d2449a Author: Roman Khabibov Date: Tue Mar 10 19:29:10 2020 +0300 serilaizer: check for recursive serialization =20 Add a limit to the number of calls to the __serialize function. Throw error in case of very deep (most likely endless) recursion. =20 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; =20 +enum { + SERIALIZER_CRITICAL_RECURSION_DEPTH =3D 256 +}; =20 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) =3D=3D 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 =3D require('test_run').new() + | --- + | ... + +-- +-- gh-3228: Check the error message in the case of a __serialize +-- function generating infinite recursion. +-- +setmetatable({}, {__serialize =3D function(a) return a end}) + | --- + | - error: 'console: an exception occurred when formatting the output: = __serialize generates + | too deep recursion' + | ... +setmetatable({}, {__serialize =3D 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 =3D require('test_run').new() + +-- +-- gh-3228: Check the error message in the case of a __serialize +-- function generating infinite recursion. +-- +setmetatable({}, {__serialize =3D function(a) return a end}) +setmetatable({}, {__serialize =3D function(a, b, c) return a, b, c = end})