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

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- 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 --]