MDEV-40568 Assertion `read_only' failed in mariadb_error_read_only

[arcivanov]

MDEV-40568 Assertion 'read_only' failed in mariadb_error_read_only

A statement rejected because the server is read-only can crash a debug build, or report the wrong read-only level in a release build, if another connection changes read_only at the same moment.

mysqld: sql/lock.cc:113: void mariadb_error_read_only(): Assertion `read_only' failed.

mariadb_error_read_only () at sql/lock.cc:113
mysql_execute_command (thd=..., is_called_from_prepared_stmt=false) at sql/sql_parse.cc:3685
mysql_parse (...) at sql/sql_parse.cc:7945

Reported by Elena Stepanova against a branch carrying the HEAP blob work, but it is not related to it: the statement is rejected before any table is opened. The bug is reproducible on a plain InnoDB table with no blobs and no temporary tables involved, and reaches back to 12.0.1.

Cause

opt_readonly is read up to three times for a single 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.

void mariadb_error_read_only()
{
  char msg[60];
  int read_only= opt_readonly;         /* third read */
  DBUG_ASSERT(read_only);
  if (unlikely(read_only == 0))
    read_only= 1;                      // If global readonly changed during call

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:

  if (read_only == FALSE || read_only == opt_readonly)
  {
    opt_readonly= read_only;
    DBUG_RETURN(false);
  }

Turning read-only on is serialized behind lock_global_read_lock() and make_global_read_lock_block_commit(), which is why the variable looks protected. Turning it off is a bare store, and every race here is in that direction. Holding locks does not help the reader: ha_commit_trans() holds MDL_BACKUP_COMMIT, which does block a concurrent read_only=ON, and an OFF still walks past it.

So when the store lands between two of the reads, the statement is denied on one value and the error is built from another. If the last read sees OFF, the assertion fires. Release builds took the fallback above and reported --read-only=ON regardless of which level was actually rejected.

The fallback and its comment show the race was anticipated; the assertion one line above contradicts them.

Before MDEV-36425 every site emitted a constant string from a single read:

my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");

so the reads could not disagree. Centralizing 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 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, at the !opt_readonly early return and at the PRIV_IGNORE_READ_ONLY check, and so could decide from two different states.
2. lock_tables_check(), read per table.
3. trans_begin().
4. THD::check_read_only_with_error(), which serves ha_commit_trans(), the XA paths and sql_trigger.cc.

The read in lock_tables_check() is deliberately not hoisted out of the per-table loop. 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. Hoisting would make a mid-loop change to ON get ignored for the whole lock request. For the same reason the value is kept for exactly one decision through to its own error message, and is never cached in THD, per statement or per transaction.

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 local in mysql_execute_command() is wrapped in a block because with EMBEDDED_LIBRARY the enclosing else is compiled away, which would otherwise put the declaration in the function scope that holds the error: and finish: labels.

Tests

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 test user is granted SUPER so it can arm the sync points, which since 10.11 does not imply READ ONLY ADMIN and so leaves it subject to read_only.

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.

Tests were written before the fix and verified against it, each on its own build:

• Unfixed: the first case crashes at lock.cc:113 with the reported stack.
• Reverting only the lock_tables_check() local: the lock case crashes on case READONLY_OFF: DBUG_ASSERT(0), the statement cases still pass.
• Reverting only the trans_begin() local: the transaction case crashes in mariadb_error_read_only(), the other three still pass.
• Formatting the message from the global with the passed value only as a fallback: caught by the second case alone, as a one-line difference reporting --read-only=ON where --read-only=NO_LOCK was rejected, with no crash.

One path is fixed but not covered: THD::check_read_only_with_error() is a header inline and sql/sql_class.h does not include debug_sync.h, so the commit and XA sites cannot be raced from a test without out-lining it. The pre-existing ha_commit_trans_after_acquire_commit_lock sync point does not substitute, as it parks before the function is entered.

Verification

• main: 1416/1416, main.read_only_debug stable over --repeat=5.
• read_only, read_only_innodb, grant_read_only, trans_read_only, lock, lock_tables_lost_commit, xa, flush_read_lock, commit: 10/10.
• Clean build, no new warnings.

Basing this PR

Cut against main as requested. The bug was introduced by ce8a74f2352 (MDEV-36425, 2025-03-31) and is present in 12.0.1 through 13.0.1, so it may warrant a lower base if it should be fixed on a released series.