[Tarantool-patches] [PATCH] serilaizer: check for recursive serialization
Igor Munkin
imun at tarantool.org
Tue Jul 14 12:45:33 MSK 2020
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
More information about the Tarantool-patches
mailing list