From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 92CC945C305 for ; Tue, 8 Dec 2020 19:59:39 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: Date: Tue, 8 Dec 2020 19:59:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20201117164012.14652-1-roman.habibov@tarantool.org> <20201123202841.GC14086@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] serializer: check for recursive serialization List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! My biggest concern was how would you check the recursion appears. You = just check if the result is equivalent to the argument. To me it is not enough, obviously. I tried this on your = branch and=E2=80=A6 tarantool> setmetatable({},{__serialize =3D function(_) return {_} end}) Segmentation fault (core dumped) Regards, Sergos > On 2 Dec 2020, at 03:53, Roman Khabibov = wrote: >=20 > Thanks for the LGTM. >=20 > SergOs, could you, please, look through the patch? >=20 >> On Nov 23, 2020, at 23:28, Igor Munkin wrote: >>=20 >> Roma, >>=20 >> Thanks for the patch! This version looks much better than the = previous one, >> but I still have a couple of nits. Otherwise LGTM. >>=20 >> On 17.11.20, Roman Khabibov wrote: >>> Print error if object after serialization is the same. >>=20 >> I believe we need a doc request to update __serialize description, = since >> its behaviour is restricted with the introduced constraint now. >>=20 >>>=20 >>> Closes #3228 >>> --- >>>=20 >>> Branch: = https://github.com/tarantool/tarantool/tree/romanhabibov/serialize-check >>> Issue: https://github.com/tarantool/tarantool/issues/3228 >>>=20 >>> @ChangeLog: >>> * Fix bug with bus error when __serialize function generates = infinite recursion (gh-3228). >>>=20 >>> 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 >>>=20 >>=20 >> >>=20 >>> 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 =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: Bad __serialize >>> + | function. It can''t return the same value.' >>> + | ... >>> +setmetatable({}, {__serialize =3D 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.' >>=20 >> 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. >>=20 >>> + | ... >>=20 >> >>=20 >>> --=20 >>> 2.24.3 (Apple Git-128) >>>=20 >>=20 >> --=20 >> Best regards, >> IM >=20 > commit 0eebee84ac425fc028f07920352ad2f9ec8be1e1 (HEAD -> = romanhabibov/serialize-check, origin/romanhabibov/serialize-check) > Author: Roman Khabibov > Date: Thu Oct 8 18:22:24 2020 +0300 >=20 > serializer: check for recursive serialization >=20 > Print error if object after serialization is the same. >=20 > Closes #3228 >=20 > @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. >=20 > Example: > ``` > tarantool> setmetatable({},{__serialize =3D function(_) return _ = end}) > --- > - error: 'console: an exception occurred when formatting the = output: Bad __serialize > function. It can''t return the same value.' > ... > ``` >=20 > 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) =3D=3D 1) { > + diag_set(LuajitError, "Bad __serialize function. = It " > + "can't return the same value."); > + return -1; > + } > if (luaL_tofield(L, cfg, NULL, -1, field) !=3D 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 =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: Bad __serialize > + | function. It can''t return the same value.' > + | ... > + > +-- > +--Check that __eq metamethod is ignored. > +-- > +local table =3D setmetatable({}, {__eq =3D function(a, b) error('__eq = is called') end}) > + | --- > + | ... > +setmetatable(table, {__serialize =3D 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 =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}) > + > +-- > +--Check that __eq metamethod is ignored. > +-- > +local table =3D setmetatable({}, {__eq =3D function(a, b) error('__eq = is called') end}) > +setmetatable(table, {__serialize =3D function(a) return a end}) >=20