From: Alexander Nozdrin Date: December 5 2007 11:48am Subject: Re: bk commit into 5.0 tree (thek:1.2589) BUG#27440 List-Archive: http://lists.mysql.com/commits/39310 Message-Id: <200712051448.32710.alik@mysql.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit 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