From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 E6A59469710 for ; Tue, 17 Nov 2020 15:25:41 +0300 (MSK) Date: Tue, 17 Nov 2020 15:25:48 +0300 From: Alexander Turenko Message-ID: <20201117122548.oj3xd4vdtb4a5mfn@tkn_work_nb> References: <05c701d6bcbd$0df4dbc0$29de9340$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <05c701d6bcbd$0df4dbc0$29de9340$@tarantool.org> 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 V. Tikhonov" Cc: tarantool-patches@dev.tarantool.org, 'Vladislav Shpilevoy' 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 > >