Re: [Tarantool-patches] [PATCH v2 1/3] merger: fix NULL dereference when called via iproto

[Igor Munkin <imun@tarantool.org>]

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