From e92850483338fce113c7d4af48d677882292034d Mon Sep 17 00:00:00 2001 From: Arcadiy Ivanov Date: Thu, 30 Jul 2026 14:46:45 -0400 Subject: [PATCH] MDEV-40568 `Assertion 'read_only' failed` in `mariadb_error_read_only` `opt_readonly` was read up to three times for a single read-only rejection: once by the gate that decides to deny, once more while dispatching, and a third time inside `mariadb_error_read_only()`, which formats the message from it and asserts that it is non-zero. `SET GLOBAL read_only=OFF` takes the fast path in `fix_read_only()` and stores `opt_readonly= 0` while holding only `LOCK_global_system_variables`, which none of these readers take. When that store lands between two of the reads, the statement is denied on one value and the error is built from another, and if the last read sees `OFF` then `DBUG_ASSERT(read_only)` fires. Release builds took the `read_only == 0 -> 1` fallback and reported `--read-only=ON` regardless of which level was actually rejected. Before MDEV-36425 every site emitted the constant string `--read-only` from a single read, so the reads could not disagree. Centralizing the message construction into a function that reads the global again is what added the second and third read, and the assertion. **Fix**: `mariadb_error_read_only()` takes the value as a parameter, and every caller reads `opt_readonly` once into a local and passes that on, so that the decision and its message are always based on the same value: 1. `mysql_execute_command()`, where the read is hoisted out of `deny_updates_if_read_only_option()`. That function read the global twice itself and so could decide from two different states. 2. `lock_tables_check()`, read per table. Deliberately not hoisted out of the loop: a `read_only` that is turned on while we are looping must still stop the remaining tables. 3. `trans_begin()`. 4. `THD::check_read_only_with_error()`, which serves `ha_commit_trans()`, the XA paths and `sql_trigger.cc`. Both existing assertions are kept, and now state something true: the emitter asserts a parameter that its caller has already tested, and `case READONLY_OFF: DBUG_ASSERT(0)` is unreachable because the `if` above it tested the same local. The `read_only == 0 -> 1` fallback is removed together with the race it was covering. The read-only checks are staged on purpose, at statement start, at lock acquisition and at commit, so that a completed `SET GLOBAL read_only=ON` also stops work that is already in flight. The value read is therefore kept for exactly one decision through to its own error message, and is never cached in `THD`, per statement or per transaction. The local in `mysql_execute_command()` is wrapped in a block because with `EMBEDDED_LIBRARY` the enclosing `else` is compiled away, which would put the declaration in the function scope that holds the `error:` and `finish:` labels. **Test**: `main.read_only_debug` covers four races with `DEBUG_SYNC`: the statement gate with `read_only` turned off, the statement gate with `read_only` changed to another level, the lock gate, and `START TRANSACTION READ WRITE`. The second case is the one that shows the message comes from the value that was tested rather than from the global: it changes `NO_LOCK` to `ON` instead of to `OFF`, so an implementation that still formats from the global reports the wrong level instead of silently agreeing. --- mysql-test/main/read_only_debug.result | 84 +++++++++++++++++ mysql-test/main/read_only_debug.test | 124 +++++++++++++++++++++++++ sql/lock.cc | 31 +++++-- sql/sql_class.h | 9 +- sql/sql_parse.cc | 32 +++++-- sql/transaction.cc | 8 +- 6 files changed, 264 insertions(+), 24 deletions(-) create mode 100644 mysql-test/main/read_only_debug.result create mode 100644 mysql-test/main/read_only_debug.test diff --git a/mysql-test/main/read_only_debug.result b/mysql-test/main/read_only_debug.result new file mode 100644 index 0000000000000..559e5efe367e0 --- /dev/null +++ b/mysql-test/main/read_only_debug.result @@ -0,0 +1,84 @@ +set @start_read_only= @@global.read_only; +create user test@localhost; +grant SUPER on *.* to test@localhost; +grant SELECT, INSERT, UPDATE, LOCK TABLES on test.* to test@localhost; +create table t1 (a int); +insert into t1 values (1),(2); +connect con1,localhost,test,,test; +connection default; +# +# read_only turned off between the statement check and the error +# +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +update t1 set a= a + 1; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +set debug_sync='RESET'; +# +# read_only changed to another level between the statement check and +# the error. The error must still name the level that was rejected on. +# +connection default; +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +update t1 set a= a + 1; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='ON'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +set debug_sync='RESET'; +# +# read_only turned off between the lock check and the error +# +connection default; +set global read_only='NO_LOCK'; +connection con1; +set debug_sync='after_lock_tables_read_only_check SIGNAL parked WAIT_FOR go'; +lock tables t1 read; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=NO_LOCK option so it cannot execute this statement +unlock tables; +set debug_sync='RESET'; +# +# read_only turned off between the start transaction check and the +# error +# +connection default; +set global read_only='ON'; +connection con1; +set debug_sync='after_trans_begin_read_only_check SIGNAL parked WAIT_FOR go'; +start transaction read write; +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; +connection con1; +ERROR HY000: The MariaDB server is running with the --read-only=ON option so it cannot execute this statement +set debug_sync='RESET'; +# +# The rows must be unchanged +# +select * from t1; +a +1 +2 +disconnect con1; +connection default; +set debug_sync='RESET'; +drop table t1; +drop user test@localhost; +set global read_only= @start_read_only; +# End of 13.1 tests diff --git a/mysql-test/main/read_only_debug.test b/mysql-test/main/read_only_debug.test new file mode 100644 index 0000000000000..486a03b9a7d80 --- /dev/null +++ b/mysql-test/main/read_only_debug.test @@ -0,0 +1,124 @@ +# Test that changing the read_only option concurrently, while another +# connection has already decided to reject a statement because of it, does +# not confuse the rejection. The error must still be given and must name +# the read_only value the rejection was decided on, not whatever the option +# happens to be by the time the error is produced. + +--source include/not_embedded.inc +--source include/have_debug_sync.inc + +set @start_read_only= @@global.read_only; + +# read_only does not apply to users with the READ ONLY ADMIN privilege, +# so we use a user without it. SUPER is needed to set debug_sync and does +# not imply READ ONLY ADMIN. +create user test@localhost; +grant SUPER on *.* to test@localhost; +grant SELECT, INSERT, UPDATE, LOCK TABLES on test.* to test@localhost; + +create table t1 (a int); +insert into t1 values (1),(2); + +connect (con1,localhost,test,,test); +connection default; + +--echo # +--echo # read_only turned off between the statement check and the error +--echo # + +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +--send update t1 set a= a + 1 + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # read_only changed to another level between the statement check and +--echo # the error. The error must still name the level that was rejected on. +--echo # + +connection default; +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_read_only_check SIGNAL parked WAIT_FOR go'; +--send update t1 set a= a + 1 + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='ON'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # read_only turned off between the lock check and the error +--echo # + +connection default; +set global read_only='NO_LOCK'; + +connection con1; +set debug_sync='after_lock_tables_read_only_check SIGNAL parked WAIT_FOR go'; +--send lock tables t1 read + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +unlock tables; +set debug_sync='RESET'; + +--echo # +--echo # read_only turned off between the start transaction check and the +--echo # error +--echo # + +connection default; +set global read_only='ON'; + +connection con1; +set debug_sync='after_trans_begin_read_only_check SIGNAL parked WAIT_FOR go'; +--send start transaction read write + +connection default; +set debug_sync='now WAIT_FOR parked'; +set global read_only='OFF'; +set debug_sync='now SIGNAL go'; + +connection con1; +--error ER_OPTION_PREVENTS_STATEMENT +--reap +set debug_sync='RESET'; + +--echo # +--echo # The rows must be unchanged +--echo # + +select * from t1; + +disconnect con1; +connection default; +set debug_sync='RESET'; +drop table t1; +drop user test@localhost; +set global read_only= @start_read_only; + +--echo # End of 13.1 tests diff --git a/sql/lock.cc b/sql/lock.cc index 79a3f98f5dda6..922d576d1d242 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -104,15 +104,19 @@ extern const char *read_only_mode_names[]; /* Give an error in case if users violates read only state + + @param read_only The opt_readonly value the caller made its decision on. + + The caller has to pass the value it tested, not opt_readonly, as a + concurrent SET GLOBAL read_only can change opt_readonly at any time. + Reading it again here could report a value that no one was denied for, + or, if read_only was just turned off, no value at all. */ -void mariadb_error_read_only() +void mariadb_error_read_only(ulong read_only) { char msg[60]; - int read_only= opt_readonly; DBUG_ASSERT(read_only); - if (unlikely(read_only == 0)) - read_only= 1; // If global readonly changed during call strxnmov(msg, sizeof(msg), "--read-only=", read_only_mode_names[read_only], @@ -200,12 +204,21 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) Prevent modifications to base tables if READ_ONLY is activated. In any case, read only does not apply to temporary tables or slave threads. + + Read opt_readonly once per table, so that this table is checked and + reported against one value even if SET GLOBAL read_only changes it + while we are here. It is read per table, not once for the whole lock + request, so that a read_only that is turned on while we are looping + still stops the remaining tables. */ - if (unlikely(opt_readonly) && + ulong read_only= opt_readonly; + + if (unlikely(read_only) && !(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_ONLY) && !t->s->tmp_table && !thd->slave_thread) { - switch (opt_readonly) + DEBUG_SYNC(thd, "after_lock_tables_read_only_check"); + switch (read_only) { case READONLY_OFF: // Impossible DBUG_ASSERT(0); @@ -214,7 +227,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && t->reginfo.lock_type >= TL_FIRST_WRITE) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; @@ -223,7 +236,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) (thd->lex->sql_command == SQLCOM_LOCK_TABLES || t->reginfo.lock_type >= TL_BLOCKS_READONLY)) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; @@ -231,7 +244,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) if (thd->lex->sql_command == SQLCOM_LOCK_TABLES || t->reginfo.lock_type >= TL_BLOCKS_READONLY) { - mariadb_error_read_only(); + mariadb_error_read_only(read_only); DBUG_RETURN(1); } break; diff --git a/sql/sql_class.h b/sql/sql_class.h index 25ffcf49d3bab..5fda70c389f7d 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -258,7 +258,7 @@ extern "C" int thd_current_status(MYSQL_THD thd); extern "C" enum enum_server_command thd_current_command(MYSQL_THD thd); extern "C" int thd_double_innodb_cardinality(MYSQL_THD thd); -extern void mariadb_error_read_only(); +extern void mariadb_error_read_only(ulong read_only); /** @class CSET_STRING @@ -3821,11 +3821,12 @@ class THD: public THD_count, /* this must be first */ */ inline bool check_read_only_with_error() { - if (likely(!opt_readonly) || slave_thread || + ulong read_only= opt_readonly; // Read once, may be changed any time + if (likely(!read_only) || slave_thread || ((security_ctx->master_access & PRIV_IGNORE_READ_ONLY) && - opt_readonly != READONLY_NO_LOCK_NO_ADMIN)) + read_only != READONLY_NO_LOCK_NO_ADMIN)) return false; - mariadb_error_read_only(); + mariadb_error_read_only(read_only); return true; } diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6a1f53f7ba102..1d6ddb2526e67 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1482,25 +1482,29 @@ dispatch_command_return do_command(THD *thd, bool blocking) @note SQLCOM_MULTI_UPDATE is an exception and dealt with elsewhere. + @param read_only The opt_readonly value to check against. The caller reads + it once and also passes it to mariadb_error_read_only(), + so that the decision and the error message are based on + the same value even if SET GLOBAL read_only changes it. + @see mysql_execute_command @returns Status code @retval TRUE The statement should be denied. @retval FALSE The statement isn't updating any relevant tables. */ -static bool deny_updates_if_read_only_option(THD *thd, TABLE_LIST *all_tables) +static bool deny_updates_if_read_only_option(THD *thd, TABLE_LIST *all_tables, + ulong read_only) { DBUG_ENTER("deny_updates_if_read_only_option"); DBUG_ASSERT(!thd->slave_thread); // Checked by caller - - if (!opt_readonly) - DBUG_RETURN(FALSE); + DBUG_ASSERT(read_only); // Checked by caller LEX *lex= thd->lex; /* Super user is allowed to do changes in some cases */ if ((thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) != NO_ACL && - opt_readonly < READONLY_NO_LOCK_NO_ADMIN) + read_only < READONLY_NO_LOCK_NO_ADMIN) DBUG_RETURN(FALSE); /* Check if command doesn't update anything */ @@ -3679,11 +3683,23 @@ mysql_execute_command(THD *thd, bool is_called_from_prepared_stmt) /* When option readonly is set deny operations which change non-temporary tables. Except for the replication thread and the 'super' users. + + opt_readonly is read once here and passed on, so that the check and + the error message use the same value even if SET GLOBAL read_only + changes it in between. The block keeps read_only out of the function + scope also when HAVE_REPLICATION is not defined and the enclosing + 'else' is compiled away. */ - if (deny_updates_if_read_only_option(thd, all_tables)) { - mariadb_error_read_only(); - DBUG_RETURN(-1); + ulong read_only= opt_readonly; + + if (unlikely(read_only) && + deny_updates_if_read_only_option(thd, all_tables, read_only)) + { + DEBUG_SYNC(thd, "after_read_only_check"); + mariadb_error_read_only(read_only); + DBUG_RETURN(-1); + } } #ifdef HAVE_REPLICATION } /* endif unlikely slave */ diff --git a/sql/transaction.cc b/sql/transaction.cc index 6aaa7140b59d2..3046507efd958 100644 --- a/sql/transaction.cc +++ b/sql/transaction.cc @@ -171,12 +171,14 @@ bool trans_begin(THD *thd, uint flags) Implicitly starting a RW transaction is allowed for backward compatibility. */ - if (opt_readonly) + ulong read_only= opt_readonly; // Read once, may be changed any time + if (read_only) { if (!(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY) || - opt_readonly == READONLY_NO_LOCK_NO_ADMIN) + read_only == READONLY_NO_LOCK_NO_ADMIN) { - mariadb_error_read_only(); + DEBUG_SYNC(thd, "after_trans_begin_read_only_check"); + mariadb_error_read_only(read_only); DBUG_RETURN(true); } }