From 8d3f75b44071ae6c1ffb800d097b86c1f6c8bf4a Mon Sep 17 00:00:00 2001 From: Georgiy Lebedev Date: Sat, 1 Oct 2022 16:08:08 +0300 Subject: [PATCH] memtx: fix use-after-free of successor in `tree_iterator_start` We assumed that the successor tuple's story could not get garbage collected on clarify of result tuple in `tree_iterator_start`, since they coincide in case of regular iterators. But this is not the case for reverse iterators: the result tuple is of-by-one from the successor, which means the successor's story can get garbage collected along with the tuple itself getting deleted, leading to use-after-free of successor: remove garbage collection from `memtx_tx_tuple_clarify` and call it manually. The crash in #7756 revealed that the `put` in transaction manager's story hash table was performed incorrectly: fix it and add an assertion that nothing was replaced. Closes #7755 Closes #7756 NO_DOC=bugfix --- ...mtx-rev-iters-repeatable-read-violation.md | 4 ++ .../gh-7756-memtx-crash-on-series-of-txs.md | 3 + src/box/memtx_bitset.cc | 3 +- src/box/memtx_hash.cc | 12 +++- src/box/memtx_rtree.cc | 6 +- src/box/memtx_tree.cc | 13 ++-- src/box/memtx_tx.c | 7 ++- src/box/memtx_tx.h | 3 - ...v_iters_repeatable_read_violation_test.lua | 61 +++++++++++++++++++ ...7756_memtx_crash_on_series_of_txs_test.lua | 54 ++++++++++++++++ 10 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/gh-7755-memtx-rev-iters-repeatable-read-violation.md create mode 100644 changelogs/unreleased/gh-7756-memtx-crash-on-series-of-txs.md create mode 100644 test/box-luatest/gh_7755_memtx_rev_iters_repeatable_read_violation_test.lua create mode 100644 test/box-luatest/gh_7756_memtx_crash_on_series_of_txs_test.lua diff --git a/changelogs/unreleased/gh-7755-memtx-rev-iters-repeatable-read-violation.md b/changelogs/unreleased/gh-7755-memtx-rev-iters-repeatable-read-violation.md new file mode 100644 index 000000000000..cfbf1e1b03d8 --- /dev/null +++ b/changelogs/unreleased/gh-7755-memtx-rev-iters-repeatable-read-violation.md @@ -0,0 +1,4 @@ +## bugfix/memtx + +* Fixed possibility of repeatable read violation with reverse iterators + (gh-7755). diff --git a/changelogs/unreleased/gh-7756-memtx-crash-on-series-of-txs.md b/changelogs/unreleased/gh-7756-memtx-crash-on-series-of-txs.md new file mode 100644 index 000000000000..9ae1f74cf4cd --- /dev/null +++ b/changelogs/unreleased/gh-7756-memtx-crash-on-series-of-txs.md @@ -0,0 +1,3 @@ +## bugfix/memtx + +* Fixed crash on series of transactions in memtx (gh-7756). diff --git a/src/box/memtx_bitset.cc b/src/box/memtx_bitset.cc index 77cbce3c813e..8331f51a04b6 100644 --- a/src/box/memtx_bitset.cc +++ b/src/box/memtx_bitset.cc @@ -211,8 +211,9 @@ bitset_index_iterator_next(struct iterator *iterator, struct tuple **ret) struct index *idx = iterator->index; struct txn *txn = in_txn(); struct space *space = space_by_id(iterator->space_id); -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *ret = memtx_tx_tuple_clarify(txn, space, tuple, idx, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } while (*ret == NULL); diff --git a/src/box/memtx_hash.cc b/src/box/memtx_hash.cc index 23aab5584c8e..f04cac5769e5 100644 --- a/src/box/memtx_hash.cc +++ b/src/box/memtx_hash.cc @@ -152,6 +152,9 @@ name(struct iterator *iterator, struct tuple **ret) \ return rc; \ is_first = false; \ *ret = memtx_tx_tuple_clarify(txn, space, *ret, idx, 0); \ +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/\ + memtx_tx_story_gc(); \ +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/\ } while (*ret == NULL); \ return 0; \ } \ @@ -172,8 +175,9 @@ hash_iterator_eq(struct iterator *it, struct tuple **ret) return 0; struct txn *txn = in_txn(); struct space *sp = space_by_id(it->space_id); -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *ret = memtx_tx_tuple_clarify(txn, sp, *ret, it->index, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ return 0; } @@ -304,6 +308,9 @@ memtx_hash_index_random(struct index *base, uint32_t rnd, struct tuple **result) *result = light_index_get(hash_table, k); assert(*result != NULL); *result = memtx_tx_tuple_clarify(txn, space, *result, base, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); +/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } while (*result == NULL); return memtx_prepare_result_tuple(result); } @@ -334,8 +341,9 @@ memtx_hash_index_get_internal(struct index *base, const char *key, uint32_t k = light_index_find_key(&index->hash_table, h, key); if (k != light_index_end) { struct tuple *tuple = light_index_get(&index->hash_table, k); -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *result = memtx_tx_tuple_clarify(txn, space, tuple, base, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } else { /********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ diff --git a/src/box/memtx_rtree.cc b/src/box/memtx_rtree.cc index eb45eaf3eba8..1d9db44b40cc 100644 --- a/src/box/memtx_rtree.cc +++ b/src/box/memtx_rtree.cc @@ -159,8 +159,9 @@ index_rtree_iterator_next(struct iterator *i, struct tuple **ret) struct index *idx = i->index; struct txn *txn = in_txn(); struct space *space = space_by_id(i->space_id); -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *ret = memtx_tx_tuple_clarify(txn, space, *ret, idx, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } while (*ret == NULL); return 0; @@ -242,8 +243,9 @@ memtx_rtree_index_get_internal(struct index *base, const char *key, break; struct txn *txn = in_txn(); struct space *space = space_by_id(base->def->space_id); -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *result = memtx_tx_tuple_clarify(txn, space, tuple, base, 0); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } while (*result == NULL); rtree_iterator_destroy(&iterator); diff --git a/src/box/memtx_tree.cc b/src/box/memtx_tree.cc index fc9316216b26..96e6edf9fbf4 100644 --- a/src/box/memtx_tree.cc +++ b/src/box/memtx_tree.cc @@ -776,22 +776,19 @@ tree_iterator_start(struct iterator *iterator, struct tuple **ret) * We need to clarify the result tuple before story garbage * collection, otherwise it could get cleaned there. */ -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *ret = memtx_tx_tuple_clarify(txn, space, res->tuple, idx, mk_index); -/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } - if (key_is_full && !eq_match) /********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + if (key_is_full && !eq_match) memtx_tx_track_point(txn, space, idx, it->key_data.key); -/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ if (!key_is_full || ((type == ITER_GE || type == ITER_LE) && !equals) || (type == ITER_GT || type == ITER_LT)) -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ memtx_tx_track_gap(txn, space, idx, successor, type, start_data.key, start_data.part_count); + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ return res == NULL || !eq_match || *ret != NULL ? 0 : iterator->next_internal(iterator, ret); @@ -957,6 +954,9 @@ memtx_tree_index_random(struct index *base, uint32_t rnd, struct tuple **result) uint32_t mk_index = is_multikey ? (uint32_t)res->hint : 0; *result = memtx_tx_tuple_clarify(txn, space, res->tuple, base, mk_index); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); +/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ } while (*result == NULL); return memtx_prepare_result_tuple(result); } @@ -1000,9 +1000,10 @@ memtx_tree_index_get_internal(struct index *base, const char *key, } bool is_multikey = base->def->key_def->is_multikey; uint32_t mk_index = is_multikey ? (uint32_t)res->hint : 0; -/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ *result = memtx_tx_tuple_clarify(txn, space, res->tuple, base, mk_index); +/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/ + memtx_tx_story_gc(); /*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/ return 0; } diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c index 584b286ed9ba..8a6be1d2d75e 100644 --- a/src/box/memtx_tx.c +++ b/src/box/memtx_tx.c @@ -778,8 +778,10 @@ memtx_tx_story_new(struct space *space, struct tuple *tuple, const struct memtx_story **put_story = (const struct memtx_story **) &story; - struct memtx_story **empty = NULL; - mh_history_put(txm.history, put_story, &empty, 0); + struct memtx_story *replaced = NULL; + struct memtx_story **preplaced = &replaced; + mh_history_put(txm.history, put_story, &preplaced, 0); + assert(preplaced == NULL); tuple_set_flag(tuple, TUPLE_IS_DIRTY); tuple_ref(tuple); story->status = MEMTX_TX_STORY_USED; @@ -2713,7 +2715,6 @@ memtx_tx_tuple_clarify_slow(struct txn *txn, struct space *space, struct tuple *res = memtx_tx_tuple_clarify_impl(txn, space, tuple, index, mk_index, is_prepared_ok); - memtx_tx_story_gc(); return res; } diff --git a/src/box/memtx_tx.h b/src/box/memtx_tx.h index 4891b1434724..767a7fe6fe79 100644 --- a/src/box/memtx_tx.h +++ b/src/box/memtx_tx.h @@ -516,8 +516,6 @@ memtx_tx_story_gc(); /** * Clean a tuple if it's dirty - finds a visible tuple in history. * - * NB: can trigger story garbage collection. - * * @param txn - current transactions. * @param space - space in which the tuple was found. * @param tuple - tuple to clean. @@ -535,7 +533,6 @@ memtx_tx_tuple_clarify(struct txn *txn, struct space *space, return tuple; if (!tuple_has_flag(tuple, TUPLE_IS_DIRTY)) { memtx_tx_track_read(txn, space, tuple); - memtx_tx_story_gc(); return tuple; } return memtx_tx_tuple_clarify_slow(txn, space, tuple, index, mk_index); diff --git a/test/box-luatest/gh_7755_memtx_rev_iters_repeatable_read_violation_test.lua b/test/box-luatest/gh_7755_memtx_rev_iters_repeatable_read_violation_test.lua new file mode 100644 index 000000000000..a6719855ec7d --- /dev/null +++ b/test/box-luatest/gh_7755_memtx_rev_iters_repeatable_read_violation_test.lua @@ -0,0 +1,61 @@ +local server = require('test.luatest_helpers.server') +local t = require('luatest') + +local g = t.group(nil, t.helpers.matrix{iter = {'LT', 'LE'}}) + +g.before_all(function(cg) + cg.server = server:new { + alias = 'dflt', + box_cfg = {memtx_use_mvcc_engine = true} + } + cg.server:start() +end) + +g.after_all(function(cg) + cg.server:drop() +end) + +g.before_each(function(cg) + cg.server:exec(function() + local s = box.schema.create_space('s') + s:create_index('pk') + box.internal.memtx_tx_gc(1) + end) +end) + +g.after_each(function(cg) + cg.server:exec(function() + box.space.s:drop() + end) +end) + +--[[ +Checks that repeatable read violation with reverse iterators is not possible. +]] +g.test_repeatable_read_violation_with_rev_iter = function(cg) + cg.server:exec(function(iter) + local t = require('luatest') + local txn_proxy = require('test.box.lua.txn_proxy') + + local tx1 = txn_proxy:new() + local tx2 = txn_proxy:new() + local tx3 = txn_proxy:new() + + tx1('box.begin()') + tx2('box.begin()') + tx3('box.begin()') + + tx1('box.space.s:insert{3}') + tx1('box.rollback()') + + tx2('box.space.s:insert{0}') + local read_operation = 'box.space.s:select({2}, {iterator = "%s"})' + read_operation = read_operation:format(iter) + tx3(read_operation) + box.space.s:insert{1} + + t.assert_equals(tx3(read_operation), {{}}) + t.assert_equals(tx3('box.space.s:insert{3}'), + {{error = 'Transaction has been aborted by conflict'}}) + end, {cg.params.iter}) +end diff --git a/test/box-luatest/gh_7756_memtx_crash_on_series_of_txs_test.lua b/test/box-luatest/gh_7756_memtx_crash_on_series_of_txs_test.lua new file mode 100644 index 000000000000..15dbcf13012e --- /dev/null +++ b/test/box-luatest/gh_7756_memtx_crash_on_series_of_txs_test.lua @@ -0,0 +1,54 @@ +local server = require('test.luatest_helpers.server') +local t = require('luatest') + +local g = t.group(nil, t.helpers.matrix{iter = {'LT', 'LE'}}) + +g.before_all(function(cg) + cg.server = server:new { + alias = 'dflt', + box_cfg = {memtx_use_mvcc_engine = true} + } + cg.server:start() +end) + +g.after_all(function(cg) + cg.server:drop() +end) + +g.before_each(function(cg) + cg.server:exec(function() + local s = box.schema.create_space('s') + s:create_index('pk') + box.internal.memtx_tx_gc(1) + end) +end) + +g.after_each(function(cg) + cg.server:exec(function() + box.space.s:drop() + end) +end) + +--[[ +Checks that server does not crash on series of transactions from gh-7756. +]] +g.test_server_crash_on_series_of_txs = function(cg) + local stream1 = cg.server.net_box:new_stream() + local stream2 = cg.server.net_box:new_stream() + local stream3 = cg.server.net_box:new_stream() + + stream1:begin() + stream2:begin() + stream3:begin() + + stream1.space.s:insert{3} + stream1:rollback() + + stream2.space.s:insert{0} + + stream3.space.s:select({2}, {iterator = cg.params.iter}) + + cg.server:exec(function() + box.space.s:insert{1} + end) +end