[Tarantool-patches] [PATCH] lua: panic on lua_gettop() negative return value

[Tarantool-patches] [PATCH] lua: panic on lua_gettop() negative return value

[Ilya Kosarev]

According to gh-4649 report it seems to be possible that we are getting
segfault on empty diag in iproto_reply_error() due to negative count of
dumped entries returned from port_lua_do_dump() in tx_process_call().
It can only happen due to lua_gettop() returning negative value in
encode_lua_call(). This should not happen at all, so it is the reason
to panic.

Closes #4649
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-empty-diag-from-tx_process_call
Issue: https://github.com/tarantool/tarantool/issues/4649

@ChangeLog:
 * Panic in case of critical problem: lua_gettop() returning negative
 value (gh-4649).

 src/box/lua/call.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index ca871e077..82ca47cbe 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -361,6 +361,8 @@ encode_lua_call(lua_State *L)
 
 	struct luaL_serializer *cfg = luaL_msgpack_default;
 	int size = lua_gettop(port->L);
+	if (size < 0)
+		panic("lua_gettop() returned negative value");
 	for (int i = 1; i <= size; ++i)
 		luamp_encode(port->L, cfg, &stream, i);
 	port->size = size;
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] lua: panic on lua_gettop() negative return value

[sergos]

Hi!

Thanks for the patch!
LGTM.

Sergos

> On 16 Jul 2020, at 21:16, Ilya Kosarev <i.kosarev@tarantool.org> wrote:
> 
> According to gh-4649 report it seems to be possible that we are getting
> segfault on empty diag in iproto_reply_error() due to negative count of
> dumped entries returned from port_lua_do_dump() in tx_process_call().
> It can only happen due to lua_gettop() returning negative value in
> encode_lua_call(). This should not happen at all, so it is the reason
> to panic.
> 
> Closes #4649
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-empty-diag-from-tx_process_call
> Issue: https://github.com/tarantool/tarantool/issues/4649
> 
> @ChangeLog:
> * Panic in case of critical problem: lua_gettop() returning negative
> value (gh-4649).
> 
> src/box/lua/call.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index ca871e077..82ca47cbe 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -361,6 +361,8 @@ encode_lua_call(lua_State *L)
> 
> 	struct luaL_serializer *cfg = luaL_msgpack_default;
> 	int size = lua_gettop(port->L);
> +	if (size < 0)
> +		panic("lua_gettop() returned negative value");
> 	for (int i = 1; i <= size; ++i)
> 		luamp_encode(port->L, cfg, &stream, i);
> 	port->size = size;
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH] lua: panic on lua_gettop() negative return value

[Igor Munkin]

Ilya,

Thanks for the patch! The patch itself is OK, but I see no reason to
apply it to stable branches. Yes, it can be freely applied for a custom
branch to be tested under the particular workload. But this assumption
is simply crazy and I've seen nothing confirming it. If <lua_gettop>
returns a negative value, it means the internal state of the given Lua
coroutine (i.e. port->L) is completely broken (e.g. as a result of
manual manipulation with L->base and L->top). If such breakage occurs
*its root cause* definitely has to be fixed ASAP.

What I want to say is that assert is more than enough here for stable
branches. However, I'm totally for such experimental build to check your
assumption and reproduce the possible failure.

On 16.07.20, Ilya Kosarev wrote:
> According to gh-4649 report it seems to be possible that we are getting
> segfault on empty diag in iproto_reply_error() due to negative count of
> dumped entries returned from port_lua_do_dump() in tx_process_call().
> It can only happen due to lua_gettop() returning negative value in
> encode_lua_call(). This should not happen at all, so it is the reason
> to panic.
> 
> Closes #4649
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-empty-diag-from-tx_process_call
> Issue: https://github.com/tarantool/tarantool/issues/4649
> 

<snipped>

> 

-- 
Best regards,
IM