[PATCH] vinyl: fix crash if iterator is used throughout DDL

[PATCH] vinyl: fix crash if iterator is used throughout DDL

[Vladimir Davydov]

vy_run_iterator doesn't take a reference to the format it uses to decode
statements loaded from disk. As a result, the format may be deleted by
DDL, leading to a use-after-free bug. Fix this by taking a reference to
the format used by an iterator.

Closes #4000
---
https://github.com/tarantool/tarantool/issues/4000
https://github.com/tarantool/tarantool/commits/dv/gh-4000-vy-fix-iterator-crash-on-ddl

 src/box/vy_run.c               |  7 +++++++
 test/vinyl/errinj_ddl.result   | 44 ++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua | 20 +++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 581f8cb0..9be89357 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -1436,6 +1436,12 @@ vy_run_iterator_open(struct vy_run_iterator *itr,
 
 	itr->search_started = false;
 	itr->search_ended = false;
+
+	/*
+	 * Make sure the format we use to create tuples won't
+	 * go away if DDL is called while the iterator is used.
+	 */
+	tuple_format_ref(format);
 }
 
 /**
@@ -1602,6 +1608,7 @@ void
 vy_run_iterator_close(struct vy_run_iterator *itr)
 {
 	vy_run_iterator_stop(itr);
+	tuple_format_unref(itr->format);
 	TRASH(itr);
 }
 
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 4f5d59cd..932132db 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -593,3 +593,47 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- gh-4000: index iterator crashes if used throughout DDL.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01)
+---
+- ok
+...
+c = fiber.channel(1)
+---
+...
+_ = fiber.create(function() c:put(s.index.sk:select()) end)
+---
+...
+s.index.sk:alter{parts = {2, 'number'}}
+---
+...
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
+---
+- ok
+...
+c:get()
+---
+- - [1, 1]
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 0948bc3d..95e0ad3c 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -258,3 +258,23 @@ fiber.sleep(0)
 s:create_index('sk', {parts = {2, 'unsigned'}})
 s:select()
 s:drop()
+
+--
+-- gh-4000: index iterator crashes if used throughout DDL.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+
+s:replace{1, 1}
+box.snapshot()
+
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01)
+c = fiber.channel(1)
+_ = fiber.create(function() c:put(s.index.sk:select()) end)
+s.index.sk:alter{parts = {2, 'number'}}
+errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
+
+c:get()
+
+s:drop()
-- 
2.11.0

Re: [PATCH] vinyl: fix crash if iterator is used throughout DDL

[Vladimir Davydov]

On Thu, Feb 14, 2019 at 04:54:46PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/02/14 16:23]:
> > vy_run_iterator doesn't take a reference to the format it uses to decode
> > statements loaded from disk. As a result, the format may be deleted by
> > DDL, leading to a use-after-free bug. Fix this by taking a reference to
> > the format used by an iterator.
> 
> Please add a comment before _ref():
> 
> /* 
>  * Please remove this kludge when proper DDL locking is
>  * implemented on transaction management level or multi-version
>  * data dictionary is in place. 
>  */
> > 
> 
> With that, oK to push.

Pushed to 2.1 and 1.10.