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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
