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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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
> 
>