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 95E78445320 for ; Tue, 14 Jul 2020 12:55:51 +0300 (MSK) Date: Tue, 14 Jul 2020 12:45:33 +0300 From: Igor Munkin Message-ID: <20200714094533.GK5559@tarantool.org> References: <20200710120109.91675-1-roman.habibov@tarantool.org> <20200710122958.GF1999@grain> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200710122958.GF1999@grain> 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: Cyrill Gorcunov Cc: tarantool-patches@dev.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 < 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 "", line 5, in | File "", line 3, in f | File "", line 3, in f | File "", 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