BUG#20769: Dangling pointer in ctype_recoding test case.

In some functions dealing with strings and character sets, the wrong
pointers were saved for restoration in THD::rollback_item_tree_changes().
This could potentially cause random corruption or crashes.

Fixed by passing the original Item ** locations, not local stack copies.

Also remove unnecessary use of default arguments.

sql/item.cc

@@ -1315,35 +1315,37 @@ void my_coll_agg_error(DTCollation &c1, DTCollation &c2, DTCollation &c3,
 
 
 static
-void my_coll_agg_error(Item** args, uint count, const char *fname)
+void my_coll_agg_error(Item** args, uint count, const char *fname,
+                       int item_sep)
 {
   if (count == 2)
-    my_coll_agg_error(args[0]->collation, args[1]->collation, fname);
+    my_coll_agg_error(args[0]->collation, args[item_sep]->collation, fname);
   else if (count == 3)
-    my_coll_agg_error(args[0]->collation, args[1]->collation,
-                      args[2]->collation, fname);
+    my_coll_agg_error(args[0]->collation, args[item_sep]->collation,
+                      args[2*item_sep]->collation, fname);
   else
     my_error(ER_CANT_AGGREGATE_NCOLLATIONS,MYF(0),fname);
 }
 
 
 bool agg_item_collations(DTCollation &c, const char *fname,
-                         Item **av, uint count, uint flags)
+                         Item **av, uint count, uint flags, int item_sep)
 {
   uint i;
+  Item **arg;
   c.set(av[0]->collation);
-  for (i= 1; i < count; i++)
+  for (i= 1, arg= &av[item_sep]; i < count; i++, arg++)
   {
-    if (c.aggregate(av[i]->collation, flags))
+    if (c.aggregate((*arg)->collation, flags))
     {
-      my_coll_agg_error(av, count, fname);
+      my_coll_agg_error(av, count, fname, item_sep);
       return TRUE;
     }
   }
   if ((flags & MY_COLL_DISALLOW_NONE) &&
       c.derivation == DERIVATION_NONE)
   {
-    my_coll_agg_error(av, count, fname);
+    my_coll_agg_error(av, count, fname, item_sep);
     return TRUE;
   }
   return FALSE;
@@ -1354,7 +1356,7 @@ bool agg_item_collations_for_comparison(DTCollation &c, const char *fname,
                                         Item **av, uint count, uint flags)
 {
   return (agg_item_collations(c, fname, av, count,
-                              flags | MY_COLL_DISALLOW_NONE));
+                              flags | MY_COLL_DISALLOW_NONE, 1));
 }
 
 
@@ -1377,13 +1379,22 @@ bool agg_item_collations_for_comparison(DTCollation &c, const char *fname,
   For functions with more than two arguments:
 
     collect(A,B,C) ::= collect(collect(A,B),C)
+
+  Since this function calls THD::change_item_tree() on the passed Item **
+  pointers, it is necessary to pass the original Item **'s, not copies.
+  Otherwise their values will not be properly restored (see BUG#20769).
+  If the items are not consecutive (eg. args[2] and args[5]), use the
+  item_sep argument, ie.
+
+    agg_item_charsets(coll, fname, &args[2], 2, flags, 3)
+
 */
 
 bool agg_item_charsets(DTCollation &coll, const char *fname,
-                       Item **args, uint nargs, uint flags)
+                       Item **args, uint nargs, uint flags, int item_sep)
 {
-  Item **arg, **last, *safe_args[2];
-  if (agg_item_collations(coll, fname, args, nargs, flags))
+  Item **arg, *safe_args[2];
+  if (agg_item_collations(coll, fname, args, nargs, flags, item_sep))
     return TRUE;
 
   /*
@@ -1396,19 +1407,20 @@ bool agg_item_charsets(DTCollation &coll, const char *fname,
   if (nargs >=2 && nargs <= 3)
   {
     safe_args[0]= args[0];
-    safe_args[1]= args[1];
+    safe_args[1]= args[item_sep];
   }
 
   THD *thd= current_thd;
   Query_arena *arena, backup;
   bool res= FALSE;
+  uint i;
   /*
     In case we're in statement prepare, create conversion item
     in its memory: it will be reused on each execute.
   */
   arena= thd->activate_stmt_arena_if_needed(&backup);
 
-  for (arg= args, last= args + nargs; arg < last; arg++)
+  for (i= 0, arg= args; i < nargs; i++, arg+= item_sep)
   {
     Item* conv;
     uint32 dummy_offset;
@@ -1423,9 +1435,9 @@ bool agg_item_charsets(DTCollation &coll, const char *fname,
       {
         /* restore the original arguments for better error message */
         args[0]= safe_args[0];
-        args[1]= safe_args[1];
+        args[item_sep]= safe_args[1];
       }
-      my_coll_agg_error(args, nargs, fname);
+      my_coll_agg_error(args, nargs, fname, item_sep);
       res= TRUE;
       break; // we cannot return here, we need to restore "arena".
     }

sql/item.h

@@ -1075,12 +1075,11 @@ public:
 };
 
 bool agg_item_collations(DTCollation &c, const char *name,
-                         Item **items, uint nitems, uint flags= 0);
+                         Item **items, uint nitems, uint flags, int item_sep);
 bool agg_item_collations_for_comparison(DTCollation &c, const char *name,
-                                        Item **items, uint nitems,
-                                        uint flags= 0);
+                                        Item **items, uint nitems, uint flags);
 bool agg_item_charsets(DTCollation &c, const char *name,
-                       Item **items, uint nitems, uint flags= 0);
+                       Item **items, uint nitems, uint flags, int item_sep);
 
 
 class Item_num: public Item

