Hi Kristofer,
OK to push. A few minor comments below.
On 5/29/09 9:36 AM, Kristofer Pettersson wrote:
> #At file:///Users/thek/Development/51-bug44658/ based on
> revid:mats@stripped
>
> 2874 Kristofer Pettersson 2009-05-29
> Bug#44658 Create procedure makes server crash when user does not have ALL
> privilege
>
> MySQL crashes if a user without proper privileges attempts to create a
> procedure.
>
> The crash happens because more than one error state is pushed onto the
> Diagnostic
> area. In this particular case the user is denied to implicitly create a new
> user
> account with the implicitly granted privileges ALTER- and EXECUTE ROUTINE.
>
> The new account is needed if the original user account contained a host mask.
> A user account with a host mask is a distinct user account in this context.
> An alternative would be to first get the most permissive user account which
> include the current user connection and then assign privileges to that
> account. This behavior change is considered out of scope for this bug patch.
>
> The implicit assignment of privileges when a user creates a stored routine is
> a
> considered to be a feature for user convenience and as such it is not
> a critical operation. Any failure to complete this operation is thus
> considered
> non-fatal (an error becomes a warning).
>
> The patch back ports a stack implementation of the internal error handler
> interface.
> This enables the use of multiple error handlers so that it is possible to
> intercept
> and cancel errors thrown by lower layers. This is needed as a error handler
> already
> is used in the call stack emitting the errors which needs to be converted.
> @ mysql-test/r/grant.result
> * Added test case for bug44658
> @ mysql-test/t/grant.test
> * Added test case for bug44658
> @ sql/sp.cc
> * Removed non functional paramter no_error and my_error calls as all errors
paramter -> parameter
> from this function will be converted to a warning anyway.
> * Change function return type from int to bool.
> @ sql/sp.h
> * Removed non functional paramter no_error and my_error calls as all errors
paramter -> parameter
> from this function will be converted to a warning anyway.
> * Changed function return value from int to bool
> @ sql/sql_acl.cc
> * Removed the non functional no_error parameter from the function
> prototype.
> The function is called from two places and in one of the places we now
> ignore errors through error handlers.
> * Introduced the parameter write_to_binlog
> * Introduced an error handler to cancel any error state from
> mysql_routine_grant.
> * Moved my_ok() signal from mysql_routine_grant to make it easier to avoid
> setting the wrong state in the Diagnostic area.
> * Changed the broken error state in sp_grant_privileges() to a warning
> so that if "CREATE PROCEDURE" fails because "Password hash isn't a
> hexidecimal number"
> it is still clear what happened.
> @ sql/sql_acl.h
> * Removed the non functional no_error parameter from the function
> prototype.
> The function is called from two places and in one of the places we now
> ignore errors through error handlers.
> * Introduced the parameter write_to_binlog
> * Changed return type for sp_grant_privileges() from int to bool
> @ sql/sql_class.cc
> * Back ported implementation of internal error handler from 6.0 branch
> @ sql/sql_class.h
> * Back ported implementation of internal error handler from 6.0 branch
> @ sql/sql_parse.cc
> * Moved my_ok() signal from mysql_routine_grant() to make it easier to
> avoid
> setting the wrong state in the Diagnostic area.
>
> modified:
> mysql-test/r/grant.result
> mysql-test/t/grant.test
> sql/sp.cc
> sql/sp.h
> sql/sql_acl.cc
> sql/sql_acl.h
> sql/sql_class.cc
> sql/sql_class.h
> sql/sql_parse.cc
> === modified file 'mysql-test/r/grant.result'
> --- a/mysql-test/r/grant.result 2009-02-25 12:18:24 +0000
> +++ b/mysql-test/r/grant.result 2009-05-29 12:36:19 +0000
> @@ -1358,3 +1358,58 @@ DROP USER 'userbug33464'@'localhost';
> USE test;
> DROP DATABASE dbbug33464;
> SET @@global.log_bin_trust_function_creators=
> @old_log_bin_trust_function_creators;
> +CREATE USER user1;
> +CREATE USER user2;
> +GRANT CREATE ON db1.* TO 'user1'@'localhost';
> +GRANT CREATE ROUTINE ON db1.* TO 'user1'@'localhost';
> +GRANT CREATE ON db1.* TO 'user2'@'%';
> +GRANT CREATE ROUTINE ON db1.* TO 'user2'@'%';
> +FLUSH PRIVILEGES;
> +SHOW GRANTS FOR 'user1'@'localhost';
> +Grants for user1@localhost
> +GRANT USAGE ON *.* TO 'user1'@'localhost'
> +GRANT CREATE, CREATE ROUTINE ON `db1`.* TO 'user1'@'localhost'
> +** Connect as user1 and create a procedure.
> +** The creation will imply implicitly assigned
> +** EXECUTE and ALTER ROUTINE privileges to
> +** the current user user1@localhost.
> +SELECT @@GLOBAL.sql_mode;
> +@@GLOBAL.sql_mode
> +
> +SELECT @@SESSION.sql_mode;
> +@@SESSION.sql_mode
> +
> +CREATE DATABASE db1;
> +CREATE PROCEDURE db1.proc1(p1 INT)
> +BEGIN
> +SET @x = 0;
> +REPEAT SET @x = @x + 1; UNTIL @x> p1 END REPEAT;
> +END ;||
> +** Connect as user2 and create a procedure.
> +** Implicitly assignment of privileges will
> +** fail because the user2@localhost is an
> +** unknown user.
> +CREATE PROCEDURE db1.proc2(p1 INT)
> +BEGIN
> +SET @x = 0;
> +REPEAT SET @x = @x + 1; UNTIL @x> p1 END REPEAT;
> +END ;||
> +Warnings:
> +Warning 1404 Failed to grant EXECUTE and ALTER ROUTINE privileges
> +SHOW GRANTS FOR 'user1'@'localhost';
> +Grants for user1@localhost
> +GRANT USAGE ON *.* TO 'user1'@'localhost'
> +GRANT CREATE, CREATE ROUTINE ON `db1`.* TO 'user1'@'localhost'
> +GRANT EXECUTE, ALTER ROUTINE ON PROCEDURE `db1`.`proc1` TO 'user1'@'localhost'
> +SHOW GRANTS FOR 'user2';
> +Grants for user2@%
> +GRANT USAGE ON *.* TO 'user2'@'%'
> +GRANT CREATE, CREATE ROUTINE ON `db1`.* TO 'user2'@'%'
> +DROP PROCEDURE db1.proc1;
> +DROP PROCEDURE db1.proc2;
> +REVOKE ALL ON db1.* FROM 'user1'@'localhost';
> +REVOKE ALL ON db1.* FROM 'user2'@'%';
> +DROP USER 'user1';
> +DROP USER 'user1'@'localhost';
> +DROP USER 'user2';
> +DROP DATABASE db1;
>
> === modified file 'mysql-test/t/grant.test'
> --- a/mysql-test/t/grant.test 2009-02-10 10:23:47 +0000
> +++ b/mysql-test/t/grant.test 2009-05-29 12:36:19 +0000
> @@ -1471,5 +1471,59 @@ DROP DATABASE dbbug33464;
>
> SET @@global.log_bin_trust_function_creators=
> @old_log_bin_trust_function_creators;
>
> +#
> +# Bug#44658 Create procedure makes server crash when user does not have ALL
> privilege
> +#
> +CREATE USER user1;
> +CREATE USER user2;
> +GRANT CREATE ON db1.* TO 'user1'@'localhost';
> +GRANT CREATE ROUTINE ON db1.* TO 'user1'@'localhost';
> +GRANT CREATE ON db1.* TO 'user2'@'%';
> +GRANT CREATE ROUTINE ON db1.* TO 'user2'@'%';
> +FLUSH PRIVILEGES;
> +SHOW GRANTS FOR 'user1'@'localhost';
> +connect (con1,localhost,user1,,);
> +--echo ** Connect as user1 and create a procedure.
> +--echo ** The creation will imply implicitly assigned
> +--echo ** EXECUTE and ALTER ROUTINE privileges to
> +--echo ** the current user user1@localhost.
> +SELECT @@GLOBAL.sql_mode;
> +SELECT @@SESSION.sql_mode;
> +CREATE DATABASE db1;
> +DELIMITER ||;
> +CREATE PROCEDURE db1.proc1(p1 INT)
> + BEGIN
> + SET @x = 0;
> + REPEAT SET @x = @x + 1; UNTIL @x> p1 END REPEAT;
> + END ;||
> +DELIMITER ;||
> +
> +connect (con2,localhost,user2,,);
> +--echo ** Connect as user2 and create a procedure.
> +--echo ** Implicitly assignment of privileges will
> +--echo ** fail because the user2@localhost is an
> +--echo ** unknown user.
> +DELIMITER ||;
> +CREATE PROCEDURE db1.proc2(p1 INT)
> + BEGIN
> + SET @x = 0;
> + REPEAT SET @x = @x + 1; UNTIL @x> p1 END REPEAT;
> + END ;||
> +DELIMITER ;||
> +
> +connection default;
> +SHOW GRANTS FOR 'user1'@'localhost';
> +SHOW GRANTS FOR 'user2';
> +disconnect con1;
> +disconnect con2;
> +DROP PROCEDURE db1.proc1;
> +DROP PROCEDURE db1.proc2;
> +REVOKE ALL ON db1.* FROM 'user1'@'localhost';
> +REVOKE ALL ON db1.* FROM 'user2'@'%';
> +DROP USER 'user1';
> +DROP USER 'user1'@'localhost';
> +DROP USER 'user2';
> +DROP DATABASE db1;
> +
> # Wait till we reached the initial number of concurrent sessions
> --source include/wait_until_count_sessions.inc
>
> === modified file 'sql/sp.cc'
> --- a/sql/sp.cc 2009-04-08 23:42:51 +0000
> +++ b/sql/sp.cc 2009-05-29 12:36:19 +0000
> @@ -1308,13 +1308,21 @@ sp_find_routine(THD *thd, int type, sp_n
> /**
> This is used by sql_acl.cc:mysql_routine_grant() and is used to find
> the routines in 'routines'.
> +
> + @param thd Thread handler
> + @param routines List of needles in the hay stack
> + @param any Any of the needles are good enough
> +
> + @return
> + @retval FALSE Found.
> + @retval TRUE Not found
> */
>
> -int
> -sp_exist_routines(THD *thd, TABLE_LIST *routines, bool any, bool no_error)
> +bool
> +sp_exist_routines(THD *thd, TABLE_LIST *routines, bool any)
> {
> TABLE_LIST *routine;
> - bool result= 0;
> + bool result= FALSE;
> bool sp_object_found;
> DBUG_ENTER("sp_exists_routine");
> for (routine= routines; routine; routine= routine->next_global)
> @@ -1336,18 +1344,14 @@ sp_exist_routines(THD *thd, TABLE_LIST *
> if (sp_object_found)
> {
> if (any)
> - DBUG_RETURN(1);
> - result= 1;
> + DBUG_RETURN(FALSE);
> + result= FALSE;
No need to assign as there isn't a place that sets result to TRUE.
Actually, no need to have the result variable at all...
> }
> else if (!any)
> {
> - if (!no_error)
> - {
> - my_error(ER_SP_DOES_NOT_EXIST, MYF(0), "FUNCTION or PROCEDURE",
> - routine->table_name);
> - DBUG_RETURN(-1);
> - }
> - DBUG_RETURN(0);
> + my_error(ER_SP_DOES_NOT_EXIST, MYF(0), "FUNCTION or PROCEDURE",
> + routine->table_name);
> + DBUG_RETURN(TRUE);
> }
> }
> DBUG_RETURN(result);
>
Otherwise, looks good. Thanks.
Regards,
-- Davi Arnaut
