From: Martin Hansson Date: August 11 2010 1:55pm Subject: Re: bzr commit into mysql-5.1-bugteam branch (Georgi.Kodinov:3479) Bug#55580 List-Archive: http://lists.mysql.com/commits/115497 Message-Id: <4C62ABE8.9080305@oracle.com> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="------------020403050208080201070309" --------------020403050208080201070309 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Joro, your patch is probably correct but I can't tell without a test case. Is there a reason for not having one? You should be able to at least simulate an error. Georgi Kodinov skrev 2010-08-10 11.24: > #At file:///home/kgeorge/mysql/work/B55580-5.1-bugteam/ based on revid:georgi.kodinov@stripped > > 3479 Georgi Kodinov 2010-08-10 > Bug #55580 : segfault in read_view_sees_trx_id > > The server was not checking for errors generated during > the execution of Item::val_xxx() methods when copying > data to the group, order, or distinct temp table's row. > Fixed by extending the copy_funcs() to return an error > code and by checking for that error code on the places > copy_funcs() is called. > > modified: > sql/item_sum.cc > sql/sql_select.cc > sql/sql_select.h > === modified file 'sql/item_sum.cc' > --- a/sql/item_sum.cc 2010-06-10 20:45:22 +0000 > +++ b/sql/item_sum.cc 2010-08-10 09:24:35 +0000 > @@ -2556,7 +2556,8 @@ bool Item_sum_count_distinct::add() > if (always_null) > return 0; > copy_fields(tmp_table_param); > - copy_funcs(tmp_table_param->items_to_copy); > + if (copy_funcs(tmp_table_param->items_to_copy, table->in_use)) > + return TRUE; > > for (Field **field=table->field ; *field ; field++) > if ((*field)->is_real_null(0)) > @@ -3128,7 +3129,8 @@ bool Item_func_group_concat::add() > if (always_null) > return 0; > copy_fields(tmp_table_param); > - copy_funcs(tmp_table_param->items_to_copy); > + if (copy_funcs(tmp_table_param->items_to_copy, table->in_use)) > + return TRUE; > > for (uint i= 0; i< arg_count_field; i++) > { > > === modified file 'sql/sql_select.cc' > --- a/sql/sql_select.cc 2010-07-19 18:34:28 +0000 > +++ b/sql/sql_select.cc 2010-08-10 09:24:35 +0000 > @@ -12485,7 +12485,9 @@ end_write(JOIN *join, JOIN_TAB *join_tab > if (!end_of_records) > { > copy_fields(&join->tmp_table_param); > - copy_funcs(join->tmp_table_param.items_to_copy); > + if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd)) > + DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */ > + > #ifdef TO_BE_DELETED > if (!table->uniques) // If not unique handling > { > @@ -12591,7 +12593,8 @@ end_update(JOIN *join, JOIN_TAB *join_ta > memcpy(table->record[0]+key_part->offset, group->buff, 1); > } > init_tmptable_sum_functions(join->sum_funcs); > - copy_funcs(join->tmp_table_param.items_to_copy); > + if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd)) > + DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */ > if ((error=table->file->ha_write_row(table->record[0]))) > { > if (create_myisam_from_heap(join->thd, table,&join->tmp_table_param, > @@ -12626,7 +12629,8 @@ end_unique_update(JOIN *join, JOIN_TAB * > > init_tmptable_sum_functions(join->sum_funcs); > copy_fields(&join->tmp_table_param); // Groups are copied twice. > - copy_funcs(join->tmp_table_param.items_to_copy); > + if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd)) > + DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */ > > if (!(error=table->file->ha_write_row(table->record[0]))) > join->send_records++; // New group > @@ -12713,7 +12717,8 @@ end_write_group(JOIN *join, JOIN_TAB *jo > if (idx< (int) join->send_group_parts) > { > copy_fields(&join->tmp_table_param); > - copy_funcs(join->tmp_table_param.items_to_copy); > + if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd)) > + DBUG_RETURN(NESTED_LOOP_ERROR); > if (init_sum_functions(join->sum_funcs, join->sum_funcs_end[idx+1])) > DBUG_RETURN(NESTED_LOOP_ERROR); > if (join->procedure) > @@ -15773,14 +15778,29 @@ update_sum_func(Item_sum **func_ptr) > return 0; > } > > -/** Copy result of functions to record in tmp_table. */ > +/** > + Copy result of functions to record in tmp_table. > > -void > -copy_funcs(Item **func_ptr) > + Uses the thread pointer to check for errors in > + some of the val_xxx() methods called by the > + save_in_result_field() function. > + TODO: make the Item::val_xxx() return error code > + > + @param func_ptr list of the function Items to copy to the tmp table > Should probably be called array, not list. > + @param thd pointer to the current thread for error checking > + @retval > + 0 if OK > + @retval > + 1 on error > The data type is bool, so the result should be true or false, not 1 or 0. > +*/ > + > +bool > +copy_funcs(Item **func_ptr, THD *thd) > You should be able to take a const pointer to thd. > { > Item *func; > for (; (func = *func_ptr) ; func_ptr++) > func->save_in_result_field(1); > + return (thd->is_error()); > } > Don't you think it's better to exit as soon as you see the error happen? > > > > === modified file 'sql/sql_select.h' > --- a/sql/sql_select.h 2010-02-26 13:16:46 +0000 > +++ b/sql/sql_select.h 2010-08-10 09:24:35 +0000 > @@ -601,7 +601,7 @@ bool setup_copy_fields(THD *thd, TMP_TAB > List &new_list1, List &new_list2, > uint elements, List &fields); > void copy_fields(TMP_TABLE_PARAM *param); > -void copy_funcs(Item **func_ptr); > +bool copy_funcs(Item **func_ptr, THD *thd); > bool create_myisam_from_heap(THD *thd, TABLE *table, TMP_TABLE_PARAM *param, > int error, bool ignore_last_dupp_error); > uint find_shortest_key(TABLE *table, const key_map *usable_keys); > > > > > Best Regards Martin Hansson --------------020403050208080201070309--