From f7f491b36ea60ff210f092cd7f0a63b52892dbb5 Mon Sep 17 00:00:00 2001
From: Andrey Saranchin <Andrey22102001@gmail.com>
Date: Mon, 2 Sep 2024 11:54:16 +0300
Subject: [PATCH] memtx: handle all statements related to the space on DDL
When DDL happens, we remove statements of concurrent transactions
from MVCC. When removing statements, we set their `engine_savepoint` to
`NULL` so that they won't be rolled back because we've already handled
them.
However, we remove statements only from stories, and not all statements
can be accessed in this way. For example, when we have several delete
statements of one story, and one of them gets prepared, others are
unlinked. It leads to use-after-free, but it's read-only and doesn't
affect anything, so only ASAN can catch it. It happens when the
statement is being rolled back in `memtx_engine_rollback_statement`:
we check if `space->upgrade` is not `NULL` (space can be already deleted)
but this check affects instruction flow only if `stmt->new_tuple != NULL`
and in our case that's not so. Anyway, let's iterate over all statements
of all transactions and remove savepoints for ones related to the space
that is being invalidated. It takes more time, but anyway, we are doing
DDL that is heavy, so it doesn't really matter.
Along the way, the commit removes helper `memtx_tx_history_remove_stmt`
and all its helpers because they are not needed anymore. This helper
unlinks added story from history chain, links all its delete statements
to the previous story, if any, unlinks the statement from related stories
and sets `engine_savepoint` to `NULL`. Since we already do all of this
things except for unlinking statements from stories, let's simply call
`memtx_tx_story_unlink_added[deleted]_by` instead. This change makes the
code much more straightforward.
Closes #10146
NO_DOC=bugfix
(cherry picked from commit ac112b73192ad96271a02ee85dba3e9737fdaa9d)
---
...vcc-ddl-crashes-and-isolation-violation.md | 4 +
src/box/memtx_tx.c | 129 +++---------------
test/box-luatest/mvcc_ddl_test.lua | 22 +++
3 files changed, 46 insertions(+), 109 deletions(-)
create mode 100644 changelogs/unreleased/gh-10146-mvcc-ddl-crashes-and-isolation-violation.md
diff --git a/changelogs/unreleased/gh-10146-mvcc-ddl-crashes-and-isolation-violation.md b/changelogs/unreleased/gh-10146-mvcc-ddl-crashes-and-isolation-violation.md
new file mode 100644
index 0000000000..dc0aa0b252
--- /dev/null
+++ b/changelogs/unreleased/gh-10146-mvcc-ddl-crashes-and-isolation-violation.md
@@ -0,0 +1,4 @@
+## bugfix/memtx
+
+* Fixed several bugs when DDL with MVCC enabled could lead to a crash
+ or violate isolation of other transactions (gh-10146).
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index 97b0ee2088..8c29c9432c 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -1202,53 +1202,6 @@ memtx_tx_story_link_top(struct memtx_story *new_top,
rlist_splice(&new_link->read_gaps, &old_link->read_gaps);
}
-static void
-memtx_tx_story_unlink_top_on_space_delete(struct memtx_story *story,
- uint32_t idx)
-{
- struct memtx_story_link *link = &story->link[idx];
- struct memtx_story *old_story = link->older_story;
- if (old_story != NULL)
- memtx_tx_story_unlink(story, old_story, idx);
-}
-
-/**
- * Unlink a @a story from history chain in @a index in both directions.
- * Handles case when the story is not on top of the history: simply remove from
- * list.
- */
-static void
-memtx_tx_story_unlink_both_common(struct memtx_story *story,
- uint32_t idx)
-{
- assert(idx < story->index_count);
- struct memtx_story_link *link = &story->link[idx];
- struct memtx_story *newer_story = link->newer_story;
- struct memtx_story *older_story = link->older_story;
- memtx_tx_story_unlink(newer_story, story, idx);
- memtx_tx_story_unlink(story, older_story, idx);
- memtx_tx_story_link(newer_story, older_story, idx);
-}
-
-/**
- * Unlink a @a story from history chain in @a index in both directions.
- * If the story was in the top of history chain - unlink from top. Otherwise,
- * see description of `memtx_tx_story_unlink_both_common`.
- * Since the space is being deleted, we need to simply unlink the story.
- */
-static void
-memtx_tx_story_unlink_both_on_space_delete(struct memtx_story *story,
- uint32_t idx)
-{
- assert(idx < story->index_count);
- struct memtx_story_link *link = &story->link[idx];
- if (link->newer_story == NULL) {
- memtx_tx_story_unlink_top_on_space_delete(story, idx);
- } else {
- memtx_tx_story_unlink_both_common(story, idx);
- }
-}
-
/**
* Change the order of stories in history chain.
*/
@@ -2076,28 +2029,6 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple,
result);
}
-/*
- * Relink those who delete this story and make them delete older story.
- */
-static void
-memtx_tx_history_remove_story_del_stmts(struct memtx_story *story)
-{
- struct memtx_story *old_story = story->link[0].older_story;
- while (story->del_stmt) {
- struct txn_stmt *del_stmt = story->del_stmt;
-
- /* Unlink from old list in any case. */
- story->del_stmt = del_stmt->next_in_del_list;
- del_stmt->next_in_del_list = NULL;
- del_stmt->del_story = NULL;
-
- /* Link to old story's list. */
- if (old_story != NULL)
- memtx_tx_story_link_deleted_by(old_story,
- del_stmt);
- }
-}
-
/*
* Abort with conflict all transactions that have read @a story.
*/
@@ -2332,44 +2263,6 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt)
assert(stmt->add_story == NULL && stmt->del_story == NULL);
}
-/*
- * Completely remove statement that adds a story.
- */
-static void
-memtx_tx_history_remove_added_story(struct txn_stmt *stmt)
-{
- assert(stmt->add_story != NULL);
- assert(stmt->add_story->tuple == stmt->rollback_info.new_tuple);
- struct memtx_story *story = stmt->add_story;
- memtx_tx_history_remove_story_del_stmts(story);
- for (uint32_t i = 0; i < story->index_count; i++)
- memtx_tx_story_unlink_both_on_space_delete(story, i);
- memtx_tx_story_unlink_added_by(story, stmt);
-}
-
-/*
- * Completely remove statement that deletes a story.
- */
-static void
-memtx_tx_history_remove_deleted_story(struct txn_stmt *stmt)
-{
- memtx_tx_story_unlink_deleted_by(stmt->del_story, stmt);
-}
-
-/*
- * Completely (as opposed to rollback) remove statement from history.
- */
-static void
-memtx_tx_history_remove_stmt(struct txn_stmt *stmt)
-{
- if (stmt->add_story != NULL)
- memtx_tx_history_remove_added_story(stmt);
- if (stmt->del_story != NULL)
- memtx_tx_history_remove_deleted_story(stmt);
- if (stmt->txn->status == TXN_ABORTED)
- stmt->engine_savepoint = NULL;
-}
-
/**
* Abort or send to read view readers of @a story, except the transaction
* @a writer that is actually deletes the story.
@@ -2944,9 +2837,10 @@ memtx_tx_invalidate_space(struct space *space, struct txn *active_txn)
struct memtx_story,
in_space_stories);
if (story->add_stmt != NULL)
- memtx_tx_history_remove_stmt(story->add_stmt);
+ memtx_tx_story_unlink_added_by(story, story->add_stmt);
while (story->del_stmt != NULL)
- memtx_tx_history_remove_stmt(story->del_stmt);
+ memtx_tx_story_unlink_deleted_by(story,
+ story->del_stmt);
memtx_tx_story_full_unlink_on_space_delete(story);
for (uint32_t i = 0; i < story->index_count; i++) {
struct rlist *read_gaps = &story->link[i].read_gaps;
@@ -2972,6 +2866,23 @@ memtx_tx_invalidate_space(struct space *space, struct txn *active_txn)
}
memtx_tx_story_delete(story);
}
+
+ /*
+ * Phase three: remove savepoints from all affected statements so that
+ * they won't be rolled back because we already did it. Moreover, they
+ * could access the old space that is going to be deleted leading to
+ * use-after-free.
+ */
+ struct txn *txn;
+ rlist_foreach_entry(txn, &txm.all_txs, in_all_txs) {
+ if (txn->status != TXN_ABORTED || txn->psn != 0)
+ continue;
+ struct txn_stmt *stmt;
+ stailq_foreach_entry(stmt, &txn->stmts, next) {
+ if (stmt->space == space)
+ stmt->engine_savepoint = NULL;
+ }
+ }
}
/**
diff --git a/test/box-luatest/mvcc_ddl_test.lua b/test/box-luatest/mvcc_ddl_test.lua
index 0279172c55..7f0381adfa 100644
--- a/test/box-luatest/mvcc_ddl_test.lua
+++ b/test/box-luatest/mvcc_ddl_test.lua
@@ -472,3 +472,25 @@ g.test_ddl_handles_prepared = function(cg)
end
end)
end
+
+-- The test covers the problem when already deleted space could be accessed
+-- by rollback of a deletion when another concurrent deletion was already
+-- prepared.
+-- Since the access is read-only and doesn't break anything, the test actually
+-- covers the problem only when ASAN is enabled.
+g.test_ddl_use_after_free_on_rollback_of_deletion = function(cg)
+ cg.server:exec(function()
+ local txn_proxy = require('test.box.lua.txn_proxy')
+ box.schema.space.create('test')
+ box.space.test:create_index('pk')
+ box.space.test:replace{1}
+
+ local tx = txn_proxy.new()
+ tx:begin()
+ tx('box.space.test:delete(1)')
+
+ box.space.test:delete(1)
+ box.space.test:create_index('sk')
+ tx:rollback()
+ end)
+end
--
GitLab