sql/item_cmpfunc.cc

@@ -394,7 +394,7 @@ void Item_bool_func2::fix_length_and_dec()
   DTCollation coll;
   if (args[0]->result_type() == STRING_RESULT &&
       args[1]->result_type() == STRING_RESULT &&
-      agg_arg_charsets(coll, args, 2, MY_COLL_CMP_CONV))
+      agg_arg_charsets(coll, args, 2, MY_COLL_CMP_CONV, 1))
     return;
     
  
@@ -1211,7 +1211,7 @@ void Item_func_between::fix_length_and_dec()
   agg_cmp_type(thd, &cmp_type, args, 3);
 
   if (cmp_type == STRING_RESULT)
-      agg_arg_charsets(cmp_collation, args, 3, MY_COLL_CMP_CONV);
+      agg_arg_charsets(cmp_collation, args, 3, MY_COLL_CMP_CONV, 1);
 }
 
 
@@ -1331,7 +1331,7 @@ Item_func_ifnull::fix_length_and_dec()
 
   switch (hybrid_type) {
   case STRING_RESULT:
-    agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV);
+    agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV, 1);
     break;
   case DECIMAL_RESULT:
   case REAL_RESULT:
@@ -1503,7 +1503,7 @@ Item_func_if::fix_length_and_dec()
     agg_result_type(&cached_result_type, args+1, 2);
     if (cached_result_type == STRING_RESULT)
     {
-      if (agg_arg_charsets(collation, args+1, 2, MY_COLL_ALLOW_CONV))
+      if (agg_arg_charsets(collation, args+1, 2, MY_COLL_ALLOW_CONV, 1))
         return;
     }
     else
@@ -1584,7 +1584,7 @@ Item_func_nullif::fix_length_and_dec()
     unsigned_flag= args[0]->unsigned_flag;
     cached_result_type= args[0]->result_type();
     if (cached_result_type == STRING_RESULT &&
-        agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV))
+        agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV, 1))
       return;
   }
 }
@@ -1876,7 +1876,7 @@ void Item_func_case::fix_length_and_dec()
   
   agg_result_type(&cached_result_type, agg, nagg);
   if ((cached_result_type == STRING_RESULT) &&
-      agg_arg_charsets(collation, agg, nagg, MY_COLL_ALLOW_CONV))
+      agg_arg_charsets(collation, agg, nagg, MY_COLL_ALLOW_CONV, 1))
     return;
   
   
@@ -1892,7 +1892,7 @@ void Item_func_case::fix_length_and_dec()
     nagg++;
     agg_cmp_type(thd, &cmp_type, agg, nagg);
     if ((cmp_type == STRING_RESULT) &&
-        agg_arg_charsets(cmp_collation, agg, nagg, MY_COLL_CMP_CONV))
+        agg_arg_charsets(cmp_collation, agg, nagg, MY_COLL_CMP_CONV, 1))
       return;
   }
   
@@ -2022,7 +2022,7 @@ void Item_func_coalesce::fix_length_and_dec()
   case STRING_RESULT:
     count_only_length();
     decimals= NOT_FIXED_DEC;
-    agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV);
+    agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV, 1);
     break;
   case DECIMAL_RESULT:
     count_decimal_length();
@@ -2486,7 +2486,7 @@ void Item_func_in::fix_length_and_dec()
   agg_cmp_type(thd, &cmp_type, args, arg_count);
 
   if (cmp_type == STRING_RESULT &&
-      agg_arg_charsets(cmp_collation, args, arg_count, MY_COLL_CMP_CONV))
+      agg_arg_charsets(cmp_collation, args, arg_count, MY_COLL_CMP_CONV, 1))
     return;
 
   for (arg=args+1, arg_end=args+arg_count; arg != arg_end ; arg++)
