Skip to content

Commit

Permalink
memtx: fix use-after-free of successor in tree_iterator_start
Browse files Browse the repository at this point in the history
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 tarantool#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 tarantool#7755
Closes tarantool#7756

NO_DOC=bugfix
  • Loading branch information
CuriousGeorgiy committed Oct 1, 2022
1 parent 8dcefeb commit 1344779
Show file tree
Hide file tree
Showing 10 changed files with 150 additions and 18 deletions.
@@ -0,0 +1,4 @@
## bugfix/memtx

* Fixed possibility of repeatable read violation with reverse iterators
(gh-7755).
3 changes: 3 additions & 0 deletions 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).
3 changes: 2 additions & 1 deletion src/box/memtx_bitset.cc
Expand Up @@ -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);

Expand Down
9 changes: 7 additions & 2 deletions src/box/memtx_hash.cc
Expand Up @@ -171,8 +171,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;
}
Expand Down Expand Up @@ -303,6 +304,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);
}
Expand Down Expand Up @@ -333,8 +337,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*********/
Expand Down
6 changes: 4 additions & 2 deletions src/box/memtx_rtree.cc
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
18 changes: 11 additions & 7 deletions src/box/memtx_tree.cc
Expand Up @@ -429,8 +429,8 @@ tree_iterator_next_base(struct iterator *iterator, struct tuple **ret)
*/
struct tuple *successor = res != NULL ? res->tuple : NULL;
memtx_tx_track_gap(in_txn(), space, idx, successor, ITER_GE, NULL, 0);
memtx_tx_story_gc();
/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/

return 0;
}

Expand Down Expand Up @@ -474,6 +474,7 @@ tree_iterator_prev_base(struct iterator *iterator, struct tuple **ret)
* two tuples must lead to conflict.
*/
memtx_tx_track_gap(in_txn(), space, idx, successor, ITER_LE, NULL, 0);
memtx_tx_story_gc();
/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/

tuple_unref(successor);
Expand Down Expand Up @@ -533,6 +534,7 @@ tree_iterator_next_equal_base(struct iterator *iterator, struct tuple **ret)
*/
memtx_tx_track_gap(in_txn(), space, idx, res->tuple, ITER_GE,
NULL, 0);
memtx_tx_story_gc();
/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/
}

Expand Down Expand Up @@ -594,6 +596,7 @@ tree_iterator_prev_equal_base(struct iterator *iterator, struct tuple **ret)
*/
memtx_tx_track_gap(in_txn(), space, idx, successor, ITER_LE,
NULL, 0);
memtx_tx_story_gc();
/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/
}
tuple_unref(successor);
Expand Down Expand Up @@ -775,22 +778,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);
Expand Down Expand Up @@ -956,6 +956,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);
}
Expand Down Expand Up @@ -999,9 +1002,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;
}
Expand Down
7 changes: 4 additions & 3 deletions src/box/memtx_tx.c
Expand Up @@ -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;
Expand Down Expand Up @@ -2633,7 +2635,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;
}

Expand Down
3 changes: 0 additions & 3 deletions src/box/memtx_tx.h
Expand Up @@ -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.
Expand All @@ -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);
Expand Down
@@ -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
54 changes: 54 additions & 0 deletions 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

0 comments on commit 1344779

Please sign in to comment.