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

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

[Ilya Kosarev]

In case lua_gettop() called from encode_lua_call() returns negative
value, we will segfault in iproto_reply_error() with empty diag, as far
as it is unexpected error path not covered with diagnostics. Thus
corresponding sane check with assert is introduced.

Closes #4649
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-sane-check-on-lua_gettop
Issue: https://github.com/tarantool/tarantool/issues/4649

Changes in v2:
- added reasoning in comment

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

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index ca871e077..516276a9f 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -361,6 +361,12 @@ encode_lua_call(lua_State *L)
 
 	struct luaL_serializer *cfg = luaL_msgpack_default;
 	int size = lua_gettop(port->L);
+	/*
+	 * lua_gettop() might return negative value in case the internal state
+	 * of the given Lua coroutine is seriously broken. In case of such
+	 * behavior execution has to be aborted immediately.
+	 */
+	assert(size >= 0);
 	for (int i = 1; i <= size; ++i)
 		luamp_encode(port->L, cfg, &stream, i);
 	port->size = size;
-- 
2.17.1

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

[Nikita Pettik]

On 21 Jul 14:05, Ilya Kosarev wrote:
> In case lua_gettop() called from encode_lua_call() returns negative
> value, we will segfault in iproto_reply_error() with empty diag, as far
> as it is unexpected error path not covered with diagnostics. Thus
> corresponding sane check with assert is introduced.
> 
> Closes #4649
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-sane-check-on-lua_gettop
> Issue: https://github.com/tarantool/tarantool/issues/4649
> 
> Changes in v2:
> - added reasoning in comment
> 
>  src/box/lua/call.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index ca871e077..516276a9f 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -361,6 +361,12 @@ encode_lua_call(lua_State *L)
>  
>  	struct luaL_serializer *cfg = luaL_msgpack_default;
>  	int size = lua_gettop(port->L);
> +	/*
> +	 * lua_gettop() might return negative value in case the internal state
> +	 * of the given Lua coroutine is seriously broken. In case of such
> +	 * behavior execution has to be aborted immediately.
> +	 */
> +	assert(size >= 0);

As I said, assuming lua_gettop() does return negative index under no
circumstances and your assumption is false, what other asserts or
checks can you suggest? I mean this problem appears extremely rare,
so we'd better provide extra checks just in case (so that we locate
problem with ease next time). Could you elaborate on this?

>  	for (int i = 1; i <= size; ++i)
>  		luamp_encode(port->L, cfg, &stream, i);
>  	port->size = size;
> -- 
> 2.17.1
> 

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

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]


Hi!
 
See my answer below. 
>Вторник, 21 июля 2020, 14:54 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> 
>On 21 Jul 14:05, Ilya Kosarev wrote:
>> In case lua_gettop() called from encode_lua_call() returns negative
>> value, we will segfault in iproto_reply_error() with empty diag, as far
>> as it is unexpected error path not covered with diagnostics. Thus
>> corresponding sane check with assert is introduced.
>>
>> Closes #4649
>> ---
>> Branch:  https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-sane-check-on-lua_gettop
>> Issue:  https://github.com/tarantool/tarantool/issues/4649
>>
>> Changes in v2:
>> - added reasoning in comment
>>
>> src/box/lua/call.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
>> index ca871e077..516276a9f 100644
>> --- a/src/box/lua/call.c
>> +++ b/src/box/lua/call.c
>> @@ -361,6 +361,12 @@ encode_lua_call(lua_State *L)
>>
>> struct luaL_serializer *cfg = luaL_msgpack_default;
>> int size = lua_gettop(port->L);
>> + /*
>> + * lua_gettop() might return negative value in case the internal state
>> + * of the given Lua coroutine is seriously broken. In case of such
>> + * behavior execution has to be aborted immediately.
>> + */
>> + assert(size >= 0);
>
>As I said, assuming lua_gettop() does return negative index under no
>circumstances and your assumption is false, what other asserts or
>checks can you suggest? I mean this problem appears extremely rare,
>so we'd better provide extra checks just in case (so that we locate
>problem with ease next time). Could you elaborate on this?
After last careful review finally using proper coredump
as far as i see it is the last option for empty diag segfault
mentioned in gh-4649. For some reasons it wasn’t previously
detected.
>
>> for (int i = 1; i <= size; ++i)
>> luamp_encode(port->L, cfg, &stream, i);
>> port->size = size;
>> --
>> 2.17.1
>> 
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 3055 bytes --]

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

[Igor Munkin]

Ilya,

Thanks for the patch! As I wrote in the previous series, your assumption
is a bit insane. So if you're right it means the complete mess in the
internal state of the given coroutine.

There are little places where the pointers to top and base Lua stack
slots are invalidated, *but* all such occurrences are enclosed in VM.
These places are unreachable for the "client" code (e.g. via Lua C API).

However, since Tarantool bundles LuaJIT and <struct lua_State> is not
opaque anymore, there *might* be places where the mentioned pointers are
managed manually. I glanced the sources and failed to find top or base
pointer used as an assignment destination.

Besides, Tarantool doesn't respect the native way Lua coroutines switch.
We already faced several issues with JIT machinery caused by this
peculiarity (e.g. so called FFI sandwiches), so maybe your case also
relates to fiber switch specifics.

Considering everything above it looks more valuable to add the assertion
right to <lua_gettop> interface. Negative result violates Lua reference
manual[1], so it's an extra check the platform works fine. Thoughts?

On 21.07.20, Ilya Kosarev wrote:
> In case lua_gettop() called from encode_lua_call() returns negative
> value, we will segfault in iproto_reply_error() with empty diag, as far
> as it is unexpected error path not covered with diagnostics. Thus
> corresponding sane check with assert is introduced.
> 
> Closes #4649

Side note: I doubt this patch closes #4649. It just adds more
diagnostics but the doesn't fix the root issue. I guess "Relates to" tag
is much better here.

> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-sane-check-on-lua_gettop
> Issue: https://github.com/tarantool/tarantool/issues/4649
> 
> Changes in v2:
> - added reasoning in comment
> 
>  src/box/lua/call.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

<snipped>

> -- 
> 2.17.1
> 

-- 
Best regards,
IM

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

[Nikita Pettik]

On 21 Jul 21:07, Igor Munkin wrote:
> > corresponding sane check with assert is introduced.
> > 
> > Closes #4649
> 
> Side note: I doubt this patch closes #4649. It just adds more
> diagnostics but the doesn't fix the root issue. I guess "Relates to" tag
> is much better here.

Agree.