MDEV-30978: On slave XA COMMIT/XA ROLLBACK fail to return an error in read-only mode

Where a read-only server permits writes through replication, it
should not permit user connections to commit/rollback XA
transactions prepared via replication. The bug reported in
MDEV-30978 shows that this can happen. This is because there is no
read only check in the XA transaction logic, the most relevant one
occurs in ha_commit_trans() for normal statements/transactions.

This patch extends the XA transaction logic to check the read only
status of the server before performing an XA COMMIT or ROLLBACK.

Reviewed By:
Andrei Elkin <andrei.elkin@mariadb.com>

mysql-test/main/xa.result

@@ -544,6 +544,54 @@ formatID	gtrid_length	bqual_length	data
 drop table asd;
 disconnect con_tmp;
 connection default;
+# MDEV-30978: a read-only server should still allow the commit of
+#             read-only XA transactions
+set @sav_read_only=@@global.read_only;
+set global read_only=1;
+xa start '1';
+select 0;
+0
+0
+xa end '1';
+xa prepare '1';
+xa commit '1';
+xa start '2';
+select 0;
+0
+0
+xa end '2';
+xa prepare '2';
+xa rollback '2';
+# Read-only disconnect case
+connect  con1_ro,localhost,root,,;
+xa start '3';
+select 0;
+0
+0
+xa end '3';
+xa prepare '3';
+disconnect con1_ro;
+connection default;
+xa recover;
+formatID	gtrid_length	bqual_length	data
+1	1	0	3
+xa commit '3';
+ERROR XA100: XA_RBROLLBACK: Transaction branch was rolled back
+connect  con2_ro,localhost,root,,;
+xa start '4';
+select 0;
+0
+0
+xa end '4';
+xa prepare '4';
+disconnect con2_ro;
+connection default;
+xa recover;
+formatID	gtrid_length	bqual_length	data
+1	1	0	4
+xa rollback '4';
+ERROR XA100: XA_RBROLLBACK: Transaction branch was rolled back
+set @@global.read_only=@sav_read_only;
 #
 # End of 10.5 tests
 #

mysql-test/main/xa.test

@@ -695,6 +695,58 @@ disconnect con_tmp;
 --source include/wait_until_disconnected.inc
 connection default;
 
+--echo # MDEV-30978: a read-only server should still allow the commit of
+--echo #             read-only XA transactions
+set @sav_read_only=@@global.read_only;
+set global read_only=1;
+
+# Commit case
+xa start '1';
+select 0;
+xa end '1';
+xa prepare '1';
+xa commit '1';
+
+# Rollback case
+xa start '2';
+select 0;
+xa end '2';
+xa prepare '2';
+xa rollback '2';
+
+--echo # Read-only disconnect case
+
+--source include/count_sessions.inc
+connect (con1_ro,localhost,root,,);
+xa start '3';
+select 0;
+xa end '3';
+xa prepare '3';
+disconnect con1_ro;
+
+connection default;
+--source include/wait_until_count_sessions.inc
+xa recover;
+--error ER_XA_RBROLLBACK
+xa commit '3';
+
+--source include/count_sessions.inc
+connect (con2_ro,localhost,root,,);
+xa start '4';
+select 0;
+xa end '4';
+xa prepare '4';
+disconnect con2_ro;
+
+connection default;
+--source include/wait_until_count_sessions.inc
+xa recover;
+--error ER_XA_RBROLLBACK
+xa rollback '4';
+
+set @@global.read_only=@sav_read_only;
+
+
 --echo #
 --echo # End of 10.5 tests
 --echo #

mysql-test/suite/rpl/r/rpl_read_only.result

@@ -139,6 +139,26 @@ insert into t1 values(1006);
 ERROR HY000: The MariaDB server is running with the --read-only option so it cannot execute this statement
 insert into t2 values(2006);
 ERROR HY000: The MariaDB server is running with the --read-only option so it cannot execute this statement
+#
+# MDEV-30978: On slave XA COMMIT/XA ROLLBACK fail to return an error in read-only mode
+#
+#   Where a read-only server permits writes through replication, it
+# should not permit user connections to commit/rollback XA transactions
+# prepared via replication. This test ensure this behavior is prohibited
+#
+connection master;
+xa start '1';
+insert into t1 values (1007);
+xa end '1';
+xa prepare '1';
+connection slave;
+connection slave2;
+xa commit '1';
+ERROR HY000: The MariaDB server is running with the --read-only option so it cannot execute this statement
+xa rollback '1';
+ERROR HY000: The MariaDB server is running with the --read-only option so it cannot execute this statement
+connection master;
+xa rollback '1';
 connection master;
 drop user test;
 drop table t1;

