List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 28 2009 11:43am
Subject:Re: bzr commit into mysql-5.1-bugteam branch
(kristofer.pettersson:2874) Bug#44658
View as plain text  
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
Thread
bzr commit into mysql-5.1-bugteam branch (kristofer.pettersson:2874)Bug#44658Kristofer Pettersson28 May
  • Re: bzr commit into mysql-5.1-bugteam branch(kristofer.pettersson:2874) Bug#44658Davi Arnaut28 May