Hi Kristofer,
On 5/28/09 7:36 AM, Kristofer Pettersson wrote:
> #At file:///Users/thek/Development/51-bug44658/ based on
> revid:mats@stripped
>
> 2874 Kristofer Pettersson 2009-05-28
> 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.
> @ mysql-test/r/grant.result
> * Added test case for bug44658
> @ mysql-test/t/grant.test
> * Added test case for bug44658
> @ 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 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.
> @ 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.
> @ 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.
>
[..]
>
> if (!revoke_grant)
> {
> - if (sp_exist_routines(thd, table_list, is_proc, no_error)<0)
> + if (sp_exist_routines(thd, table_list, is_proc, FALSE)<0)
Please also drop the no_error parameter from sp_exist_routines, this
function is the only caller.
> DBUG_RETURN(TRUE);
> }
>
> @@ -3317,9 +3315,8 @@ bool mysql_routine_grant(THD *thd, TABLE
> {
> if (revoke_grant)
> {
> - if (!no_error)
> - my_error(ER_NONEXISTING_PROC_GRANT, MYF(0),
> - Str->user.str, Str->host.str, table_name);
> + my_error(ER_NONEXISTING_PROC_GRANT, MYF(0),
> + Str->user.str, Str->host.str, table_name);
> result= TRUE;
> continue;
> }
> @@ -3344,16 +3341,13 @@ bool mysql_routine_grant(THD *thd, TABLE
> }
> thd->mem_root= old_root;
> pthread_mutex_unlock(&acl_cache->lock);
> - if (!result&& !no_error)
> + if (!result)
> {
> write_bin_log(thd, TRUE, thd->query, thd->query_length);
Hum.. we might log the create procedure twice..
Perhaps would it make sense to tear mysql_routine_grant apart and
isolate the parts specific to grants to a new local function?
Something along the lines of:
static bool grant_routine_privs(..)
{
// whatever is necessary to make grants
}
bool mysql_routine_grant()
{
// stuff only associated with a SQLCOM_GRANT
if (rights & ~PROC_ACLS)
..
if (!revoke_grant)
..
...
grant_routine_privs(..)
...
if (!result)
write_bin_log(..)
}
int sp_grant_privileges()
{
...
thd->push_internal_handler(..);
grant_routine_priv();
thd->pop_internal_handler(..);
}
> }
>
> rw_unlock(&LOCK_grant);
>
> - if (!result&& !no_error)
> - my_ok(thd);
> -
> /* Tables are automatically closed */
> DBUG_RETURN(result);
> }
> @@ -6174,6 +6168,7 @@ int sp_grant_privileges(THD *thd, const
> bool result;
> ACL_USER *au;
> char passwd_buff[SCRAMBLED_PASSWORD_CHAR_LENGTH+1];
> + Dummy_error_handler error_handler;
> DBUG_ENTER("sp_grant_privileges");
>
> if (!(combo=(LEX_USER*) thd->alloc(sizeof(st_lex_user))))
> @@ -6224,7 +6219,6 @@ int sp_grant_privileges(THD *thd, const
> }
> else
> {
> - my_error(ER_PASSWD_LENGTH, MYF(0), SCRAMBLED_PASSWORD_CHAR_LENGTH);
OK.
> return -1;
> }
> combo->password.str= passwd_buff;
> @@ -6241,8 +6235,14 @@ int sp_grant_privileges(THD *thd, const
> thd->lex->ssl_type= SSL_TYPE_NOT_SPECIFIED;
> bzero((char*)&thd->lex->mqh, sizeof(thd->lex->mqh));
>
> + /*
> + Only care about whether the operation failed or succeeded
> + as all errors will be handled later.
> + */
> + thd->push_internal_handler(&error_handler);
> result= mysql_routine_grant(thd, tables, is_proc, user_list,
> - DEFAULT_CREATE_PROC_ACLS, 0, 1);
> + DEFAULT_CREATE_PROC_ACLS, 0);
> + thd->pop_internal_handler();
> DBUG_RETURN(result);
> }
>
>
> === modified file 'sql/sql_acl.h'
> --- a/sql/sql_acl.h 2008-03-21 15:48:28 +0000
> +++ b/sql/sql_acl.h 2009-05-28 10:36:23 +0000
> @@ -233,7 +233,7 @@ int mysql_table_grant(THD *thd, TABLE_LI
> bool revoke);
> bool mysql_routine_grant(THD *thd, TABLE_LIST *table, bool is_proc,
> List<LEX_USER> &user_list, ulong rights,
> - bool revoke, bool no_error);
> + bool revoke);
> my_bool grant_init();
> void grant_free(void);
> my_bool grant_reload(THD *thd);
>
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc 2009-05-10 16:20:35 +0000
> +++ b/sql/sql_class.cc 2009-05-28 10:36:23 +0000
> @@ -674,31 +674,40 @@ THD::THD()
>
> void THD::push_internal_handler(Internal_error_handler *handler)
> {
> - /*
> - TODO: The current implementation is limited to 1 handler at a time only.
> - THD and sp_rcontext need to be modified to use a common handler stack.
> - */
> - DBUG_ASSERT(m_internal_handler == NULL);
> - m_internal_handler= handler;
> + if(m_internal_handler)
> + {
> + handler->m_prev_internal_handler= m_internal_handler;
> + m_internal_handler= handler;
> + }
> + else
> + {
> + m_internal_handler= handler;
> + }
> }
>
>
> bool THD::handle_error(uint sql_errno, const char *message,
> MYSQL_ERROR::enum_warning_level level)
> {
> - if (m_internal_handler)
> + if (!m_internal_handler)
> + return FALSE;
> +
> + for(Internal_error_handler *error_handler= m_internal_handler;
Coding style: for(
Please review for other occurrences..
> + error_handler;
> + error_handler= m_internal_handler->m_prev_internal_handler)
> {
> - return m_internal_handler->handle_error(sql_errno, message, level, this);
> + if (error_handler->handle_error(sql_errno, message, level, this))
> + return TRUE;
> }
>
> - return FALSE; // 'FALSE', as per coding style
> + return FALSE;
> }
>
Regards,
-- Davi Arnaut
