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@mariadb.org