@@ -3219,7 +3219,7 @@ Item_func_regex::fix_fields(THD *thd, Item **ref)
   max_length= 1;
   decimals= 0;
 
-  if (agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV))
+  if (agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV, 1))
     return TRUE;
 
   used_tables_cache=args[0]->used_tables() | args[1]->used_tables();

sql/item_func.cc

@@ -2038,7 +2038,7 @@ void Item_func_min_max::fix_length_and_dec()
     cmp_type=item_cmp_type(cmp_type,args[i]->result_type());
   }
   if (cmp_type == STRING_RESULT)
-    agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV);
+    agg_arg_charsets(collation, args, arg_count, MY_COLL_CMP_CONV, 1);
   else if ((cmp_type == DECIMAL_RESULT) || (cmp_type == INT_RESULT))
     max_length= my_decimal_precision_to_length(max_int_part+decimals, decimals,
                                             unsigned_flag);
@@ -2227,7 +2227,7 @@ longlong Item_func_coercibility::val_int()
 void Item_func_locate::fix_length_and_dec()
 {
   maybe_null=0; max_length=11;
-  agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV);
+  agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV, 1);
 }
 
 
@@ -2344,7 +2344,7 @@ void Item_func_field::fix_length_and_dec()
   for (uint i=1; i < arg_count ; i++)
     cmp_type= item_cmp_type(cmp_type, args[i]->result_type());
   if (cmp_type == STRING_RESULT)
-    agg_arg_charsets(cmp_collation, args, arg_count, MY_COLL_CMP_CONV);
+    agg_arg_charsets(cmp_collation, args, arg_count, MY_COLL_CMP_CONV, 1);
 }
 
 
@@ -2411,7 +2411,7 @@ void Item_func_find_in_set::fix_length_and_dec()
       }
     }
   }
-  agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV);
+  agg_arg_charsets(cmp_collation, args, 2, MY_COLL_CMP_CONV, 1);
 }
 
 static const char separator=',';
@@ -4401,7 +4401,8 @@ bool Item_func_match::fix_fields(THD *thd, Item **ref)
     return 1;
   }
   table->fulltext_searched=1;
-  return agg_arg_collations_for_comparison(cmp_collation, args+1, arg_count-1);
+  return agg_arg_collations_for_comparison(cmp_collation,
+                                           args+1, arg_count-1, 0);
 }
 
 bool Item_func_match::fix_index()

sql/item_func.h

@@ -166,21 +166,21 @@ public:
   my_decimal *val_decimal(my_decimal *);
 
   bool agg_arg_collations(DTCollation &c, Item **items, uint nitems,
-                          uint flags= 0)
+                          uint flags)
   {
-    return agg_item_collations(c, func_name(), items, nitems, flags);
+    return agg_item_collations(c, func_name(), items, nitems, flags, 1);
   }
   bool agg_arg_collations_for_comparison(DTCollation &c,
                                          Item **items, uint nitems,
-                                         uint flags= 0)
+                                         uint flags)
   {
     return agg_item_collations_for_comparison(c, func_name(),
                                               items, nitems, flags);
   }
   bool agg_arg_charsets(DTCollation &c, Item **items, uint nitems,
-                        uint flags= 0)
+                        uint flags, int item_sep)
   {
-    return agg_item_charsets(c, func_name(), items, nitems, flags);
+    return agg_item_charsets(c, func_name(), items, nitems, flags, item_sep);
   }
   bool walk(Item_processor processor, byte *arg);
   Item *transform(Item_transformer transformer, byte *arg);

sql/item_strfunc.cc

@@ -405,7 +405,7 @@ void Item_func_concat::fix_length_and_dec()
 {
   ulonglong max_result_length= 0;
 
-  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV))
+  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV, 1))
     return;
 
   for (uint i=0 ; i < arg_count ; i++)
