From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 CEFA0469710 for ; Mon, 23 Nov 2020 23:28:45 +0300 (MSK) Date: Mon, 23 Nov 2020 23:28:41 +0300 From: Igor Munkin Message-ID: <20201123202841.GC14086@tarantool.org> References: <20201117164012.14652-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201117164012.14652-1-roman.habibov@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, alexander.turenko@tarantool.org Roma, Thanks for the patch! This version looks much better than the previous one, but I still have a couple of nits. Otherwise LGTM. On 17.11.20, Roman Khabibov wrote: > Print error if object after serialization is the same. I believe we need a doc request to update __serialize description, since its behaviour is restricted with the introduced constraint now. > > Closes #3228 > --- > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/serialize-check > Issue: https://github.com/tarantool/tarantool/issues/3228 > > @ChangeLog: > * Fix bug with bus error when __serialize function generates infinite recursion (gh-3228). > > 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 > > 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 = 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: Bad __serialize > + | function. It can''t return the same value.' > + | ... > +setmetatable({}, {__serialize = 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.' 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. > + | ... > -- > 2.24.3 (Apple Git-128) > -- Best regards, IM