Skip to content

MDEV-40568 Assertion `read_only' failed in mariadb_error_read_only #5474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arcivanov wants to merge 1 commit into
base: main
Choose a base branch
from
+264 −24
Open

MDEV-40568 Assertion `read_only' failed in mariadb_error_read_only #5474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions mysql-test/main/read_only_debug.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
124 changes: 124 additions & 0 deletions mysql-test/main/read_only_debug.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
31 changes: 22 additions & 9 deletions sql/lock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -223,15 +236,15 @@ 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;
case READONLY_NO_LOCK_NO_ADMIN:
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;
Expand Down
9 changes: 5 additions & 4 deletions sql/sql_class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down
32 changes: 24 additions & 8 deletions sql/sql_parse.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 */
Expand Down
8 changes: 5 additions & 3 deletions sql/transaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down