From 1d48aec228e6a753f4a802771d4ebd411d4c19dc Mon Sep 17 00:00:00 2001 From: unknown Date: Sun, 10 Oct 2004 02:39:22 +0400 Subject: [PATCH] A fix and test case for Bug#5987 "subselect in bool function crashes server (prepared statements)": the bug was that all boolean items always recovered its original arguments at statement cleanup stage. This collided with Item_subselect::select_transformer, which tries to permanently change the item tree to use a transformed subselect instead of original one. So we had this call sequence for prepare: mysql_stmt_prepare -> JOIN::prepare -> Item_subselect::fix_fields -> the item tree gets transformed -> Item_bool_rowready_func2::cleanup, item tree is recovered to original state, while it shouldn't have been; mysql_stmt_execute -> attempts to execute a broken tree -> crash. Now instead of bluntly recovering all arguments of bool functions in Item_bool_rowready_func2::cleanup, we recover only those which were changed, and do it in one place. There still would exist a possibility for a collision with subselect tranformation, if permanent and temporary changes were performed at the same stage. But fortunately subselect transformation is always done first, so it doesn't conflict with the optimization done by propogate_cond_constants. Now we have: mysql_stmt_prepare -> JOIN::prepare -> subselect transformation permanently changes the tree -> cleanup doesn't recover anything, because nothing was registered for recovery. mysql_stmt_execute -> JOIN::prepare (the tree is already transformed, so it doesn't change), JOIN::optimize -> propogate_cond_constants -> temporary changes the item tree with constants -> JOIN::execute -> cleanup -> the changes done by propogate_cond_constants are recovered, as they were registered for recovery. mysql-test/r/ps.result: Bug#5987: test results fixed. mysql-test/t/ps.test: A test for bug#5987 "subselect in bool function crashes server (prepared statements)" sql/item.cc: resolve_const_item is now responsible to register all changes of the item tree for recovery sql/item.h: resolve_const_item signagture changed sql/item_cmpfunc.h: Arguments of boolean functions are now recovered using the centralized registry of THD. sql/sql_class.cc: It's crucial to add new items to the beginning of the recovery list, so that the recovery is performed in LIFO mode: otherwise if we change one node of a tree twice, it will be recovered to some intermediate state. sql/sql_select.cc: change_cond_ref_to_const and propogate_cond_constants are now responsible to register all changes of the item tree for recovery. The recovery is done using the centralized THD registry of changed tree items. --- mysql-test/r/ps.result | 10 +++++++ mysql-test/t/ps.test | 14 +++++++++ sql/item.cc | 32 +++++++++++++------- sql/item.h | 2 +- sql/item_cmpfunc.h | 12 +------- sql/sql_class.cc | 2 +- sql/sql_select.cc | 66 ++++++++++++++++++++++-------------------- 7 files changed, 82 insertions(+), 56 deletions(-) diff --git a/mysql-test/r/ps.result b/mysql-test/r/ps.result index 8e63e0cdf00..c10cb7bb25a 100644 --- a/mysql-test/r/ps.result +++ b/mysql-test/r/ps.result @@ -308,3 +308,13 @@ execute stmt using @a, @a; a drop table t1; deallocate prepare stmt; +create table t1 (a int); +prepare stmt from "select * from t1 where 1 > (1 in (SELECT * FROM t1))"; +execute stmt; +a +execute stmt; +a +execute stmt; +a +drop table t1; +deallocate prepare stmt; diff --git a/mysql-test/t/ps.test b/mysql-test/t/ps.test index c961251255d..42f6d4d0f64 100644 --- a/mysql-test/t/ps.test +++ b/mysql-test/t/ps.test @@ -329,3 +329,17 @@ execute stmt using @a, @a; drop table t1; deallocate prepare stmt; +# +# Bug #5987 subselect in bool function crashes server (prepared statements): +# don't overwrite transformed subselects with old arguments of a bool +# function. +# +create table t1 (a int); +prepare stmt from "select * from t1 where 1 > (1 in (SELECT * FROM t1))"; +execute stmt; +execute stmt; +execute stmt; +drop table t1; +deallocate prepare stmt; + + diff --git a/sql/item.cc b/sql/item.cc index 564bc72927c..3165f48aa8f 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -2209,10 +2209,12 @@ Item_result item_cmp_type(Item_result a,Item_result b) } -Item *resolve_const_item(Item *item,Item *comp_item) +void resolve_const_item(THD *thd, Item **ref, Item *comp_item) { + Item *item= *ref; + Item *new_item; if (item->basic_const_item()) - return item; // Can't be better + return; // Can't be better Item_result res_type=item_cmp_type(comp_item->result_type(), item->result_type()); char *name=item->name; // Alloced by sql_alloc @@ -2223,26 +2225,34 @@ Item *resolve_const_item(Item *item,Item *comp_item) String tmp(buff,sizeof(buff),&my_charset_bin),*result; result=item->val_str(&tmp); if (item->null_value) - return new Item_null(name); - uint length=result->length(); - char *tmp_str=sql_strmake(result->ptr(),length); - return new Item_string(name,tmp_str,length,result->charset()); + new_item= new Item_null(name); + else + { + uint length= result->length(); + char *tmp_str= sql_strmake(result->ptr(), length); + new_item= new Item_string(name, tmp_str, length, result->charset()); + } } - if (res_type == INT_RESULT) + else if (res_type == INT_RESULT) { longlong result=item->val_int(); uint length=item->max_length; bool null_value=item->null_value; - return (null_value ? (Item*) new Item_null(name) : - (Item*) new Item_int(name,result,length)); + new_item= (null_value ? (Item*) new Item_null(name) : + (Item*) new Item_int(name, result, length)); } else { // It must REAL_RESULT double result=item->val(); uint length=item->max_length,decimals=item->decimals; bool null_value=item->null_value; - return (null_value ? (Item*) new Item_null(name) : - (Item*) new Item_real(name,result,decimals,length)); + new_item= (null_value ? (Item*) new Item_null(name) : (Item*) + new Item_real(name, result, decimals, length)); + } + if (new_item) + { + thd->register_item_tree_change(ref, item, &thd->mem_root); + *ref= new_item; } } diff --git a/sql/item.h b/sql/item.h index 589edb88565..e1dd7301217 100644 --- a/sql/item.h +++ b/sql/item.h @@ -1223,5 +1223,5 @@ public: extern Item_buff *new_Item_buff(Item *item); extern Item_result item_cmp_type(Item_result a,Item_result b); -extern Item *resolve_const_item(Item *item,Item *cmp_item); +extern void resolve_const_item(THD *thd, Item **ref, Item *cmp_item); extern bool field_is_equal_to_item(Field *field,Item *item); diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h index 3e98684b6ca..dcc32441e88 100644 --- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -210,21 +210,11 @@ public: class Item_bool_rowready_func2 :public Item_bool_func2 { - Item *orig_a, *orig_b; /* propagate_const can change parameters */ public: - Item_bool_rowready_func2(Item *a,Item *b) :Item_bool_func2(a,b), - orig_a(a), orig_b(b) + Item_bool_rowready_func2(Item *a, Item *b) :Item_bool_func2(a, b) { allowed_arg_cols= a->cols(); } - void cleanup() - { - DBUG_ENTER("Item_bool_rowready_func2::cleanup"); - Item_bool_func2::cleanup(); - tmp_arg[0]= orig_a; - tmp_arg[1]= orig_b; - DBUG_VOID_RETURN; - } Item *neg_transformer(THD *thd); virtual Item *negated_item(); }; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 5273d379841..4ce655c78f8 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -730,7 +730,7 @@ void THD::nocheck_register_item_tree_change(Item **place, Item *old_value, change= new (change_mem) Item_change_record; change->place= place; change->old_value= old_value; - change_list.push_back(change); + change_list.append(change); } diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 0834f0659da..f2d814ed057 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -4171,8 +4171,9 @@ template class List_iterator; */ static void -change_cond_ref_to_const(I_List *save_list,Item *and_father, - Item *cond, Item *field, Item *value) +change_cond_ref_to_const(THD *thd, I_List *save_list, + Item *and_father, Item *cond, + Item *field, Item *value) { if (cond->type() == Item::COND_ITEM) { @@ -4181,7 +4182,7 @@ change_cond_ref_to_const(I_List *save_list,Item *and_father, List_iterator li(*((Item_cond*) cond)->argument_list()); Item *item; while ((item=li++)) - change_cond_ref_to_const(save_list,and_level ? cond : item, item, + change_cond_ref_to_const(thd, save_list,and_level ? cond : item, item, field, value); return; } @@ -4189,8 +4190,9 @@ change_cond_ref_to_const(I_List *save_list,Item *and_father, return; // Not a boolean function Item_bool_func2 *func= (Item_bool_func2*) cond; - Item *left_item= func->arguments()[0]; - Item *right_item= func->arguments()[1]; + Item **args= func->arguments(); + Item *left_item= args[0]; + Item *right_item= args[1]; Item_func::Functype functype= func->functype(); if (right_item->eq(field,0) && left_item != value && @@ -4201,7 +4203,8 @@ change_cond_ref_to_const(I_List *save_list,Item *and_father, Item *tmp=value->new_item(); if (tmp) { - func->arguments()[1] = tmp; + thd->register_item_tree_change(args + 1, args[1], &thd->mem_root); + args[1]= tmp; func->update_used_tables(); if ((functype == Item_func::EQ_FUNC || functype == Item_func::EQUAL_FUNC) && and_father != cond && !left_item->const_item()) @@ -4222,13 +4225,15 @@ change_cond_ref_to_const(I_List *save_list,Item *and_father, Item *tmp=value->new_item(); if (tmp) { - func->arguments()[0] = value = tmp; + thd->register_item_tree_change(args, args[0], &thd->mem_root); + args[0]= value= tmp; func->update_used_tables(); if ((functype == Item_func::EQ_FUNC || functype == Item_func::EQUAL_FUNC) && and_father != cond && !right_item->const_item()) { - func->arguments()[0] = func->arguments()[1]; // For easy check - func->arguments()[1] = value; + args[0]= args[1]; // For easy check + thd->register_item_tree_change(args + 1, args[1], &thd->mem_root); + args[1]= value; cond->marker=1; COND_CMP *tmp2; if ((tmp2=new COND_CMP(and_father,func))) @@ -4274,8 +4279,8 @@ static Item *remove_additional_cond(Item* conds) } static void -propagate_cond_constants(I_List *save_list,COND *and_father, - COND *cond) +propagate_cond_constants(THD *thd, I_List *save_list, + COND *and_father, COND *cond) { if (cond->type() == Item::COND_ITEM) { @@ -4286,18 +4291,19 @@ propagate_cond_constants(I_List *save_list,COND *and_father, I_List save; while ((item=li++)) { - propagate_cond_constants(&save,and_level ? cond : item, item); + propagate_cond_constants(thd, &save,and_level ? cond : item, item); } if (and_level) { // Handle other found items I_List_iterator cond_itr(save); COND_CMP *cond_cmp; while ((cond_cmp=cond_itr++)) - if (!cond_cmp->cmp_func->arguments()[0]->const_item()) - change_cond_ref_to_const(&save,cond_cmp->and_level, - cond_cmp->and_level, - cond_cmp->cmp_func->arguments()[0], - cond_cmp->cmp_func->arguments()[1]); + { + Item **args= cond_cmp->cmp_func->arguments(); + if (!args[0]->const_item()) + change_cond_ref_to_const(thd, &save,cond_cmp->and_level, + cond_cmp->and_level, args[0], args[1]); + } } } else if (and_father != cond && !cond->marker) // In a AND group @@ -4307,29 +4313,25 @@ propagate_cond_constants(I_List *save_list,COND *and_father, ((Item_func*) cond)->functype() == Item_func::EQUAL_FUNC)) { Item_func_eq *func=(Item_func_eq*) cond; - bool left_const= func->arguments()[0]->const_item(); - bool right_const=func->arguments()[1]->const_item(); + Item **args= func->arguments(); + bool left_const= args[0]->const_item(); + bool right_const= args[1]->const_item(); if (!(left_const && right_const) && - (func->arguments()[0]->result_type() == - (func->arguments()[1]->result_type()))) + args[0]->result_type() == args[1]->result_type()) { if (right_const) { - func->arguments()[1]=resolve_const_item(func->arguments()[1], - func->arguments()[0]); + resolve_const_item(thd, &args[1], args[0]); func->update_used_tables(); - change_cond_ref_to_const(save_list,and_father,and_father, - func->arguments()[0], - func->arguments()[1]); + change_cond_ref_to_const(thd, save_list, and_father, and_father, + args[0], args[1]); } else if (left_const) { - func->arguments()[0]=resolve_const_item(func->arguments()[0], - func->arguments()[1]); + resolve_const_item(thd, &args[0], args[1]); func->update_used_tables(); - change_cond_ref_to_const(save_list,and_father,and_father, - func->arguments()[1], - func->arguments()[0]); + change_cond_ref_to_const(thd, save_list, and_father, and_father, + args[1], args[0]); } } } @@ -4346,7 +4348,7 @@ optimize_cond(THD *thd, COND *conds, Item::cond_result *cond_value) { DBUG_EXECUTE("where", print_where(conds, "original");); /* change field = field to field = const for each found field = const */ - propagate_cond_constants((I_List *) 0, conds, conds); + propagate_cond_constants(thd, (I_List *) 0, conds, conds); /* Remove all instances of item == item Remove all and-levels where CONST item != CONST item