List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:December 14 2010 11:27am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3512)
Bug#58207
View as plain text  
Hi,
your fix is correct, I would only like to 'future proof' it by using the 
test

orig_type != Item::DEFAULT_VALUE_ITEM

instead.

Sergey Glukhov skrev 2010-12-13 15.04:
> Hi,
>
> On 12/10/2010 10:40 PM, Martin Hansson wrote:
>> Sergey Glukhov skrev 2010-12-10 12.27:
>>> Hi,
>>>
>>> On 12/10/2010 01:24 PM, Martin Hansson wrote:
>>>> Sergey Glukhov skrev 2010-12-09 10.25:
>>>>> Hi Martin,
>>>>>
>>>>> On 12/06/2010 01:56 PM, Martin Hansson wrote:
>>>>>> #At file:///data0/martin/bzrroot/bug58207/5.1bt-minimal_fix/ 
>>>>>> based on 
>>>>>> revid:mats.kindahl@stripped
>>>>>>
>>>>>>   3512 Martin Hansson    2010-12-06
>>>>>>        Bug#58207: invalid memory reads when using default column
> 
>>>>>> value and
>>>>>>        tmptable needed
>>>>>>
>>>>>>        The function DEFAULT() works by modifying the the data 
>>>>>> buffer pointers (often
>>>>>>        referred to as 'record' or 'table record') of its 
>>>>>> argument. This modification
>>>>>>        is done during name resolution (fix_fields().) Here, the 
>>>>>> pointers are set to
>>>>>>        point into a buffer of default values by means of 
>>>>>> offsetting the pointer by
>>>>>>        the distance between the buffers' start addresses. 
>>>>>> Unfortunately, the same
>>>>>>        modification is done when creating a temporary table 
>>>>>> during optimization. At
>>>>>>        this point the Item may or may not be name-resolved. If it
> 
>>>>>> is, the bespoke
>>>>>>        pointer offsetting method is fragile enough to point into
> 
>>>>>> an undefined memory
>>>>>>        area, giving rise to everything from valgrind warnings to
> 
>>>>>> segmentation faults.
>>>>>>
>>>>>>        Fixed by checking if an Item is name-resolved before 
>>>>>> modifying the pointers.
>>>>>>
>>>>>>      modified:
>>>>>>        mysql-test/r/subselect4.result
>>>>>>        mysql-test/t/subselect4.test
>>>>>>        sql/sql_select.cc
>>>>> ...
>>>>>> === modified file 'mysql-test/t/subselect4.test'
>>>>>> --- a/mysql-test/t/subselect4.test    2010-09-07 09:21:09 +0000
>>>>>> +++ b/mysql-test/t/subselect4.test    2010-12-06 10:56:35 +0000
>>>>>> @@ -136,6 +136,23 @@ SELECT * FROM t1 WHERE NULL NOT IN ( SEL
>>>>>>
>>>>>>   DROP TABLE t1, t2, t3, t4, t5;
>>>>>>
>>>>>> +--echo #
>>>>>> +--echo # Bug#58207: invalid memory reads when using default 
>>>>>> column value and
>>>>>> +--echo # tmptable needed
>>>>>> +--echo #
>>>>>> +CREATE TABLE t1( a CHAR(1)   DEFAULT 'a' );
>>>>>> +CREATE TABLE t2( a CHAR(245) DEFAULT 'a' );
>>>>>> +
>>>>>> +INSERT INTO t1 VALUES ('b'), ('c');
>>>>>> +INSERT INTO t2 VALUES ('b'), ('c');
>>>>>> +
>>>>>> +--echo # Caused crash in valgrind builds of revno:3517 (version
> 
>>>>>> 5.1.54)
>>>>>> +SELECT * FROM (SELECT DEFAULT(a) FROM t1) t11;
>>>>> ^^^
>>>>> This test does not cause crash and
>>>>> there is no valgrind warnings in last 5.1-bugteam tree,
>>>>> compile-pentium-valgrind-max build, Please remove it.
>>>> I don't feel comfortable removing this. When I tested it in the 
>>>> revision and build mentioned in the comment, I observed a crash. 
>>>> What fixed the crash is not identified, so it is my job to make 
>>>> sure the problem does not resurface.
>>>>>> +--echo # Should not cause valgrind error
>>>>> Test case below causes crash
>>>>> (last 5.1-bugteam,compile-pentium-valgrind-max build),
>>>>> please fix the comment in 'echo'.
>>>>> Imho comment above is unnecessary.
>>>> I changed the comment. It is necessary in my opinion. It is rather 
>>>> unusual with a crash resulting from a valgrind build. Especially 
>>>> when the bug title says otherwise.
>>> Don't see any changes in your last commit :)
>>> On some trees I got a crash, on some the test works fine,
>>> on some trees as you asserts we have valgrind warning,
>>> I did not manage to get just valgrind warnings but
>>> I admit that it might happen).
>>> So your comment is a bit misleading imho.
>> Ok, removing it. No documentation is better than wrong documentation. 
>> :-)
>>>>>
>>>>>> +SELECT * FROM (SELECT DEFAULT(a) AS b FROM t2 GROUP BY a) t21;
>>>>>> +
>>>>>> +DROP TABLE t1, t2;
>>>>>>
>>>>>>   --echo #
>>>>>>   --echo # End of 5.1 tests.
>>>>>>
>>>>>> === modified file 'sql/sql_select.cc'
>>>>>> --- a/sql/sql_select.cc    2010-11-26 12:51:48 +0000
>>>>>> +++ b/sql/sql_select.cc    2010-12-06 10:56:35 +0000
>>>>>> @@ -10207,6 +10207,15 @@ create_tmp_table(THD
> *thd,TMP_TABLE_PARA
>>>>>>                            item->marker == 4,
> force_copy_fields,
>>>>>>                            param->convert_blob_length);
>>>>>>
>>>>>> +      /*
>>>>>> +        Fields that are used as arguments to the DEFAULT() 
>>>>>> function have their
>>>>>> +        data pointers set to the default value during name 
>>>>>> resulotion. If by
>>>>>> +        any chance they have been resolved at this stage (e.g. 
>>>>>> if they appear
>>>>>> +        in a subquery) we should not update these pointers.
>>>>>> +      */
>>>>>> +      if (item->type() == Item::DEFAULT_VALUE_ITEM&& 
> item->fixed)
>>>>>> +        default_field[fieldnr]= NULL;
>>>>>> +
>>>>>>         if (!new_field)
>>>>>>         {
>>>>>>       if (thd->is_fatal_error)
>>>>> Better place to fix the problem is create_tmp_field() function,
>>>>> see suggested fix: ...
>>>> Why is it better?
>>> According to the comment in create_tmp_table()
>>>
>>> "default_field[i] is set only in the cases  when 'field' can
>>>  inherit the default value that is defined for the field referred
>>>  by the Item_field object from which 'field' has been created."
>> This is not the documentation for create_tmp_field, we cannot trust 
>> it to know how this function should behave.
>> And this comment speaks on a higher abstraction level.
> disagree...
That was a very short answer. ;-) What do you disagree to exactly? Is it 
1) That this is not the function comment to create_tmp_field? 2) That we 
cannot trust it? or 3) That it speaks on a higher abstraction level? Or 
do you disagree to a 'bitmap' of those 3 statements? ;-)
>
>> Before I am convinced I want to see a test of your fix with SELECT 
>> DEFAULT(field) and a temporary table where 'field' is not name 
>> resolved (fixed == TRUE) when create_tmp_table() is called. You can 
>> probably accomplish it with ORDER/GROUP BY.
> I an not a bugfixer for this bug, I am just reviewer.
> I suggested a simpler solution and you have some doubts
> about this. So from my point of view you should provide a
> test case which shows incorrectness of my suggestion. :)
Fair enough. But you just gave me a patch that was completely different 
than mine without any comments. I realize now that name resolution is 
not the issue; expressions - "Item's" are always name resolved when we 
create a temporary table. You obviously knew this, so you could have 
just told me when you read the sentence "At this point the Item may or 
may not be name-resolved" that I was completely lost. :-)
>
>>> With your fix you do improper initialization of default_filed in
>>> create_tmp_field for DEFAULT_VALUE_ITEM and then clear it in 
>>> create_tmp_table.
>>> It's redundant. You could just skip initialization of default_filed in
>>> create_tmp_field.
>> Either way is fine with me. But let's be sure first.
>>>
>>>> Are you aware that create_tmp_field is called twice, but only one 
>>>> of the calls is a problem?
>>> Are you sure that create_tmp_field is called twice for 
>>> DEFAULT_VALUE_ITEM?
>> Sorry, I was being unclear. I meant that this function is called from 
>> a lot of places.
> I found only 3 places, 1 in sql_insert.cc + 2 in sql_select.cc. Is it 
> a lot?
Sorry, my mistake. there's also a member of Item_sum with the same name 
that I must have mixed up.
>
>> The function comment says
>>
>>   @param default_field    If field has a default value field, store 
>> it here
>>
>> I am worried about changing the semantics of the function. This is a 
>> public function, declared in mysql_priv.h. You want to change 
>> behavior to 'If field has a default value field, store it here 
>> *UNLESS* it is used along with DEFAULT'. 
> We don't need to change semantic of this function, the change I suggested
> does not break Item_field behavior. My change is about Item_default_value
> object. 
Yes, I know that now...
> I hope you don't confuse Item_field which is inherited from the
> Field object which could have DEFAULT value and
> Item_default_value which returns the default value for a table column.
I wouldn't say 'inherit'. An Item_field comes from the parser and name 
resolution will associate it with a physical Field. So the Item_field 
actually comes first. :-)

I am not confusing those. I was concerned with *how* Item_default_value 
returns the default value. It does this by moving the field pointer, 
which is kind of dangerous.
> We don't have any problems with Item_field objects, only with 
> Item_default_value.
Yes, that's why I'm changing your test condition. There are no problems 
with other Item_field's, i.e. Item_insert_value and Item_trigger_field.
>> How would you deal with 'SELECT DEFAULT(field), field'? Should you 
>> update the entry for 'field' or not?
>> (I guess the answer is yes.)
> and probably we do it with my suggestion, don't we?
My question was wrong put. The two references to 'field' actually create 
separate instances of the Field object, and hence separate positions in 
the default_values array so it's a non-issue.


Thank you for the cooperation!

Best Regards

Martin

Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3512) Bug#58207Martin Hansson6 Dec
Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3512)Bug#58207Martin Hansson14 Dec