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

["Timur Safin" <tsafin@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 
reasonable ideas applicable in test scenarios (unless we have mechanism of 
memory allocation failures injections, which is doable in a longer run, but tricky)

LGTM

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