Re: [Tarantool-patches] [PATCH] serilaizer: check for recursive serialization

[Igor Munkin <imun@tarantool.org>]

Roma,

On 01.10.20, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> > On Sep 16, 2020, at 10:29, Igor Munkin <imun@tarantool.org> 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.

> >> 
> > 
> > <snipped>
> > 
> >> 
> > 
> > -- 
> > Best regards,
> > IM

<snipped>

-- 
Best regards,
IM