MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alexander Nozdrin Date:December 5 2007 11:48am
Subject:Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440
View as plain text  
Hi Kristofer,

thank you for working on this.

Generally, I agree with your patch, but I have a few notes.
As Mats has already approved it, you're allowed to push it as is.
However, I would be glad if you find at least some of my comments useful.

On Thursday 29 November 2007 15:03, kpettersson@stripped wrote:
> ChangeSet@stripped, 2007-11-29 13:03:08+01:00, thek@adventure.(none) +3 -0
>   Bug #27440 read_only allows create and drop database
>   
>   When read_only option was enabled, a user without SUPER privilege could
>   perform CREATE DATABASE and DROP DATABASE operations.

Could you please clarify the cause of this problem: CREATE/DROP DATABASE
statements were forgotten to check.

Another thing: you've fixed CREATE/DROP DATABASE. Could you also check
that other statements, that are not marked in the uc_update_queries array
(stored routines, events, triggers, table spaces and so on).

> diff -Nrup a/mysql-test/t/read_only.test b/mysql-test/t/read_only.test
> --- a/mysql-test/t/read_only.test	2006-11-20 15:35:19 +01:00
> +++ b/mysql-test/t/read_only.test	2007-11-29 13:03:07 +01:00
> @@ -117,4 +117,24 @@ connection default;
>  drop table t1,t2;
>  drop user test@localhost;
>  
> +#
> +# Bug #27440 read_only allows create and drop database
> +#

They say the following technique helps merging result files:

  --echo
  --echo #
  --echo # Bug #27440 read_only allows create and drop database
  --echo #
  --echo
:)

> +grant all on abc.* to `bug27440`@`%`;
> +create database bug27440;
> +grant all on bug27440.* to `bug27440`@`%`;

There is a good habit to use special names for users and databases
in the test suite:

  - users should be named like "mysqltest_XXX"
    (mysqltest_u1, mysqltest_u2, ...);

  - databases should be names like "mysqltest_dbXXX"
    (mysqltest_db1, mysqltest_db2, ...);

Also, please start with "just in case" statements:

--disable_warnings
DROP DATABASE IF EXISTS mysqltest_db1;
DROP DATABASE IF EXISTS mysqltest_db2;
--enable_warnings

DELETE FROM mysql.user WHERE User LIKE 'mysqltest_%';
DELETE FROM mysql.db WHERE User LIKE 'mysqltest_%';
DELETE FROM mysql.tables_priv WHERE User LIKE 'mysqltest_%';
DELETE FROM mysql.columns_priv WHERE User LIKE 'mysqltest_%';
FLUSH PRIVILEGES;

> +flush privileges;

AFAIK, FLUSH PRIVILEGES is not required as long as you use GRANT/REVOKE
statements to update privileges.

> +connect (con_bug27440,127.0.0.1,bug27440,,test,$MASTER_MYPORT,);
> +connection con_bug27440;

Basically, "connect" itself switches the active connection. So, you don't
need the second "connection" directive.

Also, I find it useful to add a line to the result file, when you swicth
the active connection. Something like the following:

--connect (con_bug27440,127.0.0.1,bug27440,,test,$MASTER_MYPORT,)
--echo # Connection: con_bug27440 (bug27440/test)

> +show grants for current_user();
> +--error ER_OPTION_PREVENTS_STATEMENT
> +create database abc;
> +show databases like '%abc%';
> +--error ER_OPTION_PREVENTS_STATEMENT
> +drop database bug27440;
> +disconnect con_bug27440;
> +connection default;
> +revoke all on abc.* from `bug27440`@`%`;
> +revoke all on bug27440.* from `bug27440`@`%`;
> +drop database bug27440;

While we're at it, I'd like to see the complete test coverage of this option.
I.e. let's test that:

  - if it is a multi-update, it is allowed;

  - if the user is a super user, everything is allowed;

  - if we're accessing temprary objects, it's allowed;

  - also, please add tests for other object types (stored routines, ...);

>  set global read_only=0;
> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc	2007-11-22 13:18:18 +01:00
> +++ b/sql/sql_parse.cc	2007-11-29 13:03:07 +01:00
> @@ -196,7 +196,6 @@ inline bool all_tables_not_ok(THD *thd, 
>  }
>  #endif
>  
> -
>  static bool some_non_temp_table_to_be_updated(THD *thd, TABLE_LIST *tables)
>  {
>    for (TABLE_LIST *table= tables; table; table= table->next_global)
> @@ -209,6 +208,7 @@ static bool some_non_temp_table_to_be_up
>    return 0;
>  }
>  
> +

