From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 4A7C341C5DE for ; Wed, 1 Jul 2020 23:46:49 +0300 (MSK) Date: Wed, 1 Jul 2020 23:36:33 +0300 From: Igor Munkin Message-ID: <20200701203633.GA5559@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Sasha, Thanks for the patch! It LGTM except the couple of nits I left below. On 18.06.20, Alexander Turenko wrote: > A particular source implementation may use a Lua state internally, but > it is not part of the API and should be hidden under hood. In fact all Typo: s/under hood/under the hood/ or s/under hood/under its hood/. > sources we have now (except merger itself) store some references in > LUA_REGISTRYINDEX and need a temporary Lua stack to work with them in > the next() virtual method. > A few words about the implementation. I have added three functions, > which acquire a temporary Lua state, call a function and release the > state. It may be squashed into one function that would accept a function > pointer and variable number of arguments. However GCC does not > devirtualize such calls at -O2 level, so it seems it is better to avoid > this. It maybe possible to write some weird macro that will technically > reduce code duplication, but I prefer to write in C, not some macro > based meta-language. Side note: No one pushes you to create a particular DSL for this case, but I see nothing criminal to use macros sometimes. I personally prefer to generalize the occurrences you mentioned above. On the second thought I guess performance deviation is negligible and the benefits for the further maintenance are doubtful. > > --- > src/box/lua/merger.c | 189 ++++++++++++++++-- > .../gh-4954-merger-via-net-box.test.lua | 129 ++++++++++++ > 2 files changed, 297 insertions(+), 21 deletions(-) > create mode 100755 test/box-tap/gh-4954-merger-via-net-box.test.lua > > diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c > index 1b155152b..cc5626cbc 100644 > --- a/src/box/lua/merger.c > +++ b/src/box/lua/merger.c > @@ -149,6 +149,74 @@ luaT_gettuple(struct lua_State *L, int idx, struct tuple_format *format) > return tuple; > } > > +/** > + * Get a temporary Lua state. > + * > + * Use case: a function does not accept a Lua state as an argument > + * to allow using from C code, but uses a Lua value, which is > + * referenced in LUA_REGISTRYINDEX. A temporary Lua stack is needed > + * to get and process the value. > + * > + * The returned state shares LUA_REGISTRYINDEX with `tarantool_L`. Pardon, I don't get this line. > + * > + * This Lua state should be used only from one fiber: otherwise > + * one fiber may change the stack and another one will access a > + * wrong stack slot when it will be scheduled for execution after > + * yield. > + * > + * Return a Lua state on success and set @a coro_ref. This > + * reference should be passed to `luaT_release_temp_luastate()`, > + * when the state is not needed anymore. > + * > + * Return NULL and set a diag at failure. > + */ > +static struct lua_State * > +luaT_temp_luastate(int *coro_ref) > +{ > + if (fiber()->storage.lua.stack != NULL) { > + *coro_ref = LUA_REFNIL; It definitely doesn't affect the implemented behaviour (considering you're not referencing a value within ); I'm just too pedantic here: LUA_REFNIL is the ref value obtained from call anchoring a slot. At the same time there is another special ref value for your purposes -- LUA_NOREF[1]. Furthermore, it's the way more convenient to use it for *all* initial ref values below. > + return fiber()->storage.lua.stack; > + } > + > + /* > + * luaT_newthread() pops the new Lua state from > + * tarantool_L and it is right thing to do: if we'll push > + * something to it and yield, then another fiber will not > + * know that a stack top is changed and may operate on a > + * wrong slot. It seems to relate more to contract, so you can just mention that it leaves no garbage on the given coroutine stack, ergo nothing need to be popped in the caller function. > + * > + * Second, many requests that push a value to tarantool_L > + * and yield may exhaust available slots on the stack. Pardon, I don't get this line. > + */ > + struct lua_State *L = luaT_newthread(tarantool_L); > + if (L == NULL) > + return NULL; > + /* > + * The new state is not referenced from anywhere (reasons > + * are above), so we should keep a reference to it in the > + * registry while it is in use. > + */ > + *coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX); > + return L; > +} > + > +/** > + * Release a temporary Lua state. > + * > + * It is the other half of `luaT_temp_luastate()`. It's not a half, it's a complement for function. > + */ > -- > 2.25.0 > [1]: https://www.lua.org/manual/5.1/manual.html#pdf-LUA_NOREF -- Best regards, IM