MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Kristofer Pettersson Date:December 6 2007 8:43am
Subject:Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440
View as plain text  
Alexander Nozdrin wrote:
> 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.
>   
I don't agree this is the cause of the problem. By checking CREATE/DROP 
DATABASE we may avoid the problem though.
> 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).
>   
Ok.
>   
>> 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
> :)
>   
ok.
>   
>> +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, ...);
>
>   
I don't see why this is a particular good habit, but I'll comply with 
your request.
> Also, please start with "just in case" statements:
>
> --disable_warnings
> DROP DATABASE IF EXISTS mysqltest_db1;
> DROP DATABASE IF EXISTS mysqltest_db2;
> --enable_warnings
>
>   
ok, will do. I believe that chances are that the database name 
associated with a bug-number is a fairly isolated from other name 
spaces, don't you think?
> 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.
>   
Ok, removing. The reason is a line in the manual which states:

    *

      Database privilege changes take effect at the next |USE
      /|db_name|/| statement.

It was unclear whether this meant the explicit command "USE db_name" or 
if a reference was enough. Anyway I though it was good habit to 
explicitly synchronize the privileges with FLUSH PRIVILEGES to be 
absolutely sure.

>   
>> +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.
>   
That is not very intuitive in my mind but I suppose it can be memorized.
> 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)
>   
Ok.
>   
>> +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, ...);
>
>   
Ok. Isn't most of this tested already in read-only.test? I can add tests 
for more objects of course.
>>  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? :)
>   
Superior sense of taste?

>   
>> @@ -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);
>   
Ok.
>   
>> +      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.
>   
Without cast the compiler doesn't give me the right answer. This might 
be a compiler error, but I must investigate it more thoroughly.
> 2. I prefer not using negative nouns for names. I'd name this variable
>    something like "user_is_ordinary".
>   
I do, sometimes. :)
>   
>> +
>> +      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.
>   
I agree. I'm refactoring the code into a function.
>   
>> +        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?
>   
I think this is a fundamental problem with stacked logical expressions 
and conditionals in general that what you perceive to be crystal clear 
is different from what anyone else might think. Somebody thought the 
original expression was crystal clear, and I worked with that to get the 
above. I agree that refactoring this into a function and use simpler 
filters to sort out the expression is even easier for me (and for you I 
suppose) to read.



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