From de80e0264f7deb58ea86ef85b37b92653a803430 Mon Sep 17 00:00:00 2001
From: Andrey Saranchin <Andrey22102001@gmail.com>
Date: Mon, 25 Mar 2024 13:34:30 +0300
Subject: [PATCH] box: forbid TCL statements in transactional triggers
User is not allowed to execute TCL statements in on_commit and
on_rollback triggers - it is documented UB. However, this prohibition
can be easily overseen: one could, for example, try to rollback current
transaction if something is not OK in on_commit trigger. So let's check
if TCL statements are not executed in transactional triggers.
The commit introduces a new helper `txn_check_can_complete` - it is
required because there are cases when transaction cannot be continued
(txn_check_can_continue will return error) but it's allowed to try to
commit or rollback it. Example: when MVCC aborts a transaction due to
conflict, the result is not observed by user. So he cannot execute new
statements in the transaction, but we cannot forbid to try to commit
the transaction - only then the error will be observed and the
transaction will be actually completed (rolled back due to conflict).
But after the attempt to commit the transaction, we must forbid to
try to complete it again - it will lead to UB.
Along the way, the commit removes unnecessary check in function
`box_txn_rollback`.
Closes #7331
NO_DOC=bugfix
---
...-access-and-txn-control-in-txn-triggers.md | 5 ++++
src/box/txn.c | 20 ++++++++++++-
src/box/txn.h | 28 +++++++++++++++++++
...d_database_access_in_txn_triggers_test.lua | 13 ++++++++-
4 files changed, 64 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/gh-9186-7331-forbid-db-access-and-txn-control-in-txn-triggers.md
diff --git a/changelogs/unreleased/gh-9186-7331-forbid-db-access-and-txn-control-in-txn-triggers.md b/changelogs/unreleased/gh-9186-7331-forbid-db-access-and-txn-control-in-txn-triggers.md
new file mode 100644
index 0000000000..6f17db0549
--- /dev/null
+++ b/changelogs/unreleased/gh-9186-7331-forbid-db-access-and-txn-control-in-txn-triggers.md
@@ -0,0 +1,5 @@
+## bugfix/box
+
+* An attempt to execute a DDL, DML, DQL, DCL or TCL query within
+ a transactional trigger (`on_commit` or `on_rollback`) now fails
+ with a specific error (gh-9186, gh-7331).
diff --git a/src/box/txn.c b/src/box/txn.c
index 06e7ce87e2..e7fdf4f6f3 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -1280,6 +1280,8 @@ box_txn_commit(void)
diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
return -1;
}
+ if (txn_check_can_complete(txn) != 0)
+ return -1;
return txn_commit(txn);
}
@@ -1289,10 +1291,12 @@ box_txn_rollback(void)
struct txn *txn = in_txn();
if (txn == NULL)
return 0;
- if (txn && txn->in_sub_stmt) {
+ if (txn->in_sub_stmt) {
diag_set(ClientError, ER_ROLLBACK_IN_SUB_STMT);
return -1;
}
+ if (txn_check_can_complete(txn) != 0)
+ return -1;
assert(txn->signature == TXN_SIGNATURE_UNKNOWN);
txn->signature = TXN_SIGNATURE_ROLLBACK;
txn_rollback(txn); /* doesn't throw */
@@ -1475,6 +1479,13 @@ box_txn_savepoint(void)
diag_set(ClientError, ER_NO_TRANSACTION);
return NULL;
}
+ /*
+ * Creating a savepoint does not create any statements in
+ * the transaction, so check if the transaction can be completed
+ * is sufficient.
+ */
+ if (txn_check_can_complete(txn) != 0)
+ return NULL;
return txn_savepoint_new(txn, NULL);
}
@@ -1500,6 +1511,13 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
return -1;
}
+ /*
+ * Rollback to savepoint does not create any statements in
+ * the transaction, so check if the transaction can be completed
+ * is sufficient.
+ */
+ if (txn_check_can_complete(txn) != 0)
+ return -1;
txn_rollback_to_svp(txn, svp->stmt, true);
/* Discard from list all newer savepoints. */
RLIST_HEAD(discard);
diff --git a/src/box/txn.h b/src/box/txn.h
index c4dcd378a1..aff9e3894f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -581,6 +581,34 @@ txn_check_can_continue(struct txn *txn)
return 0;
}
+/**
+ * Checks if the transaction can be completed (rollback or commit).
+ * There are cases when transaction cannot be continued (txn_check_can_continue
+ * will return error) but it's allowed to try to commit or rollback it.
+ * Example: when MVCC aborts a transaction due to conflict, the result is
+ * not observed by user. So he cannot execute new statements in the transaction
+ * but we cannot forbid to try to commit the transaction - only then the error
+ * will be observed and the transaction will be actually completed (rolled back
+ * due to conflict). But after the attempt to commit the transaction, we must
+ * forbid to try to complete it again - it will lead to UB.
+ * Returns 0 if true. Otherwise, sets diag and returns -1.
+ */
+static inline int
+txn_check_can_complete(struct txn *txn)
+{
+ enum txn_status status = txn->status;
+ if (status == TXN_ABORTED && txn_has_flag(txn, TXN_IS_ROLLED_BACK)) {
+ /* Cannot complete already rolled back transaction. */
+ diag_set(ClientError, ER_TXN_ROLLBACK);
+ return -1;
+ } else if (status == TXN_COMMITTED) {
+ /* Cannot complete already committed transaction. */
+ diag_set(ClientError, ER_TXN_COMMIT);
+ return -1;
+ }
+ return 0;
+}
+
/* Pointer to the current transaction (if any) */
static inline struct txn *
in_txn(void)
diff --git a/test/box-luatest/gh_9186_7331_forbid_database_access_in_txn_triggers_test.lua b/test/box-luatest/gh_9186_7331_forbid_database_access_in_txn_triggers_test.lua
index 98d5d41284..be4a6c4595 100644
--- a/test/box-luatest/gh_9186_7331_forbid_database_access_in_txn_triggers_test.lua
+++ b/test/box-luatest/gh_9186_7331_forbid_database_access_in_txn_triggers_test.lua
@@ -27,10 +27,14 @@ g.test_database_access_in_txn_triggers = function(cg)
-- Create user to check DCL
box.schema.user.create('test_user')
+ -- A global savepoint used to test rollback_to_savepoint, set later
+ local svp = nil
+
-- Checker for old triggers (box.on_commit and box.on_rollback)
local function check_case_old(trigger, action, finalizer, expected_err)
box.begin()
s:replace{0, 0}
+ svp = box.savepoint()
local err = nil
local function trigger_f()
local _, errmsg = pcall(action)
@@ -51,6 +55,7 @@ g.test_database_access_in_txn_triggers = function(cg)
trigger.set(event, "test_trigger", trigger_f)
box.begin()
+ svp = box.savepoint()
s:replace{0, 0}
finalizer()
@@ -85,7 +90,13 @@ g.test_database_access_in_txn_triggers = function(cg)
end,
function()
box.schema.user.revoke('test_user', 'read,write', 'space', 's')
- end
+ end,
+
+ -- TCL
+ function() box.commit() end,
+ function() box.rollback() end,
+ function() box.rollback_to_savepoint(svp) end,
+ function() box.savepoint() end,
}
for _, case in pairs(cases) do
--
GitLab