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

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

Thanks for the patch!

> 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
> @@ -396,16 +464,12 @@ luaL_merge_source_buffer_new(struct lua_State *L)
>  }
>  
>  /**
> - * 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.
> + * Helper for `luaL_merge_source_buffer_fetch()`.
>   */
>  static int
> -luaL_merge_source_buffer_fetch(struct merge_source_buffer *source)
> +luaL_merge_source_buffer_fetch_impl(struct lua_State *L,
> +				    struct merge_source_buffer *source)

I see my comment to the former first commit was missed somewhy, so I say
it here:

    - If a function takes lua_State, it is not mean it should get
      luaL_ prefix. Since the first commit was dropped, this comment
      we can't fix.

    - If a function is a method of struct, struct pointer goes *always*
      first, like 'this' argument in C++. Present of lua_State does
      not mean it should be first. This can and should be fixed, I think.

This function in the first place is a method of merge_source_buffer.
So it should take this argument first. luaL_merge_source_buffer_fetch_impl()
currently is the only method of merge_source_buffer, which takes
merge_source_buffer not in the first argument. It means, I don't see
how the consistency is improved here. The same for
luaL_merge_source_table_fetch, luaL_merge_source_table_next_impl,
luaL_merge_source_tuple_fetch, luaL_merge_source_tuple_next_impl.