[Tarantool-patches] [PATCH v4 0/2] vinyl: fix uninitialized memory accesses

[Tarantool-patches] [PATCH v4 0/2] vinyl: fix uninitialized memory accesses

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/commits/np/gh-4864-access-to-uninit-mem
Issue: https://github.com/tarantool/tarantool/issues/4864

Changes in v2:
 - replaced error injection ERRINJ_VY_MAX_TUPLE_SIZE with
ERRINJ_VY_STMT_ALLOC (i.e. now vy_stmt_alloc() fails not due to exceed
max size, but owing to allocation failure);
 - found another one use-after-free bug in case vy_read_view_merge()
fails. Fix is merged into second patch;
 - added ERRINJ_VY_READ_VIEW_MERGE_FAIL to provide test case in
case of vy_read_view_merge() failure;
 - fixed test covering second bug: error injection value accidentally
was set to a wrong value (bad copy-paste).

Changes in v3:
 - instead of nullifing read views (which may lead to tuple leaks), let's
call proper finalizing routine vy_read_view_stmt_destroy();
 - found another one possible crash due to extra tuple format unref
in case of failed compaction; fix to that is introduced in third patch
in series.

Changes in v4:
 - removed write history clean up from vy_read_view_stmt_destroy().
Instead write history now is destroyed right in
vy_write_iterator_build_read_view() when it is no longer needed;
 - moved clean-up of all write histories which belong to particular
write iterator to one function vy_write_iterator_history_destroy()
which unrefs corresponding tuples and releases region memory;
 - refactored test so that it does not rely on unchecked sleeps;
 - separated and pushed first patch in series.

@ChangeLog:
* Fixed crash during compaction due to tuples with size exceeding
vinyl_max_tuple_size setting (gh-4864).

Nikita Pettik (2):
  vinyl: clean-up unprocessed read views in *_build_read_views()
  vinyl: clean-up write iterator if vy_task_write_run() fails

 src/box/vy_write_iterator.c                   |  77 +++++--
 src/errinj.h                                  |   2 +
 test/box/errinj.result                        |   2 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 205 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  |  93 ++++++++
 5 files changed, 364 insertions(+), 15 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v4 1/2] vinyl: clean-up unprocessed read views in *_build_read_views()

[Nikita Pettik]

vy_write_iterator->read_views[i].history objects are allocated on
region (see vy_write_iterator_push_rv()) during building history of the
given key. However, in case of fail of vy_write_iterator_build_history()
region is truncated but pointers to vy_write_history objects are not
nullified. As a result, they may be accessed (for instance while
finalizing write_iterator object in  vy_write_iterator_stop) which in
turn may lead to crash, segfaul or disk formatting. The same may happen
if vy_read_view_merge() fails during processing of read view array.
Let's clean-up those objects in case of error takes place.

Part of #4864
---
 src/box/vy_write_iterator.c                   |  61 +++++++--
 src/errinj.h                                  |   1 +
 test/box/errinj.result                        |   1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 125 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  |  53 ++++++++
 5 files changed, 227 insertions(+), 14 deletions(-)

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 7a6a20627..7784dd13a 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -151,9 +151,11 @@ vy_read_view_stmt_destroy(struct vy_read_view_stmt *rv)
 	if (rv->tuple != NULL)
 		vy_stmt_unref_if_possible(rv->tuple);
 	rv->tuple = NULL;
-	if (rv->history != NULL)
-		vy_write_history_destroy(rv->history);
-	rv->history = NULL;
+	/*
+	 * History must be already cleaned up in
+	 * vy_write_iterator_build_read_views().
+	 */
+	assert(rv->history == NULL);
 }
 
 /* @sa vy_write_iterator.h */
@@ -790,8 +792,7 @@ next_lsn:
 	 * statement around if this is major compaction, because
 	 * there's no tuple it could overwrite.
 	 */
-	if (rc == 0 && stream->is_last_level &&
-	    stream->deferred_delete_stmt != NULL) {
+	if (stream->is_last_level && stream->deferred_delete_stmt != NULL) {
 		vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
 		stream->deferred_delete_stmt = NULL;
 	}
@@ -834,6 +835,15 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
 		rv->history = NULL;
 		return 0;
 	}
