Re: [Maria-developers] Review request for a fix for MDEV-5506
Hi, Alexander! Few questions, see below.
=== modified file 'mysql-test/t/timezone2.test' --- mysql-test/t/timezone2.test 2013-08-08 08:58:28 +0000 +++ mysql-test/t/timezone2.test 2014-01-23 10:34:46 +0000 @@ -298,5 +298,11 @@ SELECT CONVERT_TZ(TIME('00:00:00'),'+00: SELECT CONVERT_TZ(TIME('2010-01-01 00:00:00'),'+00:00','+7:5');
--echo # +--echo # MDEV-5506 safe_mutex: Trying to lock unitialized mutex at safemalloc.c on server shutdown after SELECT with CONVERT_TZ +--echo # +SELECT CONVERT_TZ('2001-10-08 00:00:00', MAKE_SET(0,'+01:00'), '+00:00' );
hmm, the bug description was "at shutdown". I don't see a shutdown in your test case. I suppose, you assume that the server will be eventually shut down and that'll do. But perhaps it needs to be shut down immediately, and if mtr will do a lot of work in this server the test case becomes invalid - it won't crash anymore?
=== modified file 'sql/item_strfunc.cc' --- sql/item_strfunc.cc 2013-09-25 12:30:13 +0000 +++ sql/item_strfunc.cc 2014-01-23 10:17:57 +0000 @@ -43,7 +43,7 @@ C_MODE_END /** @todo Remove this. It is not safe to use a shared String object. */
I guess, you've fixed that and can remove the comment.
-String my_empty_string("",default_charset_info); +String_const my_empty_string("", default_charset_info);
=== modified file 'sql/sql_string.h' --- sql/sql_string.h 2012-11-09 08:11:20 +0000 +++ sql/sql_string.h 2014-01-23 10:26:13 +0000 @@ -56,25 +56,52 @@ uint convert_to_printable(char *to, size
class String { + typedef enum + { + STRING_FLAG_NONE= 0, + STRING_FLAG_ALLOCED= 1, + STRING_FLAG_READ_ONLY= 2, + STRING_FLAG_NULL_TERMINATED= 4 + } flag_t; char *Ptr; uint32 str_length,Alloced_length, extra_alloc; - bool alloced; + flag_t flags; CHARSET_INFO *str_charset;
Okay, I understand what you did. But I don't understand why. What was the problem? Why there was a crash? Regards, Sergei
Hi Sergei, On 01/26/2014 11:23 PM, Sergei Golubchik wrote:
Hi, Alexander!
Few questions, see below.
=== modified file 'mysql-test/t/timezone2.test' --- mysql-test/t/timezone2.test 2013-08-08 08:58:28 +0000 +++ mysql-test/t/timezone2.test 2014-01-23 10:34:46 +0000 @@ -298,5 +298,11 @@ SELECT CONVERT_TZ(TIME('00:00:00'),'+00: SELECT CONVERT_TZ(TIME('2010-01-01 00:00:00'),'+00:00','+7:5');
--echo # +--echo # MDEV-5506 safe_mutex: Trying to lock unitialized mutex at safemalloc.c on server shutdown after SELECT with CONVERT_TZ +--echo # +SELECT CONVERT_TZ('2001-10-08 00:00:00', MAKE_SET(0,'+01:00'), '+00:00' );
hmm, the bug description was "at shutdown". I don't see a shutdown in your test case. I suppose, you assume that the server will be eventually shut down and that'll do. But perhaps it needs to be shut down immediately, and if mtr will do a lot of work in this server the test case becomes invalid - it won't crash anymore?
The crash in fact does not repeat if there is no immediate shutdown. But the real problem was not in shutdown itself - it was in realloc() with a string which is not supposed to be realloced. I have added DBUG_ASSERT() into String::realloc() which covers the problem. I thought that adding a separate test exactly as reported is not worthy, as every new tests slows down "mtr" execution.
=== modified file 'sql/item_strfunc.cc' --- sql/item_strfunc.cc 2013-09-25 12:30:13 +0000 +++ sql/item_strfunc.cc 2014-01-23 10:17:57 +0000 @@ -43,7 +43,7 @@ C_MODE_END /** @todo Remove this. It is not safe to use a shared String object. */
I guess, you've fixed that and can remove the comment.
Currently it always returns latin1, which potentially can break something when the sessions character set is not latin1 (but I can't make up an example though). So the comment is still valid - it is still somewhat potentially "unsafe", but for a different reason. I will extend the comment to tell about the potential character set issue. A safer way would be to: - either move my_empty_string as a member in THD and follow changes in @@collation_connection - or just have the argument to val_str() being populated to an empty value every time.
-String my_empty_string("",default_charset_info); +String_const my_empty_string("", default_charset_info);
=== modified file 'sql/sql_string.h' --- sql/sql_string.h 2012-11-09 08:11:20 +0000 +++ sql/sql_string.h 2014-01-23 10:26:13 +0000 @@ -56,25 +56,52 @@ uint convert_to_printable(char *to, size
class String { + typedef enum + { + STRING_FLAG_NONE= 0, + STRING_FLAG_ALLOCED= 1, + STRING_FLAG_READ_ONLY= 2, + STRING_FLAG_NULL_TERMINATED= 4 + } flag_t; char *Ptr; uint32 str_length,Alloced_length, extra_alloc; - bool alloced; + flag_t flags; CHARSET_INFO *str_charset;
Okay, I understand what you did. But I don't understand why. What was the problem? Why there was a crash?
- Item_func_make_set::val_str() returns &my_empty_string - Item_func_convert_tz::get_date() calls my_tz_find(&my_empty_string). - my_tz_find() calls name->c_ptr_safe(&my_empty_string). my_empty_string gets erroneously realloced. - shutdown deinitializes all mutexes, including THR_LOCK_malloc. - Destructor String::~String() calls my_free(my_empty_string.Ptr), which crashes on non-initialized mutex THR_LOCK_malloc After the fix my_empty_string: - remembers that it is null terminated and does not call realloc() from c_ptr_safe() - remembers that it is read-only and protects itself from realloc() by DBUG_ASSERT().
Regards, Sergei
Hi, Alexander! On Jan 27, Alexander Barkov wrote:
On 01/26/2014 11:23 PM, Sergei Golubchik wrote:
Okay, I understand what you did. But I don't understand why. What was the problem? Why there was a crash?
- Item_func_make_set::val_str() returns &my_empty_string ... After the fix my_empty_string: - remembers that it is null terminated and does not call realloc() from c_ptr_safe() - remembers that it is read-only and protects itself from realloc() by DBUG_ASSERT().
I'd rather fix Item_func_make_set instead. Now it seems to be the only Item that uses it (there's also SELECT ... OUTFILE, but that's different). And Item_func_make_set has String tmp_str property, which is already used as an internal storage. So, I'd remove my_empty_string usage from Item_func_make_set whatsoever. Simply with - String *result=&my_empty_string; + String *result=&tmp_str; + tmp_str.length(0); In fact, perhaps Item_func_make_set::tmp_str is not needed at all, and Item:str_value can be used instead: - String *result=&my_empty_string; + String *result=make_empty_result(); Regards, Sergei
Hi Sergei, On 01/27/2014 05:52 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jan 27, Alexander Barkov wrote:
On 01/26/2014 11:23 PM, Sergei Golubchik wrote:
Okay, I understand what you did. But I don't understand why. What was the problem? Why there was a crash?
- Item_func_make_set::val_str() returns &my_empty_string ... After the fix my_empty_string: - remembers that it is null terminated and does not call realloc() from c_ptr_safe() - remembers that it is read-only and protects itself from realloc() by DBUG_ASSERT().
I'd rather fix Item_func_make_set instead. Now it seems to be the only Item that uses it (there's also SELECT ... OUTFILE, but that's different).
And Item_func_make_set has String tmp_str property, which is already used as an internal storage.
So, I'd remove my_empty_string usage from Item_func_make_set whatsoever. Simply with
- String *result=&my_empty_string; + String *result=&tmp_str; + tmp_str.length(0);
In fact, perhaps Item_func_make_set::tmp_str is not needed at all, and Item:str_value can be used instead:
- String *result=&my_empty_string; + String *result=make_empty_result();
These types of bug are annoying. We've fixed a lot of them. I hoped to extend the new code gradually for non-static strings as well: - to fix String::c_ptr*() not to use realloc if constructor knows that the value is null terminated. - protect use of c_ptr*() is unsafe contexts. and also put String::thread_specific into String::flags when merging to 10.0. But this all can be done in 10.0 only. No need to do it in 5.3. I will fix 5.3 simply to use make_empty_result().
Regards, Sergei
Hi, Alexander! On Jan 28, Alexander Barkov wrote:
These types of bug are annoying. We've fixed a lot of them. I hoped to extend the new code gradually for non-static strings as well:
- to fix String::c_ptr*() not to use realloc if constructor knows that the value is null terminated. - protect use of c_ptr*() is unsafe contexts.
and also put String::thread_specific into String::flags when merging to 10.0.
But this all can be done in 10.0 only. No need to do it in 5.3.
Agree!
I will fix 5.3 simply to use make_empty_result().
Thanks! Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik