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 0000000000000000000000000000000000000000..6f17db05494610adce0b762de307cb1b15b39787 --- /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 06e7ce87e295c091060a22c6bc6dbf1e8fefce21..e7fdf4f6f3bb827fb1132bbf92272e91ec2fde46 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 c4dcd378a185908c9b857a0dcc3105ed94196161..aff9e3894f62bfae62fbf3991da47d148974fa00 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 98d5d412843ad2d73c70ca67806b1c1fbe7577c4..be4a6c4595cd1b24609a637a8c8288634fc2421c 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