+#ifndef NDEBUG
+	struct errinj *inj =
+		errinj(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL);
+	if (inj != NULL && inj->bparam) {
+		inj->bparam = false;
+		diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt");
+		return -1;
+	}
+#endif
 	/*
 	 * Two possible hints to remove the current UPSERT.
 	 * 1. If the stream is working on the last level, we
@@ -940,6 +950,25 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
 	return 0;
 }
 
+/**
+ * Clean up all histories related to given write iterator.
+ * Particular history is allocated using region, so single
+ * region truncation is enough to release all memory at once.
+ * Before that we should also unref tuples stored in those
+ * histories (which is done in vy_write_history_destroy()).
+ */
+static void
+vy_write_iterator_history_destroy(struct vy_write_iterator *stream,
+				  struct region *region, size_t used)
+{
+	for (int i = 0; i < stream->rv_count; ++i) {
+		if (stream->read_views[i].history != NULL)
+			vy_write_history_destroy(stream->read_views[i].history);
+		stream->read_views[i].history = NULL;
+	}
+	region_truncate(region, used);
+}
+
 /**
  * Split the current key into a sequence of read view
  * statements. @sa struct vy_write_iterator comment for details
@@ -960,9 +989,12 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 	struct region *region = &fiber()->gc;
 	size_t used = region_used(region);
 	stream->rv_used_count = 0;
+	int rc = 0;
 	if (vy_write_iterator_build_history(stream, &raw_count,
-					    &is_first_insert) != 0)
-		goto error;
+					    &is_first_insert) != 0) {
+		rc = -1;
+		goto cleanup;
+	}
 	if (raw_count == 0) {
 		/* A key is fully optimized. */
 		region_truncate(region, used);
@@ -983,8 +1015,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 		if (rv->history == NULL)
 			continue;
 		if (vy_read_view_merge(stream, hint, rv,
-				       is_first_insert) != 0)
-			goto error;
+				       is_first_insert) != 0) {
+			rc = -1;
+			goto cleanup;
+		}
 		assert(rv->history == NULL);
 		if (rv->tuple == NULL)
 			continue;
@@ -992,11 +1026,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 		++*count;
 		hint = rv->tuple;
 	}
