From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id EBB606EC55; Tue, 7 Sep 2021 15:42:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EBB606EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1631018543; bh=R+APCQyK2nhJi1naw9QI36N5NprhUNxEF94HETqvTAM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=fZEu2brhFtuvM2dnPfHZdmCmCe1CCbAryyQqQgvKLK3sKUKMczlv3UGVtg5xmFuiT PsaK1KtZHyUkXM3bPILP5DciCtLmjzeaoolt3BOx9P0SlCeBQma5+R3mNFyXi+iv0O rGUGGELuE7ntdYF4pDEoBzIUvarxmlrXvxDAJ9sM= Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 ADB556EC55 for ; Tue, 7 Sep 2021 15:42:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org ADB556EC55 Received: by smtp17.mail.ru with esmtpa (envelope-from ) id 1mNaQe-0003Dd-Bt; Tue, 07 Sep 2021 15:42:21 +0300 Date: Tue, 7 Sep 2021 15:40:57 +0300 To: Alexander Turenko Message-ID: References: <1591698918.771792066@f423.i.mail.ru> <20210701123434.ezfp3bsmhny73jjf@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210701123434.ezfp3bsmhny73jjf@tkn_work_nb> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D96C1EA41D18F4D5E267050317C242BB27DD410512DDC118182A05F5380850401893D6A49FBD2B2A46A2C7726D4BE5F4590641E26D33C6766E6B4952360CD92B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71BDE6A359BD5B800EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063768D6DD405B71470F8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87BBFCB7E4E2897CB796A79D91F3A4A7A117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BF1175FABE1C0F9B6A471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CD0F529D6CE73765543847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A570A3E2C2E08F6879D3200877538C32FC8608E5C7F2B9828FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A63B03BCD35E0C0A303F4A6E5C184DFD6217DFB3452C52EC6CE8360FA44511586E1C279096F71FF31D7E09C32AA3244CB486346F4483438BFE54B28ADB7D8F95BBA718C7E6A9E042927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSvkey75OmIp3na+GSefLzQ== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB419E2F3F0596F4B52BBA9D932D8A229BF38471A225AD4F9A1F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] box: remove context from stack X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Alexander! Sorry for the long response! On 01.07.21, Alexander Turenko wrote: > On Tue, Jun 09, 2020 at 01:35:18PM +0300, Maria Khaydich wrote: > > > > I suppose no tests are needed since this one is pretty straightforward. > > ---------------------------------------------------------------------- > >   > > Lua stack was broken because we forgot to clear the context > > in case of an error when return value of called function was > > not serializable. > >   > > Closes #4617 > > --- > > Branch: > > https://github.com/tarantool/tarantool/compare/eljashm/gh-4617-broken-lua-stack   > > Issue: > > https://github.com/tarantool/tarantool/issues/4617   > >   > > @ChangeLog > > Remove context from lua stack in case of an error. > > Here we describe changes for a user (in the way a user may understand > it). For complex bugs we can briefly describe a situation, when it > occurs. > > It is now in the changelogs/ directory. > > >   > >  src/box/lua/call.c | 1 + > >  1 file changed, 1 insertion(+) > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > > index 6588ec2fa..7ab49983d 100644 > > --- a/src/box/lua/call.c > > +++ b/src/box/lua/call.c > > @@ -436,6 +436,7 @@ port_lua_do_dump(struct port *base, struct mpstream *stream, > >      int top = lua_gettop(L); > >      if (lua_cpcall(L, handler, &ctx) != 0) { > >          luaT_toerror(port->L); > > +        lua_pop(L, 1);  > > There are several doubts and thoughts around it: To answer the questions below we need to describe what is going on here. The old code flow was the following: 1) `struct port_lua` given to `port_lua_do_dump()` has Lua stack with arguments to encode to MessagePack. 2) The main coroutine `tarantool_L` is used to call `encode_lua_call()` or `encode_lua_call_16`() via `lua_cpcall()`. 3) Objects on port coroutine are encoded via `luamp_encode()` or `luamp_encode_call16()`. 4) This encoding may raise an error on unprotected `port->L` coroutine. This coroutine has no protected frame on it and this call should fail in pure Lua. Calling anything on unprotected coroutine is not allowed in Lua [1]: | If an error happens outside any protected environment, Lua calls a | panic function Lua 5.1 sets protection only for specific lua_State [2] and calls a panic function if we raise an error on unprotected lua_State [3]. Nevertheless, no panic occurs now due to two facts: * The first one is LuaJIT's support of C++ exception handling [4] that allows to raise an error in Lua and catch it in C++ or vice versa. But documentation still doesn't allow raising errors on unprotected coroutines (at least we must use try-catch block). * The second one is the patch made in LuaJIT to restore currently executed coroutine, when C function or fast function raises an error [5][6] (see the related issue here [7][8]) (*). For these reasons, when an error occurs, the unwinder searches and finds the C-protected stack frame from the `lua_cpcall()` for `tarantool_L` coroutine and unwinds until that point (without aforementioned patches LuaJIT just calls a panic function and exits). If an error is raised, and `lua_cpcall()` returns not `LUA_OK`, then the error from `port->L` coroutine is converted into a Tarantool error and a diagnostic is set. (*) P.S. But raise an error on non currently executed coroutine is violation of Lua C API too (see [9][10]). > > 1. lua_cpcall() always removes arguments from the Lua stack (in case of > an error too). How we can observe this ctx userdata on tarantool_L > without yield in encode_lua_call()? Within the violation of Lua C API the behaviour is implementation depended. cpcall removes arguments from `port->L` Lua stack, but not from `tarantool_L`. > 2. luaT_toerror() leans on luaT_tolstring(), so it duplicates a string > with the error message. Should not we pop two elements in the case? I > would just call lua_settop(). `luaT_toerror()` works with `port->L`, while we "want" to clean `tarantool_L` (`L`) coroutine. > 3. The clash is due to tarantool_L usage (see 1.10.1-48-g18d87adf3). Is > there any real reason why we can't use port->L here? I don't see any reason to avoid it. So this issue was fixed [11][12][13] via usage of `port->L` Lua stack here instead. > > CCed Igor and Sergey to think around together. > > >          return -1; > >      } > >      lua_settop(L, top); > > --  > > 2.24.0 > [1]: https://www.lua.org/manual/5.2/manual.html#4.6 [2]: https://www.lua.org/source/5.1/lstate.h.html#lua_State [3]: https://www.lua.org/source/5.1/ldo.c.html#luaD_throw [4]: https://luajit.org/extensions.html#exceptions [5]: https://github.com/tarantool/luajit/commit/ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 [6]: https://github.com/tarantool/luajit/commit/97699d9ee2467389b6aea21a098e38aff3469b5f [7]: https://github.com/tarantool/tarantool/issues/1516 [8]: https://www.freelists.org/post/luajit/Issue-with-PCALL-in-21 [9]: https://github.com/LuaJIT/LuaJIT/pull/740#issuecomment-897173357 [10]: http://lua-users.org/lists/lua-l/2021-08/msg00084.html [11]: https://github.com/tarantool/tarantool/commit/027775ff22993b03886f3bcc002e9c257ad09c02 [12]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025078.html [13]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-June/024351.html -- Best regards, Sergey Kaplun