List:Commits« Previous MessageNext Message »
From:Guangbao Ni Date:March 5 2009 2:30am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (gni:2827) Bug#42217
View as plain text  
Hello Luis,

Thanks for your review.

Best wishes,
Guangbao

Luís Soares Wrote:
> Hi Guangbao,
>   Good work. 
>
>   I have some comments, please check below.
>
> On Wed, 2009-03-04 at 14:16 +0000, Guangbao Ni wrote:
>   
>> #At file:///home/ngb/mysql/bzr/bug42217-5.1-bugteam/
>>
>>  2827 Guangbao Ni	2009-03-04
>>       Bug #42217      mysql.procs_priv does not get replicated
>>       
>>       mysql.procs_priv table itself does not get replicated.
>>       Inserting routine privilege record into mysql.procs_priv table
>>       is triggered by creating function/procedure statements
>>       according to current user's privileges.
>>       Because the current user of SQL thread has GLOBAL_ACL,
>>       which doesn't need any check mysql.procs_priv privilege
>>       when create/alter/execute routines.
>>       Corresponding GLOBAL_ACL privilege user
>>       doesn't insert routine privilege record into
>>       mysql.procs_priv when creating a routine.
>>       
>>       Fixed by switching the current user of SQL thread to definer user if
>>       the definer user exists on slave.
>>       That populates procs_priv, otherwise to keep the SQL thread
>>       user and procs_priv remains unchanged.
>> modified:
>>   mysql-test/suite/rpl/r/rpl_do_grant.result
>>   mysql-test/suite/rpl/t/rpl_do_grant.test
>>   sql/sql_parse.cc
>>
>> per-file messages:
>>   mysql-test/suite/rpl/r/rpl_do_grant.result
>>     Test case result for routine privilege when definer user exist or not on
> slave
>>   mysql-test/suite/rpl/t/rpl_do_grant.test
>>     Test case result for routine privilege when definer user exist or not on
> slave
>>   sql/sql_parse.cc
>>     Switch current user of SQL thread to definer user if the definer user
>>     existes on slave when checking whether the routine privilege is
>>     needed to insert mysql.procs_priv table or not.
>> === modified file 'mysql-test/suite/rpl/r/rpl_do_grant.result'
>> --- a/mysql-test/suite/rpl/r/rpl_do_grant.result	2007-06-27 12:28:02 +0000
>> +++ b/mysql-test/suite/rpl/r/rpl_do_grant.result	2009-03-04 14:15:22 +0000
>> @@ -89,3 +89,73 @@ show grants for rpl_do_grant2@localhost;
>>  ERROR 42000: There is no such grant defined for user 'rpl_do_grant2' on host
> 'localhost'
>>  show grants for rpl_do_grant2@localhost;
>>  ERROR 42000: There is no such grant defined for user 'rpl_do_grant2' on host
> 'localhost'
>> +DROP DATABASE IF EXISTS privdb;
>>     
> SUGGESTION:
>   probably protect this with disable_warnings; 
>
>   
Okay, added.
>> +Warnings:
>> +Note	1008	Can't drop database 'privdb'; database doesn't exist
>> +CREATE DATABASE  privdb;
>> +GRANT CREATE ROUTINE ON privdb.* TO 'create_rout_db'@'localhost'
>> +        IDENTIFIED BY 'create_rout_db' WITH GRANT OPTION;
>> +USE privdb;
>> +CREATE FUNCTION upgrade_del_func() RETURNS CHAR(30)
>> +BEGIN
>> +RETURN "INSIDE upgrade_del_func()";
>> +END//
>> +USE privdb;
>> +SELECT * FROM mysql.procs_priv;
>> +Host	Db	User	Routine_name	Routine_type	Grantor	Proc_priv	Timestamp
>>
> +localhost	privdb	create_rout_db	upgrade_del_func	FUNCTION	create_rout_db@localhost	Execute,Alter
> Routine	#
>> +SELECT upgrade_del_func();
>> +upgrade_del_func()
>> +INSIDE upgrade_del_func()
>> +SELECT * FROM mysql.procs_priv;
>> +Host	Db	User	Routine_name	Routine_type	Grantor	Proc_priv	Timestamp
>>
> +localhost	privdb	create_rout_db	upgrade_del_func	FUNCTION	create_rout_db@localhost	Execute,Alter
> Routine	#
>> +SHOW GRANTS FOR 'create_rout_db'@'localhost';
>> +Grants for create_rout_db@localhost
>> +GRANT USAGE ON *.* TO 'create_rout_db'@'localhost' IDENTIFIED BY PASSWORD
> '*08792480350CBA057BDE781B9DF183B263934601'
>> +GRANT CREATE ROUTINE ON `privdb`.* TO 'create_rout_db'@'localhost' WITH GRANT
> OPTION
>> +GRANT EXECUTE, ALTER ROUTINE ON FUNCTION `privdb`.`upgrade_del_func` TO
> 'create_rout_db'@'localhost'
>> +USE privdb;
>> +SHOW CREATE FUNCTION upgrade_del_func;
>> +Function	sql_mode	Create
> Function	character_set_client	collation_connection	Database Collation
>> +upgrade_del_func		CREATE DEFINER=`create_rout_db`@`localhost` FUNCTION
> `upgrade_del_func`() RETURNS char(30) CHARSET latin1
>> +BEGIN
>> +RETURN "INSIDE upgrade_del_func()";
>> +END	latin1	latin1_swedish_ci	latin1_swedish_ci
>> +SELECT upgrade_del_func();
>> +upgrade_del_func()
>> +INSIDE upgrade_del_func()
>> +"Check whether the definer user will be able to execute the replicated routine
> on slave"
>> +USE privdb;
>> +SHOW CREATE FUNCTION upgrade_del_func;
>> +Function	sql_mode	Create
> Function	character_set_client	collation_connection	Database Collation
>> +upgrade_del_func		CREATE DEFINER=`create_rout_db`@`localhost` FUNCTION
> `upgrade_del_func`() RETURNS char(30) CHARSET latin1
>> +BEGIN
>> +RETURN "INSIDE upgrade_del_func()";
>> +END	latin1	latin1_swedish_ci	latin1_swedish_ci
>> +SELECT upgrade_del_func();
>> +upgrade_del_func()
>> +INSIDE upgrade_del_func()
>> +"Test the user who creates a function on master doesn't exist on slave."
>> +DROP USER 'create_rout_db'@'localhost';
>> +CREATE FUNCTION upgrade_alter_func() RETURNS CHAR(30)
>> +BEGIN
>> +RETURN "INSIDE upgrade_alter_func()";
>> +END//
>> +SELECT upgrade_alter_func();
>> +upgrade_alter_func()
>> +INSIDE upgrade_alter_func()
>> +SHOW CREATE FUNCTION upgrade_alter_func;
>> +Function	sql_mode	Create
> Function	character_set_client	collation_connection	Database Collation
>> +upgrade_alter_func		CREATE DEFINER=`create_rout_db`@`localhost` FUNCTION
> `upgrade_alter_func`() RETURNS char(30) CHARSET latin1
>> +BEGIN
>> +RETURN "INSIDE upgrade_alter_func()";
>> +END	latin1	latin1_swedish_ci	latin1_swedish_ci
>> +SELECT upgrade_alter_func();
>> +ERROR HY000: The user specified as a definer ('create_rout_db'@'localhost') does
> not exist
>> +USE privdb;
>> +DROP FUNCTION upgrade_del_func;
>> +DROP FUNCTION upgrade_alter_func;
>> +DROP DATABASE privdb;
>> +DROP USER 'create_rout_db'@'localhost';
>> +"End of test"
>>
>> === modified file 'mysql-test/suite/rpl/t/rpl_do_grant.test'
>> --- a/mysql-test/suite/rpl/t/rpl_do_grant.test	2007-06-27 12:28:02 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_do_grant.test	2009-03-04 14:15:22 +0000
>> @@ -112,3 +112,86 @@ show grants for rpl_do_grant2@localhost;
>>  sync_slave_with_master;
>>  --error 1141
>>  show grants for rpl_do_grant2@localhost;
>> +
>> +#####################################################
>> +# Purpose
>> +# Test whether mysql.procs_priv get replicated 
>> +# Related bugs:
>> +# BUG42217  mysql.procs_priv does not get replicated
>> +#####################################################
>> +connection master; 
>> +
>> +DROP DATABASE IF EXISTS privdb;
>> +CREATE DATABASE  privdb;
>> +
>>     
> SUGGESTION:
>
>   I usually create databases with name like: bug_42217 . It becomes
> easier to understand what is happening if these are left lying around
> (by any reason: test is not cleaning properly, somebody mistakenly
> erases part of the clean up, etc...).
>   
Okay, modified that.
>   
>> +GRANT CREATE ROUTINE ON privdb.* TO 'create_rout_db'@'localhost'
>> +        IDENTIFIED BY 'create_rout_db' WITH GRANT OPTION;
>> +
>> +connect (create_rout_db_master, localhost, create_rout_db, create_rout_db,
> privdb,$MASTER_MYPORT,);
>> +connect (create_rout_db_slave, localhost, create_rout_db, create_rout_db,
> privdb, $SLAVE_MYPORT,);
>> +connection create_rout_db_master;
>> +
>> +
>> +USE privdb;
>> +
>> +DELIMITER //;
>> +CREATE FUNCTION upgrade_del_func() RETURNS CHAR(30)
>> +BEGIN
>> +   RETURN "INSIDE upgrade_del_func()";
>> +END//
>> +
>> +DELIMITER ;//
>> +
>> +connection master;
>> +
>> +USE privdb;
>> +--replace_column 8 #
>> +SELECT * FROM mysql.procs_priv;
>> +SELECT upgrade_del_func();
>> +
>> +sync_slave_with_master;
>> +--replace_column 8 #
>> +SELECT * FROM mysql.procs_priv;
>> +SHOW GRANTS FOR 'create_rout_db'@'localhost';
>> +
>> +USE privdb;
>> +SHOW CREATE FUNCTION upgrade_del_func;
>> +
>> +SELECT upgrade_del_func();
>> +
>> +--echo "Check whether the definer user will be able to execute the replicated
> routine on slave"
>> +connection create_rout_db_slave;
>> +USE privdb;
>> +SHOW CREATE FUNCTION upgrade_del_func;
>> +
>> +SELECT upgrade_del_func();
>> +
>> +connection slave;
>>     
>
> Can you expand your comments on the test below? Maybe it's me, but I
> find it a bit confusing and am not 100% sure what is being tested. 
>   
Okay, expanded comments.
> I think that the case you are testing is: checking that there is no
> definer user on slave, thence SQL thread ACL_GLOBAL privilege jumps in
> and no mysql.procs_priv is inserted. Right?
>   
Right.What you understand absolutely correct. :-)
>   
>> +--echo "Test the user who creates a function on master doesn't exist on slave."
>> +DROP USER 'create_rout_db'@'localhost';
>> +
>> +connection create_rout_db_master;
>> +DELIMITER //;
>> +CREATE FUNCTION upgrade_alter_func() RETURNS CHAR(30)
>> +BEGIN
>> +   RETURN "INSIDE upgrade_alter_func()";
>> +END//
>> +DELIMITER ;//
>> +
>> +connection master;
>> +SELECT upgrade_alter_func();
>> +
>> +sync_slave_with_master;
>> +SHOW CREATE FUNCTION upgrade_alter_func;
>> +--error 1449
>> +SELECT upgrade_alter_func();
>> +
>> +###### CLEAN UP SECTION ##############
>> +connection master;
>> +USE privdb;
>> +DROP FUNCTION upgrade_del_func;
>> +DROP FUNCTION upgrade_alter_func;
>> +DROP DATABASE privdb;
>> +DROP USER 'create_rout_db'@'localhost';
>>     
>
> You should probably close (disconnect) the opened connections as well
> (create_rout_db_master, create_rout_db_slave).
>
>
>   
Okay, thanks.
>> + 
>> +--echo "End of test"
>>
>> === modified file 'sql/sql_parse.cc'
>> --- a/sql/sql_parse.cc	2009-02-16 11:38:15 +0000
>> +++ b/sql/sql_parse.cc	2009-03-04 14:15:22 +0000
>> @@ -4079,6 +4079,8 @@ end_with_restore_list:
>>      uint namelen;
>>      char *name;
>>      int sp_result= SP_INTERNAL_ERROR;
>> +    Security_context security_context;
>> +    bool restore_backup_context= false;
>>  
>>      DBUG_ASSERT(lex->sphead != 0);
>>      DBUG_ASSERT(lex->sphead->m_db.str); /* Must be initialized in the
> parser */
>> @@ -4132,6 +4134,27 @@ end_with_restore_list:
>>      case SP_OK:
>>  #ifndef NO_EMBEDDED_ACCESS_CHECKS
>>        /* only add privileges if really neccessary */
>>     
>
> This did not build on my machine (BUILD/compile-pentium-debug-max). 
>
> The error I get is:
>
> (...)
> sql_parse.cc: In function ‘int mysql_execute_command(THD*)’:
> sql_parse.cc:4181: error: jump to case label
> sql_parse.cc:4139: error:   crosses initialization of ‘LEX_USER*
> definer’
> (...)
>
> I think you may be hitting a problem with the scope of LEX_USER which
> prevents the "switch" to skip over its initialization. You can probably
> solve this by delimiting its scope (inside the "case SP_OK:") using
> curly braces. 
>
>   
Okay, fixed it.
>   
>> +
>> +      Security_context *backup;
>> +      LEX_USER *definer = thd->lex->definer;
>>     
>
> Coding guidelines mandates that there should be no space between
> variable name and "=".
>
> fix:
>   LEX_USER *definer= thd->lex->definer;
>
>   
okay, modified it.
>> +      /*
>> +        Check if the definer exists on slave, 
>> +        then use definer privilege to insert routine privileges to
> mysql.procs_priv.
>> +
>> +        For current user of SQL thread has GLOBAL_ACL privilege, 
>> +        which doesn't any check routine privileges, 
>> +        so no routine privilege record  will insert into mysql.procs_priv.
>> +      */
>> +      if (thd->slave_thread && is_acl_user(definer->host.str,
> definer->user.str))
>> +      {
>> +        security_context.change_security_context(thd, 
>> +                                                
> &thd->lex->definer->user,
>> +                                                
> &thd->lex->definer->host,
>> +                                                
> &thd->lex->sphead->m_db,
>> +                                                 &backup);
>> +        restore_backup_context= true;
>> +      }
>> +
>>        if (sp_automatic_privileges && !opt_noacl &&
>>            check_routine_access(thd, DEFAULT_CREATE_PROC_ACLS,
>>                                 lex->sphead->m_db.str, name,
>> @@ -4143,6 +4166,16 @@ end_with_restore_list:
>>                         ER_PROC_AUTO_GRANT_FAIL,
>>                         ER(ER_PROC_AUTO_GRANT_FAIL));
>>        }
>> +
>> +      /*
>> +        Restore current user with GLOBAL_ACL privilege of SQL thread
>> +      */ 
>> +      if (restore_backup_context)
>> +      {
>> +        DBUG_ASSERT(thd->slave_thread == 1);
>> +        thd->security_ctx->restore_security_context(thd, backup);
>> +      }
>> +
>>  #endif
>>      break;
>>      case SP_WRITE_ROW_FAILED:
>>
>>     
>
>
> Best Regards,
> Luís
>
>   

Thread
bzr commit into mysql-5.1-bugteam branch (gni:2827) Bug#42217Guangbao Ni4 Mar
  • Re: bzr commit into mysql-5.1-bugteam branch (gni:2827) Bug#42217Luís Soares5 Mar
    • Re: bzr commit into mysql-5.1-bugteam branch (gni:2827) Bug#42217Guangbao Ni5 Mar