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