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