maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Mar 06, Alexander Barkov wrote:
> >> @@ -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.

Great!

> > 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?

I'd say yes. What do you think?

And, by the way, could you also fix the comment in item.h around
Item::val_str(String *) not to say that it can only change allocated
strings? May be it should say about const (String::make_const) strings
instead. Thanks. Also you could add something about this your CONCAT
fix. Like, the item can use internal buffer as a temporary storage,
but the result should be in the String argument, not the other way
around. Ideally, we'd have some way to enforce it, but I couldn't think
of anything.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx