Hello Sergei,
On 03/04/2017 12:12 AM, Sergei Golubchik wrote:
> 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.
I tested this change alone, without adding the "res->is_alloced()" part.
It fixes 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.
There are more Items:
- around 16 doing "return &tmp_value;"
- around 4 doing "return &str_value;"
Here's an example of another bad result:
DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (t VARCHAR(10) CHARSET latin1);
INSERT INTO t1 VALUES('1234567');
SELECT CONCAT(t2,'-',t2) c2 FROM (SELECT REVERSE(t) t2 FROM t1) sub;
+----------------+
| c2 |
+----------------+
| 76543217654321 |
+----------------+
It should be '7654321-7654321' instead.
Should we fix all of them under terms of MDEV-10306?
>
>>
>> void Item_func_conv_charset::fix_length_and_dec()
>
> 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
