[Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto
Igor Munkin
imun at tarantool.org
Wed Jul 1 23:36:33 MSK 2020
Sasha,
Thanks for the patch! It LGTM except the couple of nits I left below.
On 18.06.20, Alexander Turenko wrote:
<snipped>
> 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.
<snipped>
> 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.
>
<snipped>
> ---
> 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 <nil> value within <luaT_temp_luastate>); I'm
just too pedantic here: LUA_REFNIL is the ref value obtained from
<luaL_ref> call anchoring a <nil> 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 <luaT_newthread> 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 <luaT_temp_luastate> function.
> + */
<snipped>
> --
> 2.25.0
>
[1]: https://www.lua.org/manual/5.1/manual.html#pdf-LUA_NOREF
--
Best regards,
IM
More information about the Tarantool-patches
mailing list