From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH] vinyl: disallow passing iterator to another fiber Date: Mon, 9 Jul 2018 20:10:14 +0300 Message-Id: To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: If a vinyl iterator is passed to another fiber, it may trigger a use-after-free bug, because the tx it's using may be destroyed while it's reading the disk. So let's explicitly ban that. Closes #3394 --- https://github.com/tarantool/tarantool/issues/3394 https://github.com/tarantool/tarantool/commits/dv/gh-3394-vy-disallow-passing-iter-to-another-fiber src/box/vinyl.c | 44 +++++++++++++++++-------- test/vinyl/iterator.result | 78 ++++++++++++++++++++++++++++++++++++++++++++ test/vinyl/iterator.test.lua | 33 +++++++++++++++++++ 3 files changed, 141 insertions(+), 14 deletions(-) diff --git a/src/box/vinyl.c b/src/box/vinyl.c index 10edbeda..bfe8d2a7 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -3845,22 +3845,44 @@ vinyl_iterator_close(struct vinyl_iterator *it) it->base.next = vinyl_iterator_last; } +/** + * Check if the transaction associated with the iterator is + * still valid. + */ static int -vinyl_iterator_primary_next(struct iterator *base, struct tuple **ret) +vinyl_iterator_check_tx(struct vinyl_iterator *it) { - assert(base->next = vinyl_iterator_primary_next); - struct vinyl_iterator *it = (struct vinyl_iterator *)base; - assert(it->lsm->index_id == 0); - if (it->tx == NULL) { + /* Transaction ended or cursor was closed. */ diag_set(ClientError, ER_CURSOR_NO_TRANSACTION); - goto fail; + return -1; + } + if (it->tx != &it->tx_autocommit && + it->tx != (in_txn() ? in_txn()->engine_tx : NULL)) { + /* + * Iterator was passed to another fiber. + * Abort it immediately to avoid use-after-free. + */ + diag_set(ClientError, ER_CURSOR_NO_TRANSACTION); + return -1; } if (it->tx->state == VINYL_TX_ABORT || it->tx->read_view->is_aborted) { + /* Transaction read view was aborted. */ diag_set(ClientError, ER_READ_VIEW_ABORTED); - goto fail; + return -1; } + return 0; +} + +static int +vinyl_iterator_primary_next(struct iterator *base, struct tuple **ret) +{ + assert(base->next = vinyl_iterator_primary_next); + struct vinyl_iterator *it = (struct vinyl_iterator *)base; + assert(it->lsm->index_id == 0); + if (vinyl_iterator_check_tx(it) != 0) + goto fail; if (vy_read_iterator_next(&it->iterator, ret) != 0) goto fail; if (*ret == NULL) { @@ -3884,14 +3906,8 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret) struct tuple *tuple; next: - if (it->tx == NULL) { - diag_set(ClientError, ER_CURSOR_NO_TRANSACTION); - goto fail; - } - if (it->tx->state == VINYL_TX_ABORT || it->tx->read_view->is_aborted) { - diag_set(ClientError, ER_READ_VIEW_ABORTED); + if (vinyl_iterator_check_tx(it) != 0) goto fail; - } if (vy_read_iterator_next(&it->iterator, &tuple) != 0) goto fail; diff --git a/test/vinyl/iterator.result b/test/vinyl/iterator.result index be0744d1..48f05246 100644 --- a/test/vinyl/iterator.result +++ b/test/vinyl/iterator.result @@ -8,6 +8,9 @@ env = require('test_run') test_run = env.new() --- ... +fiber = require('fiber') +--- +... create_iterator = require('utils').create_iterator --- ... @@ -2229,3 +2232,78 @@ t s:drop() --- ... +-- +-- gh-3394: vinyl iterator is aborted if passed to another transactions. +-- +s = box.schema.space.create('test', {engine = 'vinyl'}) +--- +... +_ = s:create_index('pk') +--- +... +_ = s:replace{1} +--- +... +_ = s:replace{2} +--- +... +c = fiber.channel(1) +--- +... +i = create_iterator(s) +--- +... +iterator_next(i) +--- +- [1] +... +_ = fiber.create(function() local _, v = pcall(iterator_next, i) c:put(v) end) +--- +... +c:get() -- ok as the iterator isn't bound to any transaction +--- +- [2] +... +box.begin() +--- +... +i = create_iterator(s) +--- +... +iterator_next(i) +--- +- [1] +... +_ = fiber.create(function() local _, v = pcall(iterator_next, i) c:put(v) end) +--- +... +c:get() -- error +--- +- The transaction the cursor belongs to has ended +... +box.commit() +--- +... +box.begin() +--- +... +i = create_iterator(s) +--- +... +iterator_next(i) +--- +- [1] +... +_ = fiber.create(function() box.begin() local _, v = pcall(iterator_next, i) c:put(v) box.commit() end) +--- +... +c:get() -- error +--- +- The transaction the cursor belongs to has ended +... +box.commit() +--- +... +s:drop() +--- +... diff --git a/test/vinyl/iterator.test.lua b/test/vinyl/iterator.test.lua index 9fa6609a..2e67079c 100644 --- a/test/vinyl/iterator.test.lua +++ b/test/vinyl/iterator.test.lua @@ -6,6 +6,8 @@ env = require('test_run') test_run = env.new() +fiber = require('fiber') + create_iterator = require('utils').create_iterator iterator_next = function(iter) return iter.next() end iterate_over = function(iter) return iter.iterate_over() end @@ -781,3 +783,34 @@ for k, v in gen, param, state do table.insert(t, v) end t s:drop() + +-- +-- gh-3394: vinyl iterator is aborted if passed to another transactions. +-- +s = box.schema.space.create('test', {engine = 'vinyl'}) +_ = s:create_index('pk') +_ = s:replace{1} +_ = s:replace{2} + +c = fiber.channel(1) + +i = create_iterator(s) +iterator_next(i) +_ = fiber.create(function() local _, v = pcall(iterator_next, i) c:put(v) end) +c:get() -- ok as the iterator isn't bound to any transaction + +box.begin() +i = create_iterator(s) +iterator_next(i) +_ = fiber.create(function() local _, v = pcall(iterator_next, i) c:put(v) end) +c:get() -- error +box.commit() + +box.begin() +i = create_iterator(s) +iterator_next(i) +_ = fiber.create(function() box.begin() local _, v = pcall(iterator_next, i) c:put(v) box.commit() end) +c:get() -- error +box.commit() + +s:drop() -- 2.11.0