List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:November 26 2010 8:44am
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)
Bug#58455
View as plain text  
On 11/25/2010 10:35 AM, Guilhem Bichot wrote:
> Hello Mats,
>
> Mats Kindahl a écrit, Le 24.11.2010 15:20:
>> #At file:///home/bzr/mkindahl/testing-trunk-bugfixing/ based on
>> revid:sergey.glukhov@stripped
>>
>>  3388 Mats Kindahl    2010-11-24
>>       Bug #58455
>>       Starting mysqld with defaults file without
>>       extension cause segmentation fault
>>             Bug occurs because fn_expand calls fn_format
>>       with NULL as ext.
>
>> === added file 'mysql-test/t/mysqld--defaults-file.test'
>> --- a/mysql-test/t/mysqld--defaults-file.test    1970-01-01 00:00:00
>> +0000
>> +++ b/mysql-test/t/mysqld--defaults-file.test    2010-11-24 14:20:25
>> +0000
>> @@ -0,0 +1,31 @@
>> +#
>> +# mysqld --defaults-file
>> +#
>> +
>> +--source include/not_embedded.inc
>> +
>> +# All these tests refer to configuration files that do not exist
>> +
>> +--error 1
>> +--exec $MYSQLD --defaults-file=/path/with/no/extension
>> --print-defaults 2>&1
>
> I suggest writing in the test why MYSQLD is used instead of
> MYSQLD_BOOTSTRAP_CMD

OK.

>
>> === modified file 'mysys/mf_format.c'
>> --- a/mysys/mf_format.c    2009-04-19 01:21:33 +0000
>> +++ b/mysys/mf_format.c    2010-11-24 14:20:25 +0000
>> @@ -31,9 +31,12 @@ char * fn_format(char * to, const char *
>>    reg1 size_t length;
>>    size_t dev_length;
>>    DBUG_ENTER("fn_format");
>> +  DBUG_ASSERT(name != NULL);
>> +  DBUG_ASSERT(extension != NULL);
>>    DBUG_PRINT("enter",("name: %s  dir: %s  extension: %s  flag: %d",
>>                 name,dir,extension,flag));
>>  
>> +
>>    /* Copy and skip directory */
>>    name+=(length=dirname_part(dev, (startpos=(char *) name),
>> &dev_length));
>>    if (length == 0 || (flag & MY_REPLACE_DIR))
>
> Two suggestions while you're in this function:
> - add in the function comment of fn_expand(), that it allocates memory
> (which will thus have to be freed).
> - "DBUG_ASSERT(result_path!=NULL)" could be removed, as it's preceded by
> if (... (result_path=...)==NULL) DBUG_RETURN

OK.

>
> Also, but it's not something to tackle here, there is something weird
> in the function's logic (but I prefer that we don't touch it (who
> knows what it would break?)):
>
>   if (!(flag & MY_APPEND_EXT) &&
>       (pos= (char*) strchr(name,FN_EXTCHAR)) != NullS)
>   {
>     if ((flag & MY_REPLACE_EXT) == 0)           /* If we should keep
> old ext */
>     {
>       length=strlength(name);                   /* Use old extension */
>       ext = "";
>     }
>     else
>     {
>       length= (size_t) (pos-(char*) name);      /* Change extension */
>       ext= extension;
>     }
>   }
>   else
>   {
>     length=strlength(name);                     /* No ext, use the now
> one */
>     ext=extension;
>   }
>
> If 'flags' does not contain MY_APPEND_EXT and 'name' does not contain
> an extension, we will come to the last "else" branch: it will add
> 'extension' to the file's name. Even though we didn't pass MY_APPEND_EXT!
> Fortunately, as you now pass "", we will add "", which is harmless.

I started looking at the logic, but soon decided against it for fear of
turning insane. :)

Yeah, I agree that there potentially are some problems in the function
but, as you say, we should not change it now since there is a risk of
breaking something.

> Ok to push.
>

OK. I'll add the suggestions you had above and also use the suggestion
from Björn to avoid changing mysql-test-run.pl.

/Matz

Thread
bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388) Bug#58455Mats Kindahl24 Nov
Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Magnus BlĂ„udd24 Nov
  • Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Mats Kindahl24 Nov
Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Guilhem Bichot25 Nov
  • Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Guilhem Bichot25 Nov
  • Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Mats Kindahl26 Nov
  • Re: bzr commit into mysql-trunk-bugfixing branch (mats.kindahl:3388)Bug#58455Mats Kindahl26 Nov
Re: bzr commit into mysql-trunk-bugfixing branch(mats.kindahl:3388) Bug#58455Bjorn Munch25 Nov