From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 0DA37469719 for ; Thu, 1 Oct 2020 17:51:22 +0300 (MSK) Date: Thu, 1 Oct 2020 17:40:48 +0300 From: Igor Munkin Message-ID: <20201001144048.GD18920@tarantool.org> References: <20200710120109.91675-1-roman.habibov@tarantool.org> <20200710122958.GF1999@grain> <20200714094533.GK5559@tarantool.org> <20200714104027.GF296695@grain> <20200916072916.GL18920@tarantool.org> <62DACB5E-6673-4DE8-A3B0-0E23FC546059@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <62DACB5E-6673-4DE8-A3B0-0E23FC546059@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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Roma, On 01.10.20, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Sep 16, 2020, at 10:29, Igor Munkin 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? Well, the patch below looks like the original one with no addresses comparison at all. > > > >> 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 :( Anyway, let's look at SWIM code/tests together or even summon Vlad for the help as the major SWIM developer. > > >> 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". E.g. if he definitely needs 257 iterations. This value is your estimate, not the exact stack limit. So the end user might need to exceed it in the user code, but has no way to tune this limit for his needs. > > > 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. OK, your patch *perfectly* solves the "bus error" issue, but I believe it's a good point to think about the following: do we need __serialize metamethod going into recursion at all? What interface/contract does this function have? I mean, if we prohibit "echo" __serialize functions, then it's better to implement a check the similar way Mons originally proposed in the issue (yeah, I know you've already tried, but let's see whether we can do anything more here). Otherwise (i.e. if "echo" __serialize functions are allowed), it's better to implement a configurable limit than a hardcoded one, isn't it? I'm not quite sure "256 slots is ought to be enough". > Nevertheless, feel free to ignore everything above if you're sure. I'm not fully in the issue context, so don't see everything around the problem. Just want to make the changes more user-friendly. > >> > > > > > > > >> > > > > -- > > Best regards, > > IM -- Best regards, IM