From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 E3328469719 for ; Tue, 17 Nov 2020 11:38:47 +0300 (MSK) From: "Timur Safin" References: In-Reply-To: Date: Tue, 17 Nov 2020 11:38:41 +0300 Message-ID: <05c701d6bcbd$0df4dbc0$29de9340$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: ru 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' , 'Vladislav Shpilevoy' Cc: tarantool-patches@dev.tarantool.org 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=20 reasonable ideas applicable in test scenarios (unless we have mechanism = of=20 memory allocation failures injections, which is doable in a longer run, = but tricky) LGTM 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 :=20 : There are two situations that may lead to a NULL dereference. :=20 : 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. :=20 : 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. :=20 : Fixes #5450 : --- :=20 : 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 :=20 : CI is not sufficient now, because of [1]. Aside : engine/errinj_ddl.test.lua, all tests are passed on my Linux machine. :=20 : [1]: https://github.com/tarantool/tarantool/issues/5473 :=20 : 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 :=20 : 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); :=20 : struct merge_source *source =3D luaL_merge_source_new(L); : - if (source =3D=3D NULL) { : - merge_source_unref(source); : + if (source =3D=3D NULL) : return luaT_error(L); : - } : *(struct merge_source **) : luaL_pushcdata(L, CTID_STRUCT_MERGE_SOURCE_REF) =3D source; : lua_pushcfunction(L, lbox_merge_source_gc); : @@ -387,10 +385,8 @@ lbox_merger_new(struct lua_State *L) : struct merge_source *merger =3D merger_new(key_def, sources, : source_count, : reverse); : free(sources); : - if (merger =3D=3D NULL) { : - merge_source_unref(merger); : + if (merger =3D=3D NULL) : return luaT_error(L); : - } :=20 : *(struct merge_source **) : luaL_pushcdata(L, CTID_STRUCT_MERGE_SOURCE_REF) =3D 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 =3D require('tap') : +local key_def_lib =3D require('key_def') : +local merger =3D require('merger') : + : +local test =3D 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 =3D key_def_lib.new({ : + {fieldno =3D 6, type =3D 'string'}, : + {fieldno =3D 6, type =3D 'unsigned'}, : +}) : +local exp_err =3D "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 =3D {} : +local ok, err =3D 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