From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 B401E469710 for ; Sun, 7 Jun 2020 19:58:30 +0300 (MSK) Date: Sun, 7 Jun 2020 19:58:16 +0300 From: Alexander Turenko Message-ID: <20200607165816.4r5s7ah4pa5uihyl@tkn_work_nb> References: <0f0ad73e2fce564e22dcc8f9970d8aedd028c279.1591028838.git.alexander.turenko@tarantool.org> <920c35d4-150b-bae5-345f-e819ada02367@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <920c35d4-150b-bae5-345f-e819ada02367@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/3] merger: fix NULL dereference when called via iproto List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > > A particular source implementation may use a Lua state internally, but > > it is not part of the API and should be hid under hood. In fact all > > 1. 'hid' -> 'hidden'. In passive voice V3 is used. Fixed. > > 1. There should be a decision about right balance between speed and > > memory footprint and maybe some eviction strategy for cached Lua > > states. Don't sure we can just always store a state in each > > 2. "Don't sure" -> "Not sure", or "I am not sure". Fixed. > > +/** > > + * Call a user provided function to get a next data chunk (a > > + * buffer). > > + * > > + * Return 1 when a new buffer is received, 0 when a buffers > > + * iterator ends and -1 at error and set a diag. > > + */ > > +static int > > +merge_source_buffer_fetch(struct merge_source_buffer *source) > > +{ > > + int coro_ref = LUA_REFNIL; > > + struct lua_State *L = luaT_temp_luastate(&coro_ref); > > + if (L == NULL) > > + return -1; > > + int rc = luaL_merge_source_buffer_fetch_impl(L, source); > > 3. Looks like if luaL_merge_source_tuple_fetch() gets > luaL_iterator_next() result != 2, you leave on the stack whatever is > left here. If fiber's Lua stack is used, there will be left garbage > on it. It is popped in case the merger is used from Lua, when an error > is thrown. But if the merger would be used from C, the stack would > contain garbage afterwards. > > The same for luaL_merge_source_buffer_fetch_impl(), but not only if > luaL_iterator_next() returns value != 2. > > Not related to this patchset. Nice catch! I didn't bother much about this, because it does not matter much for usual Lua/C functions. However this case is different. I think it should be handled by luaT_temp_luastate() / luaT_release_temp_luastate() within this patchset. I have added one more patch to the series (as third). I'll send it as answer to the cover letter ('[PATCH 2.5/3] merger: clean fiber-local Lua stack after next()'). NB: The test that is added within this patch will not work on 2.4 and below, because those tarantool versions do not expose all symbols from the executable. Updated 2nd patch (this one) with FIXME (which will be resolved and removed by the next patch). | static void | luaT_release_temp_luastate(int coro_ref) | { | + /* | + * FIXME: The reusable fiber-local Lua state is not | + * unreferenced here (coro_ref == LUA_REFNIL), but | + * it must be truncated to its past top to prevent | + * stack overflow. | + */ | luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); | } Added the following comment into the commit message: | Usage of the fiber-local Lua state is not quite correct now: merge | source code may left garbage on a stack in case of failures (like | unexpected result of Lua iterator generator function). This behaviour is | kept as is here, but it will be resolved by the next patch. While writting the test, found [1]. I'll send a fix separately. [1]: https://github.com/tarantool/tarantool/issues/5048