Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
@ 2020-11-10  8:13 Alexander Turenko
  2020-11-16 21:22 ` Vladislav Shpilevoy
  2020-11-17  8:38 ` Timur Safin
  0 siblings, 2 replies; 8+ messages in thread
From: Alexander Turenko @ 2020-11-10  8:13 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-10  8:13 [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths Alexander Turenko
@ 2020-11-16 21:22 ` Vladislav Shpilevoy
  2020-11-16 22:19   ` Alexander Turenko
  2020-11-17  8:38 ` Timur Safin
  1 sibling, 1 reply; 8+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-16 21:22 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

Hi! Thanks for the patch!

LGTM.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-16 21:22 ` Vladislav Shpilevoy
@ 2020-11-16 22:19   ` Alexander Turenko
  2020-11-17  8:35     ` Timur Safin
  0 siblings, 1 reply; 8+ messages in thread
From: Alexander Turenko @ 2020-11-16 22:19 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On Mon, Nov 16, 2020 at 10:22:18PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> LGTM.

Thanks!

Timur, can you do the second review?

WBR, Alexander Turenko.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-16 22:19   ` Alexander Turenko
@ 2020-11-17  8:35     ` Timur Safin
  0 siblings, 0 replies; 8+ messages in thread
From: Timur Safin @ 2020-11-17  8:35 UTC (permalink / raw)
  To: 'Alexander Turenko', 'Vladislav Shpilevoy'
  Cc: tarantool-patches

Will do it very soon!

Timur

: -----Original Message-----
: From: Alexander Turenko <alexander.turenko@tarantool.org>
: Sent: Tuesday, November 17, 2020 1:20 AM
: To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Cc: tarantool-patches@dev.tarantool.org; Timur Safin <tsafin@tarantool.org>
: Subject: Re: [PATCH] lua/merger: fix NULL dereferences on error paths
: 
: On Mon, Nov 16, 2020 at 10:22:18PM +0100, Vladislav Shpilevoy wrote:
: > Hi! Thanks for the patch!
: >
: > LGTM.
: 
: Thanks!
: 
: Timur, can you do the second review?
: 
: WBR, Alexander Turenko.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-10  8:13 [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths Alexander Turenko
  2020-11-16 21:22 ` Vladislav Shpilevoy
@ 2020-11-17  8:38 ` Timur Safin
  2020-11-17 12:25   ` Alexander Turenko
  1 sibling, 1 reply; 8+ messages in thread
From: Timur Safin @ 2020-11-17  8:38 UTC (permalink / raw)
  To: 'Alexander Turenko', 'Vladislav Shpilevoy'
  Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-17  8:38 ` Timur Safin
@ 2020-11-17 12:25   ` Alexander Turenko
  2020-11-18 12:27     ` Alexander V. Tikhonov
  0 siblings, 1 reply; 8+ messages in thread
From: Alexander Turenko @ 2020-11-17 12:25 UTC (permalink / raw)
  To: Alexander V. Tikhonov; +Cc: tarantool-patches, '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 <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
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-17 12:25   ` Alexander Turenko
@ 2020-11-18 12:27     ` Alexander V. Tikhonov
  2020-11-19  8:15       ` Alexander Turenko
  0 siblings, 1 reply; 8+ messages in thread
From: Alexander V. Tikhonov @ 2020-11-18 12:27 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
  2020-11-18 12:27     ` Alexander V. Tikhonov
@ 2020-11-19  8:15       ` Alexander Turenko
  0 siblings, 0 replies; 8+ messages in thread
From: Alexander Turenko @ 2020-11-19  8:15 UTC (permalink / raw)
  To: Alexander V. Tikhonov; +Cc: tarantool-patches, Vladislav Shpilevoy

Thanks!

Pushed to master, 2.6, 2.5.

Added changelog entries:

 | * Fixed NULL dereference on error paths in merger, usually on a
 |   'wrong' key_def (gh-5450).

WBR, Alexander Turenko.

On Wed, Nov 18, 2020 at 03:27:49PM +0300, Alexander V. Tikhonov wrote:
> 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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-11-19  8:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10  8:13 [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths Alexander Turenko
2020-11-16 21:22 ` Vladislav Shpilevoy
2020-11-16 22:19   ` Alexander Turenko
2020-11-17  8:35     ` Timur Safin
2020-11-17  8:38 ` Timur Safin
2020-11-17 12:25   ` Alexander Turenko
2020-11-18 12:27     ` Alexander V. Tikhonov
2020-11-19  8:15       ` Alexander Turenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox