From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 80503469710 for ; Wed, 18 Nov 2020 15:27:51 +0300 (MSK) Date: Wed, 18 Nov 2020 15:27:49 +0300 From: "Alexander V. Tikhonov" Message-ID: <20201118122749.GA34133@hpalx> References: <05c701d6bcbd$0df4dbc0$29de9340$@tarantool.org> <20201117122548.oj3xd4vdtb4a5mfn@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201117122548.oj3xd4vdtb4a5mfn@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi Alexander, thanks for the patch, as I see no new degradations found in gitlab-ci testing commit criteria pipeline [1], patch LGTM. [1] - https://gitlab.com/tarantool/tarantool/-/pipelines/217180398 On Tue, Nov 17, 2020 at 03:25:48PM +0300, Alexander Turenko wrote: > Alexander, > > I need your approve before I'll push the patch. > > The branch is Totktonada/gh-5450-merger-segfault-on-wrong-key-def. I > rebased it upward the latest master and force-pushed today, because > gh-5473 was resolved. > > CentOS 7 jobs ('centos_7', 'default_gcc_centos_7') failed due to a > problem with vault repository. Fixed in [1] and restarted. They passed > after it. > > 'debug' job failed on engine/ddl.test.lua. Looks similar to [2]. > > 'freebsd_12_release' job failed on replication/election_qsync.test.lua. > Looks similar to [3]. > > Mac OS 10.15 jobs ('osx_15_release', 'osx_15_release_lto', > 'static_build_cmake_osx_15') are still pending. > > [1]: https://github.com/packpack/packpack-docker-images/pull/62 > [2]: https://github.com/tarantool/tarantool/issues/4353 > [3]: https://github.com/tarantool/tarantool/issues/5430 > > WBR, Alexander Turenko. > > On Tue, Nov 17, 2020 at 11:38:41AM +0300, Timur Safin wrote: > > I've looked again into this patch, first part I've already ported to tuple-merger, > > need to apply the 2nd part and test very soon. Thanks you for that! > > > > I tried to think about the way to invent test for 2nd case, but went out of > > reasonable ideas applicable in test scenarios (unless we have mechanism of > > memory allocation failures injections, which is doable in a longer run, but tricky) > > Yep, I concluded the same. > > > > > LGTM > > Thanks! > > > > > Timur > > > > : -----Original Message----- > > : From: Alexander Turenko > > : Sent: Tuesday, November 10, 2020 11:13 AM > > : To: Vladislav Shpilevoy > > : Cc: Alexander Turenko ; tarantool- > > : patches@dev.tarantool.org; Timur Safin > > : Subject: [PATCH] lua/merger: fix NULL dereferences on error paths > > : > > : There are two situations that may lead to a NULL dereference. > > : > > : First, when an attempt to create a merge source is failed. Now only OOM > > : may trigger this code path. Reproducing it in a test is tricky, so I > > : left it without a test case. > > : > > : Second, when an attempt to create a merger is failed. The reason of the > > : fail may be a 'wrong' key_def: when we unable to create a tuple format > > : from it. Say, when the key_def defines several key parts using the same > > : field, but marks them with different field types. > > : > > : Fixes #5450 > > : --- > > : > > : https://github.com/tarantool/tarantool/issues/5450 > > : Totktonada/gh-5450-merger-segfault-on-wrong-key-def > > : https://github.com/tarantool/tarantool/tree/Totktonada/gh-5450-merger- > > : segfault-on-wrong-key-def > > : > > : CI is not sufficient now, because of [1]. Aside > > : engine/errinj_ddl.test.lua, all tests are passed on my Linux machine. > > : > > : [1]: https://github.com/tarantool/tarantool/issues/5473 > > : > > : src/box/lua/merger.c | 8 ++---- > > : .../gh-5450-merger-wrong-key-def.test.lua | 26 +++++++++++++++++++ > > : 2 files changed, 28 insertions(+), 6 deletions(-) > > : create mode 100755 test/box-tap/gh-5450-merger-wrong-key-def.test.lua > > : > > : diff --git a/src/box/lua/merger.c b/src/box/lua/merger.c > > : index 17e237902..89bdfffa6 100644 > > : --- a/src/box/lua/merger.c > > : +++ b/src/box/lua/merger.c > > : @@ -266,10 +266,8 @@ lbox_merge_source_new(struct lua_State *L, const char > > : *func_name, > > : lua_pushnil(L); > > : > > : struct merge_source *source = luaL_merge_source_new(L); > > : - if (source == NULL) { > > : - merge_source_unref(source); > > : + if (source == NULL) > > : return luaT_error(L); > > : - } > > : *(struct merge_source **) > > : luaL_pushcdata(L, CTID_STRUCT_MERGE_SOURCE_REF) = source; > > : lua_pushcfunction(L, lbox_merge_source_gc); > > : @@ -387,10 +385,8 @@ lbox_merger_new(struct lua_State *L) > > : struct merge_source *merger = merger_new(key_def, sources, > > : source_count, > > : reverse); > > : free(sources); > > : - if (merger == NULL) { > > : - merge_source_unref(merger); > > : + if (merger == NULL) > > : return luaT_error(L); > > : - } > > : > > : *(struct merge_source **) > > : luaL_pushcdata(L, CTID_STRUCT_MERGE_SOURCE_REF) = merger; > > : diff --git a/test/box-tap/gh-5450-merger-wrong-key-def.test.lua b/test/box- > > : tap/gh-5450-merger-wrong-key-def.test.lua > > : new file mode 100755 > > : index 000000000..033f42da4 > > : --- /dev/null > > : +++ b/test/box-tap/gh-5450-merger-wrong-key-def.test.lua > > : @@ -0,0 +1,26 @@ > > : +#!/usr/bin/env tarantool > > : + > > : +local tap = require('tap') > > : +local key_def_lib = require('key_def') > > : +local merger = require('merger') > > : + > > : +local test = tap.test('gh-5450-merger-wrong-key-def') > > : +test:plan(1) > > : + > > : +-- gh-5450: creating of merger leads to segfault, when passed > > : +-- key_def is not usable to create a tuple format. > > : +local key_def = key_def_lib.new({ > > : + {fieldno = 6, type = 'string'}, > > : + {fieldno = 6, type = 'unsigned'}, > > : +}) > > : +local exp_err = "Field 6 has type 'string' in one index, but type " .. > > : + "'unsigned' in another" > > : +-- At the moment of writting the test, it is sufficient to pass an > > : +-- empty sources table to trigger the attempt to create a tuple > > : +-- format. > > : +local sources = {} > > : +local ok, err = pcall(merger.new, key_def, sources) > > : +test:is_deeply({ok, tostring(err)}, {false, exp_err}, > > : + 'unable to create a tuple format') > > : + > > : +os.exit(test:check() and 0 or 1) > > : -- > > : 2.25.0 > > > >