From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1A152445320 for ; Tue, 21 Jul 2020 21:17:49 +0300 (MSK) Date: Tue, 21 Jul 2020 21:07:30 +0300 From: Igor Munkin Message-ID: <20200721180730.GK18920@tarantool.org> References: <20200721110514.10344-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200721110514.10344-1-i.kosarev@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] lua: assert on lua_gettop() negative return value List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@dev.tarantool.org 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 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 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(+) > > -- > 2.17.1 > -- Best regards, IM