-	region_truncate(region, used);
-	return 0;
-error:
-	region_truncate(region, used);
-	return -1;
+
+cleanup:
+	vy_write_iterator_history_destroy(stream, region, used);
+	return rc;
 }
 
 /**
diff --git a/src/errinj.h b/src/errinj.h
index 383dafcb5..b7550bb5e 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -128,6 +128,7 @@ struct errinj {
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
 	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
+	_(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL, {.bparam = false})\
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index efbb4e85e..e1b9fbe2a 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -68,6 +68,7 @@ evals
   - ERRINJ_VY_READ_PAGE: false
   - ERRINJ_VY_READ_PAGE_DELAY: false
   - ERRINJ_VY_READ_PAGE_TIMEOUT: 0
+  - ERRINJ_VY_READ_VIEW_MERGE_FAIL: false
   - ERRINJ_VY_RUN_DISCARD: false
   - ERRINJ_VY_RUN_FILE_RENAME: false
   - ERRINJ_VY_RUN_WRITE: false
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
index 1afc02bef..af116a4b4 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -121,3 +121,128 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
 s:drop()
  | ---
  | ...
+
+-- All the same except for delayed vy_stmt_alloc() fail.
+-- Re-create space for the sake of test purity.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+
+compact()
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
+ | ---
+ | - ok
+ | ...
+-- Compaction of first range fails, so it is re-scheduled and
+-- then successfully finishes at the second attempt.
+--
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 2)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+ | ---
+ | - ok
+ | ...
+-- Unthrottle scheduler to allow next dump.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
+ | ---
+ | - ok
+ | ...
+
+s:drop()
+ | ---
+ | ...
+
+-- Once again but test that clean-up is made in case
+-- vy_read_view_merge() fails.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+
+compact()
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
+ | ---
+ | - ok
+ | ...
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 2)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
+ | ---
+ | - ok
+ | ...
+s:drop()
+ | ---
+ | ...
+
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
+ | ---
+ | - ok
+ | ...
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
index bf70bdf75..a68c73d32 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -53,3 +53,56 @@ assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
 errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
 
 s:drop()
+
+-- All the same except for delayed vy_stmt_alloc() fail.
+-- Re-create space for the sake of test purity.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+dump(true)
+dump()
+
+compact()
+
+dump()
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
+-- Compaction of first range fails, so it is re-scheduled and
+-- then successfully finishes at the second attempt.
+--
+compact()
+assert(s.index.pk:stat().range_count == 2)
+assert(s.index.pk:stat().run_count == 2)
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+-- Unthrottle scheduler to allow next dump.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
+
+s:drop()
+
+-- Once again but test that clean-up is made in case
+-- vy_read_view_merge() fails.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+dump(true)
+dump()
+
+compact()
+
+dump()
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
+compact()
+assert(s.index.pk:stat().range_count == 2)
+assert(s.index.pk:stat().run_count == 2)
+assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
+s:drop()
+
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
-- 
2.17.1

[Tarantool-patches] [PATCH v4 2/2] vinyl: clean-up write iterator if vy_task_write_run() fails

[Nikita Pettik]

In vy_task_write_run if write_iterator->start() fails, there's no call
of corresponding stop() method. vy_task_write_run() is executed in
auxiliary thread (dump or compaction). Meanwhile, creating/destroying
tuples in these threads does not change reference counter of corresponding
tuple formats (see vy_tuple_delete() and vy_stmt_alloc()).
Without cleaning up write iterator right in vy_task_write_run() after
fail, this procedure takes place in vy_task_compaction_abort() and
vy_task_dump_abort(). These *_abort() functions in turn are executed in
the main thread. Taking this into consideration, tuple may be allocated
in aux. thread and deleted in the main thread. As a result, format
reference counter decreases, whereas it shouldn't change (otherwise
tuple format will be destroyed before all tuples of this format are gone).

Real example of the bug described above can be achieved in the following
way:
1. run compaction process;
2. add one or more slice sources in vy_write_iterator_start():
corresponding slice_stream structures obtain newly created tuples
in vy_slice_stream_next();
3. the next call of vy_write_iterator_add_src() fails due to OOM,
invalid run file or whatever;
4. since there's no clean-up of tuples in slice streams, they are
destroyed in vy_task_compaction_abort() in the main thread exit;
5. now format reference counter is less than it was before compaction.

Closes #4864
---
 src/box/vy_write_iterator.c                   | 16 +++-
 src/errinj.h                                  |  1 +
 test/box/errinj.result                        |  1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 80 +++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 40 ++++++++++
 5 files changed, 137 insertions(+), 1 deletion(-)

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 7784dd13a..33ad5ed51 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -401,9 +401,23 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
 	struct vy_write_src *src, *tmp;
 	rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
 		if (vy_write_iterator_add_src(stream, src) != 0)
-			return -1;
+			goto fail;
+#ifndef NDEBUG
+		struct errinj *inj =
+			errinj(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL);
+		if (inj != NULL && inj->bparam) {
+			inj->bparam = false;
+			diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt");
+			goto fail;
+		}
+#endif
 	}
 	return 0;
+fail:
+	/* Clean-up all previously added sources. */
+	rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp)
+		vy_write_iterator_delete_src(stream, src);
+	return -1;
 }
 
 /**
diff --git a/src/errinj.h b/src/errinj.h
index b7550bb5e..8562aab1c 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -129,6 +129,7 @@ struct errinj {
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
 	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
 	_(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL, {.bparam = false})\
+	_(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL, {.bparam = false})\
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index e1b9fbe2a..2a87b5f33 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -78,6 +78,7 @@ evals
   - ERRINJ_VY_SQUASH_TIMEOUT: 0
   - ERRINJ_VY_STMT_ALLOC: -1
   - ERRINJ_VY_TASK_COMPLETE: false
+  - ERRINJ_VY_WRITE_ITERATOR_START_FAIL: false
   - ERRINJ_WAL_BREAK_LSN: -1
   - ERRINJ_WAL_DELAY: false
   - ERRINJ_WAL_FALLOCATE: 0
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
index af116a4b4..133afab1c 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -242,6 +242,86 @@ s:drop()
  | ---
  | ...
 
+-- Make sure that there's no extra format unref due to tuple
+-- clean-up in the main thread. To achieve this let's sabotage
+-- compaction process and delete all tuples: in case ref/unref
+-- is the same, format will be deleted alongside with the last
+-- tuple.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+
+compact()
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
+ | ---
+ | - ok
+ | ...
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
+ | ---
+ | - ok
+ | ...
+tasks_completed = box.stat.vinyl().scheduler.tasks_completed
+ | ---
+ | ...
+s.index.pk:compact()
+ | ---
+ | ...
+-- Tuple clean-up takes place after compaction is completed.
+-- Meanwhile range count is updated during compaction process.
+-- So instead of relying on range/run match, let's check explicitly
+-- number of completed tasks.
+--
+repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1
+ | ---
+ | ...
+
+-- Drop is required to unref all tuples.
+--
+s:drop()
+ | ---
+ | ...
+-- After index is dropped, not all tuples are deallocated at once:
+-- they may be still referenced (while being pushed) in Lua. So
+-- invoke GC explicitly.
+--
+_ = collectgarbage("collect")
+ | ---
+ | ...
+
+assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false)
+ | ---
+ | - ok
+ | ...
 errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
  | ---
  | - ok
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
index a68c73d32..547ab628e 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -105,4 +105,44 @@ assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
 errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
 s:drop()
 
+-- Make sure that there's no extra format unref due to tuple
+-- clean-up in the main thread. To achieve this let's sabotage
+-- compaction process and delete all tuples: in case ref/unref
+-- is the same, format will be deleted alongside with the last
+-- tuple.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+dump(true)
+dump()
+
+compact()
+
+dump()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
+
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
+tasks_completed = box.stat.vinyl().scheduler.tasks_completed
+s.index.pk:compact()
+-- Tuple clean-up takes place after compaction is completed.
+-- Meanwhile range count is updated during compaction process.
+-- So instead of relying on range/run match, let's check explicitly
+-- number of completed tasks.
+--
+repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1
+
+-- Drop is required to unref all tuples.
+--
+s:drop()
+-- After index is dropped, not all tuples are deallocated at once:
+-- they may be still referenced (while being pushed) in Lua. So
+-- invoke GC explicitly.
+--
+_ = collectgarbage("collect")
+
+assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false)
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false)
 errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v4 0/2] vinyl: fix uninitialized memory accesses

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

LGTM.

Re: [Tarantool-patches] [PATCH v4 0/2] vinyl: fix uninitialized memory accesses

[Nikita Pettik]

On 10 May 21:49, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> LGTM.

I found that test occasionally fails. For instance:
https://gitlab.com/tarantool/tarantool/-/jobs/548447188

So I have to push fix below to make it stable. In a nutshell we
can't rely on range/count correlation if we want to avoid any
possible races: ranges/runs are updated during compaction,
meanwhile in fact we should wait till compaction is completed
(since due to fails/error some results may turn out to be rollbacked).

It seems pretty straightforward, so hope you don't mind this fix.

diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
index 547ab628e..4b3c55505 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -14,12 +14,18 @@ function dump(big)
     box.snapshot()
 end;
 
-function compact()
+-- Tuple clean-up takes place after compaction is completed.
+-- Meanwhile range count is updated during compaction process.
+-- So instead of relying on range/run match, let's check explicitly
+-- number of completed tasks.
+--
+function compact(tasks_expected)
+    local scheduler = box.stat.vinyl().scheduler
+    local tasks_completed = scheduler.tasks_completed
     s.index.pk:compact()
     repeat
         fiber.sleep(0.001)
-        local info = s.index.pk:stat()
-    until info.range_count == info.run_count
+    until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + tasks_expected
 end;
 test_run:cmd("setopt delimiter ''");
 
@@ -32,7 +38,7 @@ dump()
 assert(s.index.pk:stat().range_count == 1)
 assert(s.index.pk:stat().run_count == 2)
 
-compact()
+compact(1)
 assert(s.index.pk:stat().range_count == 1)
 assert(s.index.pk:stat().run_count == 1)
 
@@ -46,7 +52,7 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
 -- Still split_range() fails, as a result we get one range
 -- instead two.
 --
-compact()
+compact(1)
 assert(s.index.pk:stat().range_count == 1)
 assert(s.index.pk:stat().run_count == 1)
 assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
@@ -63,7 +69,7 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
 dump(true)
 dump()
 
-compact()
+compact(1)
 
 dump()
 
@@ -72,7 +78,7 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
 -- Compaction of first range fails, so it is re-scheduled and
 -- then successfully finishes at the second attempt.
 --
-compact()
+compact(2)
 assert(s.index.pk:stat().range_count == 2)
 assert(s.index.pk:stat().run_count == 2)
 assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
@@ -92,13 +98,13 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
 dump(true)
 dump()
 
-compact()
+compact(1)
 
 dump()
 
 errinj = box.error.injection
 errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
-compact()
+compact(2)
 assert(s.index.pk:stat().range_count == 2)
 assert(s.index.pk:stat().run_count == 2)
 assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
@@ -117,7 +123,7 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
 dump(true)
 dump()
 
-compact()
+compact(1)
 
 dump()
 assert(s.index.pk:stat().range_count == 1)
@@ -125,14 +131,7 @@ assert(s.index.pk:stat().run_count == 2)
 
 errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
 errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
-tasks_completed = box.stat.vinyl().scheduler.tasks_completed
-s.index.pk:compact()
--- Tuple clean-up takes place after compaction is completed.
--- Meanwhile range count is updated during compaction process.
--- So instead of relying on range/run match, let's check explicitly
--- number of completed tasks.
---
-repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1
+compact(2)

Re: [Tarantool-patches] [PATCH v4 0/2] vinyl: fix uninitialized memory accesses

[Nikita Pettik]

On 12 May 14:14, Nikita Pettik wrote:
> On 10 May 21:49, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patchset!
> > 
> > LGTM.
> 
> I found that test occasionally fails. For instance:
> https://gitlab.com/tarantool/tarantool/-/jobs/548447188
> 
> So I have to push fix below to make it stable. In a nutshell we
> can't rely on range/count correlation if we want to avoid any
> possible races: ranges/runs are updated during compaction,
> meanwhile in fact we should wait till compaction is completed
> (since due to fails/error some results may turn out to be rollbacked).
> 
> It seems pretty straightforward, so hope you don't mind this fix.

Pushed to 1.10 both patches. Pushed to 2.3, 2.4 and master first
patch and a test extracted from second patch (since problem is
already fixed on master branch via 2f17c92). Updated changelogs
correspondingly. Branch is dropped. Before that I've verified that
CI statuses are OK.
 
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> index 547ab628e..4b3c55505 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> @@ -14,12 +14,18 @@ function dump(big)
>      box.snapshot()
>  end;
>  
> -function compact()
> +-- Tuple clean-up takes place after compaction is completed.
> +-- Meanwhile range count is updated during compaction process.
> +-- So instead of relying on range/run match, let's check explicitly
> +-- number of completed tasks.
> +--
> +function compact(tasks_expected)
> +    local scheduler = box.stat.vinyl().scheduler
> +    local tasks_completed = scheduler.tasks_completed
>      s.index.pk:compact()
>      repeat
>          fiber.sleep(0.001)
> -        local info = s.index.pk:stat()
> -    until info.range_count == info.run_count
> +    until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + tasks_expected
>  end;
>  test_run:cmd("setopt delimiter ''");
>  
> @@ -32,7 +38,7 @@ dump()
>  assert(s.index.pk:stat().range_count == 1)
>  assert(s.index.pk:stat().run_count == 2)
>  
> -compact()
> +compact(1)
>  assert(s.index.pk:stat().range_count == 1)
>  assert(s.index.pk:stat().run_count == 1)
>  
> @@ -46,7 +52,7 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
>  -- Still split_range() fails, as a result we get one range
>  -- instead two.
>  --
> -compact()
> +compact(1)
>  assert(s.index.pk:stat().range_count == 1)
>  assert(s.index.pk:stat().run_count == 1)
>  assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> @@ -63,7 +69,7 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
>  dump(true)
>  dump()
>  
> -compact()
> +compact(1)
>  
>  dump()
>  
> @@ -72,7 +78,7 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
>  -- Compaction of first range fails, so it is re-scheduled and
>  -- then successfully finishes at the second attempt.
>  --
> -compact()
> +compact(2)
>  assert(s.index.pk:stat().range_count == 2)
>  assert(s.index.pk:stat().run_count == 2)
>  assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> @@ -92,13 +98,13 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
>  dump(true)
>  dump()
>  
> -compact()
> +compact(1)
>  
>  dump()
>  
>  errinj = box.error.injection
>  errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
> -compact()
> +compact(2)
>  assert(s.index.pk:stat().range_count == 2)
>  assert(s.index.pk:stat().run_count == 2)
>  assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
> @@ -117,7 +123,7 @@ _ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size
>  dump(true)
>  dump()
>  
> -compact()
> +compact(1)
>  
>  dump()
>  assert(s.index.pk:stat().range_count == 1)
> @@ -125,14 +131,7 @@ assert(s.index.pk:stat().run_count == 2)
>  
>  errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
>  errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
> -tasks_completed = box.stat.vinyl().scheduler.tasks_completed
> -s.index.pk:compact()
> --- Tuple clean-up takes place after compaction is completed.
> --- Meanwhile range count is updated during compaction process.
> --- So instead of relying on range/run match, let's check explicitly
> --- number of completed tasks.
> ---
> -repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1
> +compact(2)
> 
>