From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 3921745C304 for ; Wed, 2 Dec 2020 03:53:54 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <20201123202841.GC14086@tarantool.org> Date: Wed, 2 Dec 2020 03:53:52 +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: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Thanks for the LGTM. SergOs, could you, please, look through the patch? > 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 commit 0eebee84ac425fc028f07920352ad2f9ec8be1e1 (HEAD -> = romanhabibov/serialize-check, origin/romanhabibov/serialize-check) Author: Roman Khabibov Date: Thu Oct 8 18:22:24 2020 +0300 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.' ... ``` 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})