[Maria-developers] Please review MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
Hello Sergei, can you please review a fix for MDEV-11134. I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table(). But perhaps it can be done in different ways: 1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false. or 2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1) Thanks!
Hi Sergei, On 12/28/2016 08:01 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Dec 26, Alexander Barkov wrote:
Hello Sergei,
can you please review a fix for MDEV-11134.
Sure. Where is it? :)
Sorry, forgot to attach it.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! On Dec 26, Alexander Barkov wrote:
Hello Sergei,
can you please review a fix for MDEV-11134.
I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table().
I thought that normally basic const items are always fixed. So, Item_param is an exception? It's basic const, but not fixed?
But perhaps it can be done in different ways:
1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false.
or
2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1)
Yes, I think it's reasonable. It'll make Item_param behave as other basic constants do. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hello Sergei, On 12/29/2016 05:33 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Dec 26, Alexander Barkov wrote:
Hello Sergei,
can you please review a fix for MDEV-11134.
I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table().
I thought that normally basic const items are always fixed. So, Item_param is an exception? It's basic const, but not fixed?
But perhaps it can be done in different ways:
1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false.
or
2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1)
Yes, I think it's reasonable. It'll make Item_param behave as other basic constants do.
Please find a new version attached. It makes sure that Item_param::fixed, Item_param::state and Item_param::item_type are in sync to each other. Thanks!
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! On Jan 11, Alexander Barkov wrote:
commit adf6bb91e349e8983b146cbfcaaa002edff54fe5 Author: Alexander Barkov <bar@mariadb.org> Date: Wed Jan 11 13:42:52 2017 +0400
MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
Problem: Item_param::basic_const_item() returned true when fixed==false. This unexpected combination made Item::const_charset_converter() crash on asserts.
Fix: - Changing all Item_param::set_xxx() to set "fixed" to true. This fixes the problem. - Additionally, changing all Item_param::set_xxx() to set Item_param::item_type, to avoid duplicate code, and for consistency, to make the code symmetric between different constant types. Before this patch only set_null() set item_type. - Moving Item_param::state and Item_param::item_type from public to private, to make sure easier that these members are in sync with "fixed" and to each other. - Adding a new argument "unsigned_arg" to Item::set_decimal(), and reusing it in two places instead of duplicate code. - Adding a new method Item_param::fix_temporal() and reusing it in two places. - Adding methods is_no_value(), is_long_data_value(), is_int_value(), instead of direct access to Item_param::state.
Looks ok to push. Stylistic comments: * too many fixed= true in all set_* methods. I'd rather added a method fix_type(), to be used like - type= Item::STRING_ITEM + fix_type(Item::STRING_ITEM) and in this method I'd { type=type_arg; fixed= true }. * names like is_no_value/is_int_value/etc look strange, I'd use has_no_value, has_int_value, etc Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On 01/23/2017 07:50 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jan 11, Alexander Barkov wrote:
commit adf6bb91e349e8983b146cbfcaaa002edff54fe5 Author: Alexander Barkov <bar@mariadb.org> Date: Wed Jan 11 13:42:52 2017 +0400
MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
Problem: Item_param::basic_const_item() returned true when fixed==false. This unexpected combination made Item::const_charset_converter() crash on asserts.
Fix: - Changing all Item_param::set_xxx() to set "fixed" to true. This fixes the problem. - Additionally, changing all Item_param::set_xxx() to set Item_param::item_type, to avoid duplicate code, and for consistency, to make the code symmetric between different constant types. Before this patch only set_null() set item_type. - Moving Item_param::state and Item_param::item_type from public to private, to make sure easier that these members are in sync with "fixed" and to each other. - Adding a new argument "unsigned_arg" to Item::set_decimal(), and reusing it in two places instead of duplicate code. - Adding a new method Item_param::fix_temporal() and reusing it in two places. - Adding methods is_no_value(), is_long_data_value(), is_int_value(), instead of direct access to Item_param::state.
Looks ok to push.
Stylistic comments: * too many fixed= true in all set_* methods. I'd rather added a method fix_type(), to be used like
- type= Item::STRING_ITEM + fix_type(Item::STRING_ITEM)
and in this method I'd { type=type_arg; fixed= true }.
* names like is_no_value/is_int_value/etc look strange, I'd use has_no_value, has_int_value, etc
Thanks! Addressed your suggestions and pushed.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Alexander Barkov
-
Sergei Golubchik