maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Mar 02, Sergey Petrunia wrote:
> 
> My take on the issue: I read the comment for Item::val_str:
> 
>       The caller of this method can modify returned string, but only in case
>       when it was allocated on heap, (is_alloced() is true).  This allows
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^(*)
>       the caller to efficiently use a buffer allocated by a child without
>       having to allocate a buffer of it's own. The buffer, given to
>       val_str() as argument, belongs to the caller and is later used by the
>       caller at it's own choosing.

This rule, as stated, is invalid, it's not something we can do.

String allocates and reallocates automatically. One can create a string
with a buffer on stack and it'll move itself on the heap (is_alloced()
will be true) if one would try to store there more than a buffer can hold.

There's no way one can reliably control whether a String is allocated on
heap.

> And I see that Item_func_concat::val_str violates the rule (*) here:
> 
> =>    if (!is_const && res->alloced_length() >= res->length()+res2->length())
>       {						// Use old buffer
> 	res->append(*res2);
>
> That is, the buffer is "not alloced", but the code modifies it by
> calling append().
> 
> So, I can get this bug' testcase to pass with this patch:
> 
> diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
> index 4ea3075..2e2cecd 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*/)
>        {                                                // Use old buffer
>         res->append(*res2);
>        }

This only works because SUBSTR creates an intermediate String with
pointers into the problematic String of Item_conv_charset. That
intermediate String is not allocated.

If you'd modify the test case to use CONCAT direcly on
Item_conv_charset, this fix won't help.

The rule that could work is not to reuse a string if it is "const" in
the String::mark_as_const() sense. I'm 100% sure it's safe, but
it's something that the item can control in its val_str.

But it's not clear when an item should mark its return String value as
"const".

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx