Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths

["Alexander V. Tikhonov" <avtikhon@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 <alexander.turenko@tarantool.org>
> > : Sent: Tuesday, November 10, 2020 11:13 AM
> > : To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > : Cc: Alexander Turenko <alexander.turenko@tarantool.org>; tarantool-
> > : patches@dev.tarantool.org; Timur Safin <tsafin@tarantool.org>
> > : 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
> > 
> >