From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DC63B469719 for ; Tue, 10 Nov 2020 11:13:19 +0300 (MSK) From: Alexander Turenko Date: Tue, 10 Nov 2020 11:13:14 +0300 Message-Id: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH] lua/merger: fix NULL dereferences on error paths List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org, 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