Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440

[Mats Kindahl]

Hi Kristofer!

Here are my comments. In summary, I would like the following items handled:

    * A better cset comment for the test file (what did you add?)
    * Fixing a typo where you have SQLCOM_CREATE_DB twice instead of
      what I think is supposed to be SQLCOM_DROP_DB
    * A test for DROP DATABASE as well, since I think that it does not work
    * Checking the conditions more  thouroughly: I don't think they are
      correct

Just my few cents,
Mats Kindahl

kpettersson@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of thek. When thek does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-11-28 11:29:41+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.
>   
>   This patch adds a check to make sure this isn't possible.
>
>   mysql-test/r/read_only.result@stripped, 2007-11-28 11:29:39+01:00, thek@adventure.(none)
> +10 -0
>     Updated result file
>
>   mysql-test/t/read_only.test@stripped, 2007-11-28 11:29:39+01:00, thek@adventure.(none)
> +15 -0
>     Added test case.
>   

Added test case for what? I just see a test for create database abc, but 
what about drop database? Is it already in the result file or not. If 
the file comment indicated that, it is obvious that the case is already 
tested for in the file.

>   sql/sql_parse.cc@stripped, 2007-11-28 11:29:39+01:00, thek@adventure.(none) +44 -12
>     - Simplified complex predicate by grouping it in a read friendly way.
>     - Added predicate to fail on database updates while running in read-only
>       mode.
>
> diff -Nrup a/mysql-test/r/read_only.result b/mysql-test/r/read_only.result
> --- a/mysql-test/r/read_only.result	2006-11-20 15:35:19 +01:00
> +++ b/mysql-test/r/read_only.result	2007-11-28 11:29:39 +01:00
> @@ -46,4 +46,14 @@ Warnings:
>  Note	1051	Unknown table 'ttt'
>  drop table t1,t2;
>  drop user test@localhost;
> +grant all on abc.* to `bug27440`@`%`;
> +show grants for current_user();
> +Grants for bug27440@%
> +GRANT USAGE ON *.* TO 'bug27440'@'%'
> +GRANT ALL PRIVILEGES ON `abc`.* TO 'bug27440'@'%'
> +create database abc;
> +ERROR HY000: The MySQL server is running with the --read-only option so it cannot
> execute this statement
> +show databases like '%abc%';
> +Database (%abc%)
> +revoke all on abc.* from `bug27440`@`%`;
>  set global read_only=0;
> 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-28 11:29:39 +01:00
> @@ -117,4 +117,19 @@ connection default;
>  drop table t1,t2;
>  drop user test@localhost;
>  
> +#
> +# Bug #27440 read_only allows create and drop database
> +#
> +grant all on abc.* to `bug27440`@`%`;
> +
> +connect (con_bug27440,127.0.0.1,bug27440,,test,$MASTER_MYPORT,);
> +connection con_bug27440;
> +show grants for current_user();
> +--error ER_OPTION_PREVENTS_STATEMENT
> +create database abc;
> +show databases like '%abc%';
> +disconnect con_bug27440;
> +connection default;
> +revoke all on abc.* from `bug27440`@`%`;
> +
>   

I would like to see a test for drop database as well.

>  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-28 11:29:39 +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;
>  }
>  
> +
>  #ifndef NO_EMBEDDED_ACCESS_CHECKS
>  static HASH hash_user_connections;
>  
> @@ -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);
> -    }
> +      my_bool user_is_not_super=
> +        !((long)(thd->security_ctx->master_access & SUPER_ACL) ==
> +          (long)SUPER_ACL);
> +
> +      my_bool create_temp_tables= 
> +        (lex->sql_command == SQLCOM_CREATE_TABLE) &&
> +        (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE);
> +
> +      my_bool drop_temp_tables= 
> +        (lex->sql_command == SQLCOM_DROP_TABLE) &&
> +        lex->drop_temporary;
> +
> +      my_bool update_real_tables=
> +        some_non_temp_table_to_be_updated(thd, all_tables) &&
> +        !(create_temp_tables || drop_temp_tables);
>   

This is roughly "\exists t . \lnot temp(t) \land updated(t) \land \lnot 
(CREATE_TABLE \lor DROP_TABLE)".
> +
> +      my_bool update_temp_tables=
> +        !update_real_tables;
>   

This is roughly "\forall t . temp(t) \lor \lnot updated(t) \lor 
CREATE_TABLE \lor DROP_TABLE", which is probably not what you intended.

> +
> +      my_bool create_or_drop_databases=
> +        (lex->sql_command == SQLCOM_CREATE_DB) ||
> +        (lex->sql_command == SQLCOM_CREATE_DB);
>   
I think you mean SQLCOM_DROP_DB as the second item.

Make the variables const, it's a good habit.

> +
> +       if (opt_readonly &&
> +          user_is_not_super &&
> +          uc_update_queries[lex->sql_command])
> +      {
> +        if ((update_real_tables &&
> +             lex->sql_command != SQLCOM_UPDATE_MULTI) ||
>   

So this means that it is (potentially) OK if we are updating real tables 
using a multi-table update?

> +            (update_temp_tables && create_or_drop_databases))
>   

So this means that it's OK if we are not updating temporary tables or 
are not creating or dropping a database?

The condition does not look OK. Since the different branches of the 
conjunction checks for values of lex->sql_command with different values 
(SQLCOM_CREATE_DB and SQLCOM_DROP_TABLE, for example), the condition can 
never be true since lex->sql_command can only take one value.

> +        {
> +            /*
> +              An attempt was made to modify one or more non-temporary tables.
> +              NOTE: SQLCOM_UPDATE_MULTI is a special case and handled later.
> +            */
>   

Ah... OK.

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


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com