Hi Andrei,
Thanks for your quick review.
Some replied inline below.
Best wishes,
/Guangbao
Andrei Elkin Wrote:
> Guang Bao, hello.
>
> The patch looks convincing.
>
> I only have one minor note and some suggestion to comments' wording
> (really I hate offering that not being a native :-)
>
> Please keep width of your cset comments within 80 cols. It's a bit
> difficult to edit lines longer than the standard.
>
Sorry for my poor english writting, I will try to write it better later. :-(
Thanks.
>
>> #At file:///home/ngb/mysql/bzr/bug42217-5.1-bugteam/
>>
>> 2827 Guangbao Ni 2009-02-27
>> Bug #42217 mysql.procs_priv does not get replicated
>>
>>
>> mysql.procs_priv does not get replicated.
>>
>
> ok
>
>
>> Yet mysql.procs_priv table operations don't have correspoding
>> statement replicated to slave.
>>
>
> The above clause is not correct.
> Actually, all necessary data have been available on the slave.
>
Okay, what I mean is the privilege insert operation should be triggered
by create routine statement automatically.
Only if the create routine statement is replicated to slave, then the
privilege operation should executed successfully on slave.
>
>> the operations for mysql.procs_priv is triggered by creating
>> function/procedure statements according to current user's privileges.
>>
>
> Should not it be just said that `procs_priv' did not record the
> definer's name?
>
No.
'procs_priv' not only didn't record the definer's name, but also didn't
record the whole record of procs_priv.
>
>> Because the current user of SQL thread has GLOBAL_ACL, so needn't check
> mysql.procs_priv privilege
>> when create routines.
>>
>
> s/ needn't check / does not need any check /
>
Okay, thanks!
>
>> Corresponding GLOBAL_ACL privilege doesn't need to insert routine privilege
>> into mysql.procs_priv.
>>
>> Fxied by switch current user of SQL thread to definer user (if
>> the definer user existes on slave,
>>
>
> typo Fxied ^
> s / existes / exists /
>
> Also, i am not sure if you need any parenthesis.
> Why not just
>
> 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.
>
> ?
>
>
Okay, thanks.
>> or current user of SQL thread is still used) when checking whether the
> routine privilege is needed
>> to insert mysql.procs_priv table or not.
>>
>
>
>
>> 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,
>> or current user of SQL thread is still used) 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-02-27 21:16:12 +0000
>> @@ -89,3 +89,62 @@ 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;
>> +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()
>> +"Test the user who create 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-02-27 21:16:12 +0000
>> @@ -112,3 +112,76 @@ 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;
>> +
>> +GRANT CREATE ROUTINE ON privdb.* TO 'create_rout_db'@'localhost'
>> + IDENTIFIED BY 'create_rout_db' WITH GRANT OPTION;
>> +
>> +connect (create_rout_db, localhost, create_rout_db, create_rout_db, privdb);
>> +connection create_rout_db;
>> +
>> +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 "Test the user who create a function on master doesn't exist on slave."
>>
>
> s / create / creates /
>
>
Okay, fixed it.
>> +DROP USER 'create_rout_db'@'localhost';
>> +
>> +connection create_rout_db;
>> +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';
>> +
>> +--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-02-27 21:16:12 +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,26 @@ end_with_restore_list:
>> case SP_OK:
>> #ifndef NO_EMBEDDED_ACCESS_CHECKS
>> /* only add privileges if really neccessary */
>> +
>> + Security_context *backup;
>> + LEX_USER *definer = thd->lex->definer;
>> + /*
>> + 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 needn't
> check
>> + routine privileges, so no routine privileges 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 +4165,13 @@ 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 (thd->slave_thread && restore_backup_context)
>>
>
> This is the minor note. As `restore_backup_context' is changed ony
> with slave_thread, the if can be as simple as
>
> + if (restore_backup_context)
> + DBUG_ASSERT(thd->slave_thread == 1);
>
> and it'd be good to assert.
>
>
>
Good, fixed.
>> + thd->security_ctx->restore_security_context(thd, backup);
>> +
>> #endif
>> break;
>> case SP_WRITE_ROW_FAILED:
>>
>>
>>
>
> Please mail me your resolution and I'll approve technically on the bug
> page without a delay.
>
>
Okay, I will commit a patch as quick as possible after fixed it.
Thank you very much.
> cheers,
>
> Andrei
>
