Hello Sergei, Thanks for your review. Here's a new version of the patch. Please see comments below: On 03/06/2017 10:50 AM, Sergei Golubchik wrote:
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?
Agreed. This version fixes around 16 classes that had the same problem, and which only needed to swap "str" and "tmp_value" in their val_str() implementations. As agreed on slack, the following three classes: - Item_str_conv - Item_func_make_set - Item_char_typecast with more complex implementations will be fixed separately. I'll create separate MDEVs for these three classes.
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.
Can you please suggest proper comments and proper places for them? I got stuck with that :)
Ideally, we'd have some way to enforce it, but I couldn't think of anything.
As discussed on slack, we can try something like this: String *res= expr->val_str(str); DBUG_ASSERT(!res || res == str || res->alloced_length() == 0); in a few top-level places (e.g. protocol), but after the remaining three classes are fixed. So in the current patch I'm not adding any enforcing code. Thanks.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org