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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 3 comments below.

On 01/06/2020 20:10, Alexander Turenko wrote:
> A merge source API is designed to be quite abstract: the base structure
> and virtual methods do not depend on Lua anyhow. Each source should
> implement next() and destroy() virtual methods, which may be called from
> C without a Lua state. This design allows to use any source as from C as
> well as from Lua. The Lua API is based on the C API and supports any
> source. Even merger itself is implemented in pure C according to the
> merge source API and so may be used from Lua.
> 
> A particular source implementation may use a Lua state internally, but
> it is not part of the API and should be hid under hood. In fact all

1. 'hid' -> 'hidden'. In passive voice V3 is used.

> 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.
> 
> Before this patch, the sources ('buffer', 'table', 'tuple') assume that
> a Lua state always exists in the fiber storage of a fiber, where next()
> is called. This looks valid on the first glance, because it may be
> called either from a Lua code or from merger, which in turn is called
> from a Lua code. However background fibers (they serve binary protocol
> requests) do not store a Lua state in the fiber storage even for Lua
> call / eval requests.
> 
> Possible solution would be always store a Lua state in a fiber storage.
> There are two reasons why it is not implemented here:
> 
> 1. There should be a decision about right balance between speed and
>    memory footprint and maybe some eviction strategy for cached Lua
>    states. Don't sure we can just always store a state in each

2. "Don't sure" -> "Not sure", or "I am not sure".

>    background fiber. It would be wasteful for instances that serve box
>    DQL / DML, SQL and/or C procedure calls.
> 2. Technically contract of the next() method would assume that a Lua
>    state should exist in a fiber storage. Such requirement looks quite
>    unnatural for a C API and also looks fragile: what if we'll implement
>    some less wasteful Lua state caching strategy and the assumption
>    about presence of the Lua state will get broken?
> 
> Obviously, next() will spend extra time to create a temporary state when
> it is called from a background fiber. We should reuse existing Lua state
> at least when a Lua call is performed via a binary protocol. I consider
> it as the optimization and will solve in the next commit.
> 
> 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.
> 
> Fixes #4954
> ---
>  src/box/lua/merger.c                          | 153 +++++++++++++++---
>  .../gh-4954-merger-via-net-box.test.lua       | 129 +++++++++++++++
>  2 files changed, 261 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 16814c041..25df18442 100644
> --- a/src/box/lua/merger.c
> +++ b/src/box/lua/merger.c> @@ -446,6 +504,25 @@ merge_source_buffer_fetch(struct merge_source_buffer *source)
>  	return 1;
>  }
>  
> +/**
> + * Call a user provided function to get a next data chunk (a
> + * buffer).
> + *
> + * Return 1 when a new buffer is received, 0 when a buffers
> + * iterator ends and -1 at error and set a diag.
> + */
> +static int
> +merge_source_buffer_fetch(struct merge_source_buffer *source)
> +{
> +	int coro_ref = LUA_REFNIL;
> +	struct lua_State *L = luaT_temp_luastate(&coro_ref);
> +	if (L == NULL)
> +		return -1;
> +	int rc = luaL_merge_source_buffer_fetch_impl(L, source);

3. Looks like if luaL_merge_source_tuple_fetch() gets
luaL_iterator_next() result != 2, you leave on the stack whatever is
left here. If fiber's Lua stack is used, there will be left garbage
on it. It is popped in case the merger is used from Lua, when an error
is thrown. But if the merger would be used from C, the stack would
contain garbage afterwards.

The same for luaL_merge_source_buffer_fetch_impl(), but not only if
luaL_iterator_next() returns value != 2.

Not related to this patchset.

> +	luaT_release_temp_luastate(coro_ref);
> +	return rc;
> +}