mysql-test/suite/rpl/t/rpl_read_only.test

@@ -117,6 +117,33 @@ insert into t1 values(1006);
 --error ER_OPTION_PREVENTS_STATEMENT
 insert into t2 values(2006);
 
+
+--echo #
+--echo # MDEV-30978: On slave XA COMMIT/XA ROLLBACK fail to return an error in read-only mode
+--echo #
+--echo #   Where a read-only server permits writes through replication, it
+--echo # should not permit user connections to commit/rollback XA transactions
+--echo # prepared via replication. This test ensure this behavior is prohibited
+--echo #
+
+# Note: slave's read_only=1 is set prior to this test case
+
+connection master;
+xa start '1';
+insert into t1 values (1007);
+xa end '1';
+xa prepare '1';
+sync_slave_with_master;
+
+connection slave2;
+--error ER_OPTION_PREVENTS_STATEMENT
+xa commit '1';
+--error ER_OPTION_PREVENTS_STATEMENT
+xa rollback '1';
+
+connection master;
+xa rollback '1';
+
 ## Cleanup
 connection master;
 drop user test;

sql/handler.cc

@@ -1668,10 +1668,7 @@ int ha_commit_trans(THD *thd, bool all)
     DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock");
   }
 
-  if (rw_trans &&
-      opt_readonly &&
-      !(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) &&
-      !thd->slave_thread)
+  if (rw_trans && thd->is_read_only_ctx())
   {
     my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
     goto err;

sql/sql_class.h

@@ -2787,6 +2787,16 @@ class THD: public THD_count, /* this must be first */
     return m_binlog_filter_state;
   }
 
+  /**
+    Checks if a user connection is read-only
+  */
+  inline bool is_read_only_ctx()
+  {
+    return opt_readonly &&
+           !(security_ctx->master_access & PRIV_IGNORE_READ_ONLY) &&
+           !slave_thread;
+  }
+
 private:
   /**
     Indicate if the current statement should be discarded

sql/xa.cc

@@ -601,6 +601,16 @@ bool trans_xa_commit(THD *thd)
     if (auto xs= xid_cache_search(thd, thd->lex->xid))
     {
       bool xid_deleted= false;
+      MDL_request mdl_request;
+      bool rw_trans= (xs->rm_error != ER_XA_RBROLLBACK);
+
+      if (rw_trans && thd->is_read_only_ctx())
+      {
+        my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+        res= 1;
+        goto _end_external_xid;
+      }
+
       res= xa_trans_rolled_back(xs);
       /*
         Acquire metadata lock which will ensure that COMMIT is blocked
@@ -609,7 +619,6 @@ bool trans_xa_commit(THD *thd)
 
         We allow FLUSHer to COMMIT; we assume FLUSHer knows what it does.
       */
-      MDL_request mdl_request;
       MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
                        MDL_EXPLICIT);
       if (thd->mdl_context.acquire_lock(&mdl_request,
@@ -659,7 +668,11 @@ bool trans_xa_commit(THD *thd)
     DBUG_RETURN(res);
   }
 
-  if (xa_trans_rolled_back(xid_state.xid_cache_element))
+  if (thd->transaction->all.is_trx_read_write() && thd->is_read_only_ctx())
+  {
+    my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+    DBUG_RETURN(TRUE);
+  } else if (xa_trans_rolled_back(xid_state.xid_cache_element))
   {
     xa_trans_force_rollback(thd);
     DBUG_RETURN(thd->is_error());
@@ -777,6 +790,15 @@ bool trans_xa_rollback(THD *thd)
       bool res;
       bool xid_deleted= false;
       MDL_request mdl_request;
+      bool rw_trans= (xs->rm_error != ER_XA_RBROLLBACK);
+
+      if (rw_trans && thd->is_read_only_ctx())
+      {
+        my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+        res= 1;
+        goto _end_external_xid;
+      }
+
       MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT,
                        MDL_EXPLICIT);
       if (thd->mdl_context.acquire_lock(&mdl_request,
@@ -822,7 +844,11 @@ bool trans_xa_rollback(THD *thd)
     DBUG_RETURN(thd->get_stmt_da()->is_error());
   }
 
-  if (xid_state.xid_cache_element->xa_state == XA_ACTIVE)
+  if (thd->transaction->all.is_trx_read_write() && thd->is_read_only_ctx())
+  {
+    my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+    DBUG_RETURN(TRUE);
+  } else if (xid_state.xid_cache_element->xa_state == XA_ACTIVE)
   {
     xid_state.er_xaer_rmfail();
     DBUG_RETURN(TRUE);