maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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