maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Vicentiu,

The patch looks fine. Thanks!

I have only small comments below:

> 
> [Commits] 974064cde3c: MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view
> vicentiu vicentiu at mariadb.org
> Thu Jun 8 10:11:38 EEST 2017
> 
>     Previous message (by thread): [Commits] 7f2d3c3: MDEV-11084 Select statement with partition selection against MyISAM table opens all partitions.
>     Next message (by thread): [Commits] d03abc71a49: MDEV-12609: Allow suppression of InnoDB log messages about reserving extents
>     Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
> 
> revision-id: 974064cde3c1cd1be9157bc11ce410604807f68b (mariadb-10.0.31-2-g974064cde3c)
> parent(s): 54cd5bc703da08be45894495d7f3e2c8117617cd
> author: Vicențiu Ciorbaru
> committer: Vicențiu Ciorbaru
> timestamp: 2017-06-08 10:10:07 +0300
> message:
> 
> MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view
> 
> The problem lies in how CURRENT_ROLE is defined. The
> Item_func_current_role inherits from Item_func_sysconst, which defines
> a safe_charset_converter to be a const_charset_converter.
> 
> During view creation, if there is no role previously set, the current_role()
> function returns NULL.
> 
> This is captured on item instantiation and the
> const_charset_converter call subsequently returns an Item_null.
> In turn, the function is replaced with Item_null and the view is
> then created with an Item_null instead of Item_func_current_role.
> 
> Without this patch, the first SHOW CREATE VIEW from the testcase would
> have a where clause of WHERE role_name = NULL, while the second SHOW
> CREATE VIEW would show a correctly created view.
> 
> The same applies for the DATABASE function, as it can change as well.
> 
> There is an additional problem with CURRENT_ROLE() when used in a
> prepared statement. During prepared statement creation we used to set
> the string_value of the function to the current role as well as the
> null_value flag. During execution, if CURRENT_ROLE was not null, the
> null_value flag was never set to not-null during fix_fields.
> 
> Item_func_current_user however can never be NULL so it did not show this
> problem in a view before. At the same time, the CURRENT_USER() can not
> be changed between prepared statement execution and creation so the
> implementation where the value is stored during fix_fields is
> sufficient.
> 
> Note also that DATABASE() function behaves differently during prepared
> statements. See bug 25843 for details or commit
> 7e0ad09edff587dadc3e9855fc81e1b7de8f2199
> 

<cut>

> --- a/sql/item_strfunc.cc
> +++ b/sql/item_strfunc.cc
> @@ -2344,6 +2344,7 @@ String *Item_func_database::val_str(String *str)
>    }
>    else
>      str->copy(thd->db, thd->db_length, system_charset_info);
> +  null_value= 0;
>    return str;
>  }
>  
> @@ -2378,6 +2379,30 @@ bool Item_func_user::init(const char *user, const char *host)
>  }
>  
>  
> +Item *Item_func_sysconst::safe_charset_converter(CHARSET_INFO *tocs)
> +{
> +  /*
> +   During view or prepared statement creation, the item should not
> +   make use of const_charset_converter as it would imply substitution
> +   with constant items which is not correct. Functions can have different
> +   values during view creation and view execution based on context.
> +
> +   Return the identical item during view creation and prepare.
> +  */
> +  if (current_thd->lex->context_analysis_only &
> +      (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW))
> +    return this;
> +  return const_charset_converter(tocs, true, fully_qualified_func_name());

I propose to avoid code duplication and do like this:


  if (!Item_func_sysconst::const_item())
    return this;
  return return const_charset_converter(tocs, true,
fully_qualified_func_name());


> +}
> +
> +bool Item_func_sysconst::const_item() const
> +{
> +  if (current_thd->lex->context_analysis_only &
> +      (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW))
> +    return false;
> +  return true;
> +}
> +
>  bool Item_func_user::fix_fields(THD *thd, Item **ref)
>  {
>    return (Item_func_sysconst::fix_fields(thd, ref) ||
> @@ -2403,20 +2428,25 @@ bool Item_func_current_role::fix_fields(THD *thd, Item **ref)
>  
>    Security_context *ctx= context->security_ctx
>                            ? context->security_ctx : thd->security_ctx;
> -
>    if (ctx->priv_role[0])
>    {
>      if (str_value.copy(ctx->priv_role, strlen(ctx->priv_role),
>                         system_charset_info))
>        return 1;
> -
>      str_value.mark_as_const();
> +    null_value= maybe_null= 0;
>      return 0;
>    }
>    null_value= maybe_null= 1;
>    return 0;
>  }
>  
> +String* Item_func_current_role::val_str(String * str)
> +{
> +  DBUG_ASSERT(fixed == 1);
> +  return null_value ? NULL : &str_value;
> +}
> +

Why did you move val_str() from *.h to *.cc ?
The method is just two lines, I'd leave it in *.h

<cut>