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

[Tarantool-patches] [PATCH] 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

@ChangeLog:
 * Raise assert on lua_gettop() negative return value (gh-4649).

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

diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index ca871e077..75634de5b 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -361,6 +361,7 @@ encode_lua_call(lua_State *L)
 
 	struct luaL_serializer *cfg = luaL_msgpack_default;
 	int size = lua_gettop(port->L);
+	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] lua: assert on lua_gettop() negative return value

[Nikita Pettik]

On 17 Jul 20:25, 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
> 
> @ChangeLog:
>  * Raise assert on lua_gettop() negative return value (gh-4649).

I guess changelog is likely to be redundant in this case
(patch doesnt introduce user-visible changes).
 
>  src/box/lua/call.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index ca871e077..75634de5b 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -361,6 +361,7 @@ encode_lua_call(lua_State *L)
>  
>  	struct luaL_serializer *cfg = luaL_msgpack_default;
>  	int size = lua_gettop(port->L);
> +	assert(size >= 0);

I don't understand much in Lua internals, so it would be nice
to see rationale for this check. Even if you don't have
complete insight concerning it, any thoughts which bring you
to this check will be helpful.

>  	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: assert on lua_gettop() negative return value

[Ilya Kosarev]

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


Hi!
 
Thanks for the review.
 
Right, sent v2 considering your comments.
  
>Понедельник, 20 июля 2020, 13:50 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> 
>On 17 Jul 20:25, 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
>>
>> @ChangeLog:
>> * Raise assert on lua_gettop() negative return value (gh-4649).
>
>I guess changelog is likely to be redundant in this case
>(patch doesnt introduce user-visible changes).
> 
>> src/box/lua/call.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
>> index ca871e077..75634de5b 100644
>> --- a/src/box/lua/call.c
>> +++ b/src/box/lua/call.c
>> @@ -361,6 +361,7 @@ encode_lua_call(lua_State *L)
>>
>> struct luaL_serializer *cfg = luaL_msgpack_default;
>> int size = lua_gettop(port->L);
>> + assert(size >= 0);
>
>I don't understand much in Lua internals, so it would be nice
>to see rationale for this check. Even if you don't have
>complete insight concerning it, any thoughts which bring you
>to this check will be helpful.
>
>> 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: 2502 bytes --]