Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Alexander Turenko <alexander.turenko@tarantool.org>
Subject: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths
Date: Tue, 10 Nov 2020 11:13:14 +0300	[thread overview]
Message-ID: <ed3a462eab028de11cea256c8f0efe5bb1ff2c0c.1604995527.git.alexander.turenko@tarantool.org> (raw)

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

             reply	other threads:[~2020-11-10  8:13 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-10  8:13 Alexander Turenko [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed3a462eab028de11cea256c8f0efe5bb1ff2c0c.1604995527.git.alexander.turenko@tarantool.org \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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