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

[Igor Munkin <imun@tarantool.org>]

Cyrill,

On 10.07.20, Cyrill Gorcunov wrote:
> On Fri, Jul 10, 2020 at 03:01:09PM +0300, Roman Khabibov wrote:
> >  luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
> > @@ -490,6 +493,11 @@ static int
> >  lua_field_try_serialize(struct lua_State *L, struct luaL_serializer *cfg,
> >  			int idx, struct luaL_field *field)
> >  {
> > +	if (idx > SERIALIZER_CRITICAL_RECURSION_DEPTH) {
> > +		diag_set(LuajitError, LUAL_SERIALIZE " generates too deep "
> > +			 "recursion");
> > +		return -1;
> > +	}
> >  	if (luaL_getmetafield(L, idx, LUAL_SERIALIZE) == 0)
> >  		return 1;
> 
> Wait. The @idx stands for index in a table as far as I remember,
> this just happen to hit when you're calling youself recursively
> but @idx may be > SERIALIZER_CRITICAL_RECURSION_DEPTH for nonrecursive
> calls as well.

I guess you've just misread this part: @idx is just a guest stack slot
where Lua object being serialized is stored. If I get your concerns
right, you're talking about the following case:
* before the patch
| $ ./src/tarantool
| Tarantool 2.5.0-184-gbefa36cfd
| type 'help' for interactive help
| tarantool> local b = 0 t = setmetatable({}, {__serialize = function(a)
|          > if b < 256 then b = b + 1 return a end
|          > return "QQ" end})
|  ---
|  ...
|
|  tarantool> t
|  ---
|  - QQ
|  ...
|
* after the patch
| $ ./src/tarantool
| Tarantool 2.5.0-185-gc9501fa8e
| type 'help' for interactive help
| tarantool> local b = 0 t = setmetatable({}, {__serialize = function(a)
|          > if b < 256 then b = b + 1 return a end
|          > return "QQ" end})
| ---
| ...
|
| tarantool> t
| ---
| - error: 'console: an exception occurred when formatting the output: __serialize generates
|     too deep recursion'
| ...
|
| tarantool>

This example looks a bit synthetic, doesn't it? So, I guess we can
assume the patch is fine (omitting that Mons proposed another solution
in the issue but nobody provided its pros and cons).

However, I agree that stack overflow can be handled in a different way.
Roma's implementation is quite similar to the way Python handles stack
overflow:
| $ python <<EOF
| heredoc> def f(n):
| heredoc>   if not n: return 1
| heredoc>   return n * f(n - 1)
| heredoc>
| heredoc> f(2**64)
| heredoc> EOF
| Traceback (most recent call last):
|   File "<stdin>", line 5, in <module>
|   File "<stdin>", line 3, in f
|   File "<stdin>", line 3, in f
|   File "<stdin>", line 3, in f
|   [Previous line repeated 996 more times]
| RecursionError: maximum recursion depth exceeded

However, there is a significant difference between Roma's and Python
approaches that inquisitive reader can find here[1].

Frankly speaking, I don't like such workaround at all and prefer the way
Perl works when deep recursion occurs:
| $ perl -wE 'sub f { my $n = shift; return 1 unless $n; return $n * f($n - 1) } f(2**64)'
| Deep recursion on subroutine "main::f" at -e line 1.
| Killed

IMHO, it would be nice to drop a line to Tarantool log when "soft" stack
overflow is violated, instead of user sandboxing. These are just my
thoughts and not a full review, so feel free to ignore everything above.

> 
> Lets CC our Lua's master: Igor. I might be simply wrong.

[1]: https://docs.python.org/3/library/sys.html#sys.setrecursionlimit

-- 
Best regards,
IM