maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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 security@xxxxxxxxxxx
>