@@ -727,7 +727,7 @@ void Item_func_concat_ws::fix_length_and_dec()
 {
   ulonglong max_result_length;
 
-  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV))
+  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV, 1))
     return;
 
   /*
@@ -937,7 +937,7 @@ void Item_func_replace::fix_length_and_dec()
   }
   max_length= (ulong) max_result_length;
   
-  if (agg_arg_charsets(collation, args, 3, MY_COLL_CMP_CONV))
+  if (agg_arg_charsets(collation, args, 3, MY_COLL_CMP_CONV, 1))
     return;
 }
 
@@ -982,15 +982,11 @@ null:
 
 void Item_func_insert::fix_length_and_dec()
 {
-  Item *cargs[2];
   ulonglong max_result_length;
 
-  cargs[0]= args[0];
-  cargs[1]= args[3];
-  if (agg_arg_charsets(collation, cargs, 2, MY_COLL_ALLOW_CONV))
+  // Handle character set for args[0] and args[3].
+  if (agg_arg_charsets(collation, &args[0], 2, MY_COLL_ALLOW_CONV, 3))
     return;
-  args[0]= cargs[0];
-  args[3]= cargs[1];
   max_result_length= ((ulonglong) args[0]->max_length+
                       (ulonglong) args[3]->max_length);
   if (max_result_length >= MAX_BLOB_WIDTH)
@@ -1161,7 +1157,7 @@ void Item_func_substr_index::fix_length_and_dec()
 { 
   max_length= args[0]->max_length;
 
-  if (agg_arg_charsets(collation, args, 2, MY_COLL_CMP_CONV))
+  if (agg_arg_charsets(collation, args, 2, MY_COLL_CMP_CONV, 1))
     return;
 }
 
@@ -1497,13 +1493,10 @@ void Item_func_trim::fix_length_and_dec()
   }
   else
   {
-    Item *cargs[2];
-    cargs[0]= args[1];
-    cargs[1]= args[0];
-    if (agg_arg_charsets(collation, cargs, 2, MY_COLL_CMP_CONV))
+    // Handle character set for args[1] and args[0].
+    // Note that we pass args[1] as the first item, and args[0] as the second.
+    if (agg_arg_charsets(collation, &args[1], 2, MY_COLL_CMP_CONV, -1))
       return;
-    args[0]= cargs[1];
-    args[1]= cargs[0];
   }
 }
 
@@ -1887,7 +1880,7 @@ void Item_func_elt::fix_length_and_dec()
   max_length=0;
   decimals=0;
 
-  if (agg_arg_charsets(collation, args+1, arg_count-1, MY_COLL_ALLOW_CONV))
+  if (agg_arg_charsets(collation, args+1, arg_count-1, MY_COLL_ALLOW_CONV, 1))
     return;
 
   for (uint i= 1 ; i < arg_count ; i++)
@@ -1954,7 +1947,7 @@ void Item_func_make_set::fix_length_and_dec()
 {
   max_length=arg_count-1;
 
-  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV))
+  if (agg_arg_charsets(collation, args, arg_count, MY_COLL_ALLOW_CONV, 1))
     return;
   
   for (uint i=0 ; i < arg_count ; i++)
@@ -2162,14 +2155,9 @@ err:
 
 void Item_func_rpad::fix_length_and_dec()
 {
-  Item *cargs[2];
-
-  cargs[0]= args[0];
-  cargs[1]= args[2];
-  if (agg_arg_charsets(collation, cargs, 2, MY_COLL_ALLOW_CONV))
+  // Handle character set for args[0] and args[2].
+  if (agg_arg_charsets(collation, &args[0], 2, MY_COLL_ALLOW_CONV, 2))
     return;
-  args[0]= cargs[0];
-  args[2]= cargs[1];
   if (args[1]->const_item())
   {
     ulonglong length= ((ulonglong) args[1]->val_int() *
@@ -2249,13 +2237,9 @@ String *Item_func_rpad::val_str(String *str)
 
 void Item_func_lpad::fix_length_and_dec()
 {
-  Item *cargs[2];
-  cargs[0]= args[0];
-  cargs[1]= args[2];
-  if (agg_arg_charsets(collation, cargs, 2, MY_COLL_ALLOW_CONV))
+  // Handle character set for args[0] and args[2].
+  if (agg_arg_charsets(collation, &args[0], 2, MY_COLL_ALLOW_CONV, 2))
     return;
-  args[0]= cargs[0];
-  args[2]= cargs[1];
   
   if (args[1]->const_item())
   {
@@ -2712,8 +2696,8 @@ void Item_func_export_set::fix_length_and_dec()
   uint sep_length=(arg_count > 3 ? args[3]->max_length : 1);
   max_length=length*64+sep_length*63;
 
-  if (agg_arg_charsets(collation, args+1, min(4,arg_count)-1),
-                       MY_COLL_ALLOW_CONV)
+  if (agg_arg_charsets(collation, args+1, min(4,arg_count)-1,
+                       MY_COLL_ALLOW_CONV, 1))
     return;
 }
 

sql/item_sum.cc

@@ -3229,7 +3229,7 @@ Item_func_group_concat::fix_fields(THD *thd, Item **ref)
                         args,
 			/* skip charset aggregation for order columns */
 			arg_count - arg_count_order,
-			MY_COLL_ALLOW_CONV))
+			MY_COLL_ALLOW_CONV, 1))
     return 1;
 
   result.set_charset(collation.collation);