These are unnecessary changes, but I'm fine with them. I'm only wondering
why did you remove two empty lines in one case, and added them in another? :)

> @@ -2590,18 +2590,50 @@ mysql_execute_command(THD *thd)
>        When option readonly is set deny operations which change non-temporary
>        tables. Except for the replication thread and the 'super' users.
>      */
> -    if (opt_readonly &&
> -        !(thd->security_ctx->master_access & SUPER_ACL) &&
> -        uc_update_queries[lex->sql_command] &&
> -        !((lex->sql_command == SQLCOM_CREATE_TABLE) &&
> -          (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE)) &&
> -        !((lex->sql_command == SQLCOM_DROP_TABLE) &&
> lex->drop_temporary) &&
> -        ((lex->sql_command != SQLCOM_UPDATE_MULTI) &&
> -          some_non_temp_table_to_be_updated(thd, all_tables)))
>      {
> -      my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
> -      DBUG_RETURN(-1);
> -    }

I believe, mysql_execute_command() is already quite a big function.
I would create a private (static) function (something like
bool check_read_only_option()), and put all these checks there.

So, here would be only:

  if (check_read_only_option(...))
    DBUG_RETURN(-1);

> +      const my_bool user_is_not_super=
> +        !((long)(thd->security_ctx->master_access & SUPER_ACL) ==
> +          (long)SUPER_ACL);

1. Why do you need cast? Security_ctx::master_access is already of ulong type.

2. I prefer not using negative nouns for names. I'd name this variable
   something like "user_is_ordinary".

> +
> +      const my_bool create_temp_tables= 
> +        (lex->sql_command == SQLCOM_CREATE_TABLE) &&
> +        (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE);
> +
> +      const my_bool drop_temp_tables= 
> +        (lex->sql_command == SQLCOM_DROP_TABLE) &&
> +        lex->drop_temporary;
> +
> +      const my_bool update_real_tables=
> +        some_non_temp_table_to_be_updated(thd, all_tables) &&
> +        !(create_temp_tables || drop_temp_tables);
> +
> +      const my_bool update_temp_tables=
> +        !update_real_tables;
> +
> +      const my_bool create_or_drop_databases=
> +        (lex->sql_command == SQLCOM_CREATE_DB) ||
> +        (lex->sql_command == SQLCOM_DROP_DB);
> +
> +       if (opt_readonly &&
> +          user_is_not_super &&
> +          uc_update_queries[lex->sql_command])
> +      {

Although I don't like micro-optimizations, I think in this case it would
be useful -- you calculate a number of flags, which are not needed
if opt_readonly (for one) is not set.

If you created a separate function (check_read_only_option()),
the code coulde be as follows:

bool check_read_only_option(...)
{
  if (!opt_readonly)
    return FALSE; /* read-only is not set, no further checks needed. */

  if (thd->security_ctx->master_access & SUPER_ACL)
    return FALSE; /* user is a superuser, no further checks needed. */

  if (lex->sql_command == SQLCOM_UPDATE_MULTI)
    return FALSE; /* this is a multi-update query, it will be handled later. */

  ...
}

I believe this way these checks would be more natural/readable.

> +        if ((update_real_tables &&
> +             lex->sql_command != SQLCOM_UPDATE_MULTI) ||
> +            (update_temp_tables && create_or_drop_databases))
> +        {
> +            /*
> +              An attempt was made to modify one or more non-temporary tables.
> +              NOTE: SQLCOM_UPDATE_MULTI is a special case and handled later.
> +            */

Well... Frankly speaking, I'm finding this comment misleading. May be I'm
a fool, but it took me three times to understand that if
lex->sql_command == SQLCOM_UPDATE_MULTI you are *not* going to throw
and error. Could you please move it from this block at least?

> +            my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
> +            DBUG_RETURN(-1);
> +        } else
> +        {
> +          /* Assuming that only temporary tables are modified. */
> +        }
> +      }
> +  }

Thanks!

-- 
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com
Thread
bk commit into 5.0 tree (thek:1.2589) BUG#27440kpettersson29 Nov
  • Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440Alexander Nozdrin5 Dec
    • Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440Kristofer Pettersson6 Dec