From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 442EB469710 for ; Wed, 6 May 2020 12:04:50 +0300 (MSK) References: <1094177fe18b811b8b6f49f03e2ced30e798b95b.1587948306.git.korablev@tarantool.org> From: Aleksandr Lyapunov Message-ID: Date: Wed, 6 May 2020 12:04:48 +0300 MIME-Version: 1.0 In-Reply-To: <1094177fe18b811b8b6f49f03e2ced30e798b95b.1587948306.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org Nice catch! Jumps over initialization must be prohibited. As C++ prohibits. On 4/27/20 3:52 AM, Nikita Pettik wrote: > If vy_key_from_msgpack() fails in vy_lsm_split_range(), clean-up > procedure is called. However, at this moment struct vy_range *parts[2] > is not initialized ergo contains garbage and access to this structure > may result in crash, segfault or disk formatting. Let's move > initialization of mentioned variables before call of > vy_lsm_split_range(). > > Part of #4864 > --- > src/box/vy_lsm.c | 4 +- > src/box/vy_stmt.c | 10 ++ > src/errinj.h | 1 + > test/box/errinj.result | 1 + > .../gh-4864-stmt-alloc-fail-compact.result | 123 ++++++++++++++++++ > .../gh-4864-stmt-alloc-fail-compact.test.lua | 55 ++++++++ > test/vinyl/suite.ini | 2 +- > 7 files changed, 193 insertions(+), 3 deletions(-) > create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.result > create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua > > diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c > index 3d3f41b7a..04c9926a8 100644 > --- a/src/box/vy_lsm.c > +++ b/src/box/vy_lsm.c > @@ -1069,7 +1069,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range) > > /* Split a range in two parts. */ > const int n_parts = 2; > - > + struct vy_range *parts[2] = { NULL, NULL }; > /* > * Determine new ranges' boundaries. > */ > @@ -1088,7 +1088,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range) > * the old range's runs for them. > */ > struct vy_slice *slice, *new_slice; > - struct vy_range *part, *parts[2] = {NULL, }; > + struct vy_range *part = NULL; > for (int i = 0; i < n_parts; i++) { > part = vy_range_new(vy_log_next_id(), keys[i], keys[i + 1], > lsm->cmp_def); > diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c > index 9b7c55516..fafe6e56f 100644 > --- a/src/box/vy_stmt.c > +++ b/src/box/vy_stmt.c > @@ -140,6 +140,16 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize) > error_log(diag_last_error(diag_get())); > return NULL; > } > +#ifndef NDEBUG > + struct errinj *inj = errinj(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT); > + if (inj != NULL && inj->iparam >= 0) { > + if (inj->iparam-- == 0) { > + diag_set(OutOfMemory, total_size, "malloc", > + "struct vy_stmt"); > + return NULL; > + } > + } > +#endif > struct tuple *tuple = malloc(total_size); > if (unlikely(tuple == NULL)) { > diag_set(OutOfMemory, total_size, "malloc", "struct vy_stmt"); > diff --git a/src/errinj.h b/src/errinj.h > index 2cb090b68..383dafcb5 100644 > --- a/src/errinj.h > +++ b/src/errinj.h > @@ -127,6 +127,7 @@ struct errinj { > _(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \ > _(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \ > _(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\ > + _(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\ > > ENUM0(errinj_id, ERRINJ_LIST); > extern struct errinj errinjs[]; > diff --git a/test/box/errinj.result b/test/box/errinj.result > index 8090deedc..efbb4e85e 100644 > --- a/test/box/errinj.result > +++ b/test/box/errinj.result > @@ -75,6 +75,7 @@ evals > - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT: 0 > - ERRINJ_VY_SCHED_TIMEOUT: 0 > - ERRINJ_VY_SQUASH_TIMEOUT: 0 > + - ERRINJ_VY_STMT_ALLOC: -1 > - ERRINJ_VY_TASK_COMPLETE: false > - ERRINJ_WAL_BREAK_LSN: -1 > - ERRINJ_WAL_DELAY: false > diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result > new file mode 100644 > index 000000000..1afc02bef > --- /dev/null > +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result > @@ -0,0 +1,123 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +fiber = require('fiber') > + | --- > + | ... > +digest = require('digest') > + | --- > + | ... > + > +s = box.schema.space.create('test', {engine = 'vinyl'}) > + | --- > + | ... > +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024}) > + | --- > + | ... > + > +test_run:cmd("setopt delimiter ';'") > + | --- > + | - true > + | ... > +function dump(big) > + local step = big and 1 or 5 > + for i = 1, 20, step do > + s:replace{i, digest.urandom(1000)} > + end > + box.snapshot() > +end; > + | --- > + | ... > + > +function compact() > + s.index.pk:compact() > + repeat > + fiber.sleep(0.001) > + local info = s.index.pk:stat() > + until info.range_count == info.run_count > +end; > + | --- > + | ... > +test_run:cmd("setopt delimiter ''"); > + | --- > + | - true > + | ... > + > +-- The first run should be big enough to prevent major compaction > +-- on the next dump, because run_count_per_level is ignored on the > +-- last level. > +-- > +dump(true) > + | --- > + | ... > +dump() > + | --- > + | ... > +assert(s.index.pk:stat().range_count == 1) > + | --- > + | - true > + | ... > +assert(s.index.pk:stat().run_count == 2) > + | --- > + | - true > + | ... > + > +compact() > + | --- > + | ... > +assert(s.index.pk:stat().range_count == 1) > + | --- > + | - true > + | ... > +assert(s.index.pk:stat().run_count == 1) > + | --- > + | - true > + | ... > + > +dump() > + | --- > + | ... > +assert(s.index.pk:stat().range_count == 1) > + | --- > + | - true > + | ... > +assert(s.index.pk:stat().run_count == 2) > + | --- > + | - true > + | ... > + > +errinj = box.error.injection > + | --- > + | ... > +errinj.set('ERRINJ_VY_STMT_ALLOC', 0) > + | --- > + | - ok > + | ... > +-- Should finish successfully despite vy_stmt_alloc() failure. > +-- Still split_range() fails, as a result we get one range > +-- instead two. > +-- > +compact() > + | --- > + | ... > +assert(s.index.pk:stat().range_count == 1) > + | --- > + | - true > + | ... > +assert(s.index.pk:stat().run_count == 1) > + | --- > + | - true > + | ... > +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1) > + | --- > + | - true > + | ... > +errinj.set('ERRINJ_VY_STMT_ALLOC', -1) > + | --- > + | - ok > + | ... > + > +s:drop() > + | --- > + | ... > diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua > new file mode 100644 > index 000000000..bf70bdf75 > --- /dev/null > +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua > @@ -0,0 +1,55 @@ > +test_run = require('test_run').new() > +fiber = require('fiber') > +digest = require('digest') > + > +s = box.schema.space.create('test', {engine = 'vinyl'}) > +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024}) > + > +test_run:cmd("setopt delimiter ';'") > +function dump(big) > + local step = big and 1 or 5 > + for i = 1, 20, step do > + s:replace{i, digest.urandom(1000)} > + end > + box.snapshot() > +end; > + > +function compact() > + s.index.pk:compact() > + repeat > + fiber.sleep(0.001) > + local info = s.index.pk:stat() > + until info.range_count == info.run_count > +end; > +test_run:cmd("setopt delimiter ''"); > + > +-- The first run should be big enough to prevent major compaction > +-- on the next dump, because run_count_per_level is ignored on the > +-- last level. > +-- > +dump(true) > +dump() > +assert(s.index.pk:stat().range_count == 1) > +assert(s.index.pk:stat().run_count == 2) > + > +compact() > +assert(s.index.pk:stat().range_count == 1) > +assert(s.index.pk:stat().run_count == 1) > + > +dump() > +assert(s.index.pk:stat().range_count == 1) > +assert(s.index.pk:stat().run_count == 2) > + > +errinj = box.error.injection > +errinj.set('ERRINJ_VY_STMT_ALLOC', 0) > +-- Should finish successfully despite vy_stmt_alloc() failure. > +-- Still split_range() fails, as a result we get one range > +-- instead two. > +-- > +compact() > +assert(s.index.pk:stat().range_count == 1) > +assert(s.index.pk:stat().run_count == 1) > +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1) > +errinj.set('ERRINJ_VY_STMT_ALLOC', -1) > + > +s:drop() > diff --git a/test/vinyl/suite.ini b/test/vinyl/suite.ini > index 1417d7156..ed602bb64 100644 > --- a/test/vinyl/suite.ini > +++ b/test/vinyl/suite.ini > @@ -2,7 +2,7 @@ > core = tarantool > description = vinyl integration tests > script = vinyl.lua > -release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua > +release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua gh-4864-stmt-alloc-fail-compact.test.lua > config = suite.cfg > lua_libs = suite.lua stress.lua large.lua txn_proxy.lua ../box/lua/utils.lua > use_unix_sockets = True