maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Mar 03, Alexander Barkov wrote:
> Hello Serg and Sergey,
> 
> Serg, please review a new version of the patch.
> 
> 
> Sergey, thanks for noticing the problem with a missing
> res->is_alloced() part in the condition. This fixed the
> original problem reported in the bug.
> 
> I also had to modify Item_func_conv_charset::val_str(),
> to make this query work well:
> 
> SELECT CONCAT(t2,'-',t2) c2 FROM (SELECT CONVERT(t USING latin1) t2 FROM
> t1) sub;
> 
> (it's reported in an additional comment in MDEV).
> 
> This additional change prevents Item_func_concat from modifying
> then buffer owned by Item_func_conv_charset.
> 
> diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
> index 4ea3075..f5a925f 100644
> --- a/sql/item_strfunc.cc
> +++ b/sql/item_strfunc.cc
> @@ -671,7 +671,8 @@ String *Item_func_concat::val_str(String *str)
>  			    current_thd->variables.max_allowed_packet);
>  	goto null;
>        }
> -      if (!is_const && res->alloced_length() >= res->length()+res2->length())
> +      if (!is_const && res->alloced_length() >= res->length()+res2->length() &&
> +          res->is_alloced() /*psergey-added*/)

I think this is wrong and the comment in item.h is wrong too.
(explained in another mail, but in short String allocates the memory
automatically as needed, it's not something the caller can control)

>        {						// Use old buffer
>  	res->append(*res2);
>        }
> @@ -3378,16 +3379,16 @@ String *Item_func_conv_charset::val_str(String *str)
>    DBUG_ASSERT(fixed == 1);
>    if (use_cached_value)
>      return null_value ? 0 : &str_value;
> -  String *arg= args[0]->val_str(str);
> +  String *arg= args[0]->val_str(&tmp_value);
>    uint dummy_errors;
>    if (args[0]->null_value)
>    {
>      null_value=1;
>      return 0;
>    }
> -  null_value= tmp_value.copy(arg->ptr(), arg->length(), arg->charset(),
> -                             conv_charset, &dummy_errors);
> -  return null_value ? 0 : check_well_formed_result(&tmp_value);
> +  null_value= str->copy(arg->ptr(), arg->length(), arg->charset(),
> +                        conv_charset, &dummy_errors);
> +  return null_value ? 0 : check_well_formed_result(str);
>  }

That's a good idea. I'd expect it to fix the bug for both queries, with
and without SUBSTR. Hmm, not sure about SUBSTR, but then it'll be
Item_func_substr to fix.

But (sorry) you're fixing only Item_func_conv_charset.
Is it the only Item that stores the result in its internal buffer
(tmp_value or str_value)?  Any other item that does it will cause the
same bug.

>  
>  void Item_func_conv_charset::fix_length_and_dec()

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx