MDEV-16385 ROW SP variable is allowed in unexpected context

The problem described in the bug report happened because the code
did not test check_cols(1) after fix_fields() in a few places.

Additionally, fix_fields() could be called multiple times for SP variables,
because they are all fixed at a early stage in append_for_log().

Solution:
1. Adding a few helper methods
   - fix_fields_if_needed()
   - fix_fields_if_needed_for_scalar()
   - fix_fields_if_needed_for_bool()
   - fix_fields_if_needed_for_order_by()
  and using it in many cases instead of fix_fields() where
  the "fixed" status is not definitely known to be "false".

2. Adding DBUG_ASSERT(!fixed) into Item_splocal*::fix_fields()
   to catch double execution.

3. Adding tests.

As a good side effect, the patch removes a lot of duplicate code (~60 lines):

   if (!item->fixed &&
       item->fix_fields(..) &&
       item->check_cols(1))
     return true;

mysql-test/main/sp-row.result

@@ -2281,3 +2281,28 @@ t1	CREATE TABLE `t1` (
   `int11` int(11) DEFAULT NULL,
   `text1` text DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
+#
+# MDEV-16385 ROW SP variable is allowed in unexpected context
+#
+CREATE TABLE t1 (a INT);
+BEGIN NOT ATOMIC
+DECLARE row ROW(a INT);
+SELECT * FROM t1 ORDER BY row;
+END;
+$$
+ERROR 21000: Operand should contain 1 column(s)
+DROP TABLE t1;
+CREATE TABLE t1 (a INT);
+BEGIN NOT ATOMIC
+DECLARE row ROW(a INT);
+SELECT * FROM t1 HAVING row;
+END;
+$$
+ERROR 21000: Operand should contain 1 column(s)
+DROP TABLE t1;
+BEGIN NOT ATOMIC
+DECLARE a ROW(a INT);
+SELECT 1 LIKE 2 ESCAPE a;
+END;
+$$
+ERROR 21000: Operand should contain 1 column(s)

mysql-test/main/sp-row.test

@@ -1504,3 +1504,38 @@ BEGIN NOT ATOMIC
 END;
 $$
 DELIMITER ;$$
+
+--echo #
+--echo # MDEV-16385 ROW SP variable is allowed in unexpected context
+--echo #
+
+CREATE TABLE t1 (a INT);
+DELIMITER $$;
+--error ER_OPERAND_COLUMNS
+BEGIN NOT ATOMIC
+  DECLARE row ROW(a INT);
+  SELECT * FROM t1 ORDER BY row;
+END;
+$$
+DELIMITER ;$$
+DROP TABLE t1;
+
+CREATE TABLE t1 (a INT);
+DELIMITER $$;
+--error ER_OPERAND_COLUMNS
+BEGIN NOT ATOMIC
+  DECLARE row ROW(a INT);
+  SELECT * FROM t1 HAVING row;
+END;
+$$
+DELIMITER ;$$
+DROP TABLE t1;
+
+DELIMITER $$;
+--error ER_OPERAND_COLUMNS
+BEGIN NOT ATOMIC
+  DECLARE a ROW(a INT);
+  SELECT 1 LIKE 2 ESCAPE a;
+END;
+$$
+DELIMITER ;$$

sql/item.cc

@@ -1827,6 +1827,7 @@ Item_field *Item_splocal::get_variable(sp_rcontext *ctx) const
 
 bool Item_splocal::fix_fields(THD *thd, Item **ref)
 {
+  DBUG_ASSERT(!fixed);
   Item *item= get_variable(thd->spcont);
   set_handler(item->type_handler());
   return fix_fields_from_item(thd, ref, item);
@@ -1954,6 +1955,7 @@ bool Item_splocal::check_cols(uint n)
 
 bool Item_splocal_row_field::fix_fields(THD *thd, Item **ref)
 {
+  DBUG_ASSERT(!fixed);
   Item *item= get_variable(thd->spcont)->element_index(m_field_idx);
   return fix_fields_from_item(thd, ref, item);
 }
@@ -2011,6 +2013,7 @@ bool Item_splocal_row_field::set_value(THD *thd, sp_rcontext *ctx, Item **it)
 
 bool Item_splocal_row_field_by_name::fix_fields(THD *thd, Item **it)
 {
+  DBUG_ASSERT(!fixed);
   m_thd= thd;
   if (get_rcontext(thd->spcont)->find_row_field_by_name_or_error(&m_field_idx,
                                                                  m_var_idx,
@@ -2231,10 +2234,8 @@ bool Item_name_const::fix_fields(THD *thd, Item **ref)
   String s(buf, sizeof(buf), &my_charset_bin);
   s.length(0);
 
-  if ((!value_item->fixed &&
-       value_item->fix_fields(thd, &value_item)) ||
-      (!name_item->fixed &&
-       name_item->fix_fields(thd, &name_item)) ||
+  if (value_item->fix_fields_if_needed(thd, &value_item) ||
+      name_item->fix_fields_if_needed(thd, &name_item) ||
       !value_item->const_item() ||
       !name_item->const_item() ||
       !(item_name= name_item->val_str(&s))) // Can't have a NULL name 
@@ -9022,8 +9023,7 @@ bool Item_direct_view_ref::fix_fields(THD *thd, Item **reference)
         bitmap_set_bit(fld->table->read_set, fld->field_index);
     }
   }
-  else if (!(*ref)->fixed &&
-           ((*ref)->fix_fields(thd, ref)))
+  else if ((*ref)->fix_fields_if_needed(thd, ref))
     return TRUE;
 
   if (Item_direct_ref::fix_fields(thd, reference))
@@ -9051,7 +9051,7 @@ bool Item_outer_ref::fix_fields(THD *thd, Item **reference)
 {
   bool err;
   /* outer_ref->check_cols() will be made in Item_direct_ref::fix_fields */
-  if ((*ref) && !(*ref)->fixed && ((*ref)->fix_fields(thd, reference)))
+  if ((*ref) && (*ref)->fix_fields_if_needed(thd, reference))
     return TRUE;
   err= Item_direct_ref::fix_fields(thd, reference);
   if (!outer_ref)
@@ -9290,7 +9290,7 @@ bool Item_default_value::fix_fields(THD *thd, Item **items)
     fixed= 1;
     return FALSE;
   }
-  if (!arg->fixed && arg->fix_fields(thd, &arg))
+  if (arg->fix_fields_if_needed(thd, &arg))
     goto error;
 
 
@@ -9635,15 +9635,9 @@ bool Item_trigger_field::set_value(THD *thd, sp_rcontext * /*ctx*/, Item **it)
 {
   Item *item= thd->sp_prepare_func_item(it);
 
-  if (!item)
+  if (!item || fix_fields_if_needed(thd, NULL))
     return true;
 
-  if (!fixed)
-  {
-    if (fix_fields(thd, NULL))
-      return true;
-  }
-
   // NOTE: field->table->copy_blobs should be false here, but let's
   // remember the value at runtime to avoid subtle bugs.
   bool copy_blobs_saved= field->table->copy_blobs;

sql/item.h

@@ -816,6 +816,23 @@ class Item: public Value_source,
   void init_make_send_field(Send_field *tmp_field,enum enum_field_types type);
   virtual void cleanup();
   virtual void make_send_field(THD *thd, Send_field *field);
+
+  bool fix_fields_if_needed(THD *thd, Item **ref)
+  {
+    return fixed ? false : fix_fields(thd, ref);
+  }
+  bool fix_fields_if_needed_for_scalar(THD *thd, Item **ref)
+  {
+    return fix_fields_if_needed(thd, ref) || check_cols(1);
+  }
+  bool fix_fields_if_needed_for_bool(THD *thd, Item **ref)
+  {
+    return fix_fields_if_needed_for_scalar(thd, ref);
+  }
+  bool fix_fields_if_needed_for_order_by(THD *thd, Item **ref)
+  {
+    return fix_fields_if_needed_for_scalar(thd, ref);
+  }
   virtual bool fix_fields(THD *, Item **);
   /*
     Fix after some tables has been pulled out. Basically re-calculate all
@@ -4941,8 +4958,7 @@ class Item_direct_ref :public Item_ref
 
   bool fix_fields(THD *thd, Item **it)
   {
-    if ((!(*ref)->fixed && (*ref)->fix_fields(thd, ref)) ||
-        (*ref)->check_cols(1))
+    if ((*ref)->fix_fields_if_needed_for_scalar(thd, ref))
       return TRUE;
     return Item_ref::fix_fields(thd, it);
   }
@@ -4980,8 +4996,7 @@ class Item_direct_ref_to_ident :public Item_direct_ref
   bool fix_fields(THD *thd, Item **it)
   {
     DBUG_ASSERT(ident->type() == FIELD_ITEM || ident->type() == REF_ITEM);
-    if ((!ident->fixed && ident->fix_fields(thd, ref)) ||
-        ident->check_cols(1))
+    if (ident->fix_fields_if_needed_for_scalar(thd, ref))
       return TRUE;
     set_properties();
     return FALSE;

sql/item_cmpfunc.cc

@@ -1242,7 +1242,7 @@ bool Item_in_optimizer::fix_left(THD *thd)
     ref0= &(((Item_in_subselect *)args[1])->left_expr);
     args[0]= ((Item_in_subselect *)args[1])->left_expr;
   }
-  if ((!(*ref0)->fixed && (*ref0)->fix_fields(thd, ref0)) ||
+  if ((*ref0)->fix_fields_if_needed(thd, ref0) ||
       (!cache && !(cache= (*ref0)->get_cache(thd))))
     DBUG_RETURN(1);
   /*
@@ -1327,7 +1327,7 @@ bool Item_in_optimizer::fix_fields(THD *thd, Item **ref)
   if (args[0]->maybe_null)
     maybe_null=1;
 
-  if (!args[1]->fixed && args[1]->fix_fields(thd, args+1))
+  if (args[1]->fix_fields_if_needed(thd, args + 1))
     return TRUE;
   if (!invisible_mode() &&
       ((sub && ((col= args[0]->cols()) != sub->engine->cols())) ||
@@ -4586,11 +4586,9 @@ Item_cond::fix_fields(THD *thd, Item **ref)
         thd->restore_active_arena(arena, &backup);
     }
 
-    // item can be substituted in fix_fields
-    if ((!item->fixed &&
-	 item->fix_fields(thd, li.ref())) ||
-	(item= *li.ref())->check_cols(1))
+    if (item->fix_fields_if_needed_for_bool(thd, li.ref()))
       return TRUE; /* purecov: inspected */
+    item= *li.ref(); // item can be substituted in fix_fields
     used_tables_cache|=     item->used_tables();
     if (item->const_item() && !item->with_param &&
         !item->is_expensive() && !cond_has_datetime_is_null(item))
@@ -5306,7 +5304,7 @@ bool Item_func_like::fix_fields(THD *thd, Item **ref)
 {
   DBUG_ASSERT(fixed == 0);
   if (Item_bool_func2::fix_fields(thd, ref) ||
-      escape_item->fix_fields(thd, &escape_item) ||
+      escape_item->fix_fields_if_needed_for_scalar(thd, &escape_item) ||
       fix_escape_item(thd, escape_item, &cmp_value1, escape_used_in_parsing,
                       cmp_collation.collation, &escape))
     return TRUE;

sql/item_func.cc

@@ -360,7 +360,7 @@ Item_func::fix_fields(THD *thd, Item **ref)
 	We can't yet set item to *arg as fix_fields may change *arg
 	We shouldn't call fix_fields() twice, so check 'fixed' field first
       */
-      if ((!(*arg)->fixed && (*arg)->fix_fields(thd, arg)))
+      if ((*arg)->fix_fields_if_needed(thd, arg))
 	return TRUE;				/* purecov: inspected */
       item= *arg;
 
@@ -3279,13 +3279,10 @@ udf_handler::fix_fields(THD *thd, Item_func_or_sum *func,
 	 arg != arg_end ;
 	 arg++,i++)
     {
-      if (!(*arg)->fixed &&
-          (*arg)->fix_fields(thd, arg))
-	DBUG_RETURN(1);
+      if ((*arg)->fix_fields_if_needed_for_scalar(thd, arg))
+	DBUG_RETURN(true);
       // we can't assign 'item' before, because fix_fields() can change arg
       Item *item= *arg;
-      if (item->check_cols(1))
-	DBUG_RETURN(TRUE);
       /*
 	TODO: We should think about this. It is not always
 	right way just to set an UDF result to return my_charset_bin

sql/item_row.cc

@@ -41,8 +41,7 @@ bool Item_row::fix_fields(THD *thd, Item **ref)
   Item **arg, **arg_end;
   for (arg= args, arg_end= args + arg_count; arg != arg_end ; arg++)
   {
-    if (!(*arg)->fixed &&
-        (*arg)->fix_fields(thd, arg))
+    if ((*arg)->fix_fields_if_needed(thd, arg))
       return TRUE;
     // we can't assign 'item' before, because fix_fields() can change arg
     Item *item= *arg;

sql/item_subselect.cc

@@ -302,8 +302,7 @@ bool Item_subselect::fix_fields(THD *thd_param, Item **ref)
 	engine->exclude();
       substitution= 0;
       thd->where= "checking transformed subquery";
-      if (!(*ref)->fixed)
-	res= (*ref)->fix_fields(thd, ref);
+      res= (*ref)->fix_fields_if_needed(thd, ref);
       goto end;
 
     }
@@ -2181,7 +2180,7 @@ Item_in_subselect::create_single_in_to_exists_cond(JOIN *join,
         row_value_transformer?
       */
       item->name= in_additional_cond;
-      if (!item->fixed && item->fix_fields(thd, 0))
+      if (item->fix_fields_if_needed(thd, 0))
         DBUG_RETURN(true);
       *where_item= item;
     }
@@ -2504,7 +2503,7 @@ Item_in_subselect::create_row_in_to_exists_cond(JOIN * join,
 
   if (*where_item)
   {
-    if (!(*where_item)->fixed && (*where_item)->fix_fields(thd, 0))
+    if ((*where_item)->fix_fields_if_needed(thd, 0))
       DBUG_RETURN(true);
     (*where_item)->top_level_item();
   }
@@ -2651,7 +2650,7 @@ bool Item_in_subselect::inject_in_to_exists_cond(JOIN *join_arg)
     }
 
     where_item= and_items(thd, join_arg->conds, where_item);
-    if (!where_item->fixed && where_item->fix_fields(thd, 0))
+    if (where_item->fix_fields_if_needed(thd, 0))
       DBUG_RETURN(true);
     // TIMOUR TODO: call optimize_cond() for the new where clause
     thd->change_item_tree(&select_lex->where, where_item);
@@ -3117,7 +3116,7 @@ bool Item_exists_subselect::exists2in_processor(void *opt_arg)
         exp= optimizer;
     }
     upper_not->arguments()[0]= exp;
-    if (!exp->fixed && exp->fix_fields(thd, upper_not->arguments()))
+    if (exp->fix_fields_if_needed(thd, upper_not->arguments()))
     {
       res= TRUE;
       goto out;
@@ -3315,8 +3314,7 @@ bool Item_in_subselect::fix_fields(THD *thd_arg, Item **ref)
     }
   }
 
-  if (left_expr && !left_expr->fixed &&
-      left_expr->fix_fields(thd_arg, &left_expr))
+  if (left_expr && left_expr->fix_fields_if_needed(thd_arg, &left_expr))
     goto err;
   else
   if (Item_subselect::fix_fields(thd_arg, ref))
@@ -4999,8 +4997,8 @@ bool subselect_hash_sj_engine::init(List<Item> *tmp_columns, uint subquery_id)
     Repeat name resolution for 'cond' since cond is not part of any
     clause of the query, and it is not 'fixed' during JOIN::prepare.
   */
-  if (semi_join_conds && !semi_join_conds->fixed &&
-      semi_join_conds->fix_fields(thd, (Item**)&semi_join_conds))
+  if (semi_join_conds &&
+      semi_join_conds->fix_fields_if_needed(thd, (Item**)&semi_join_conds))
     DBUG_RETURN(TRUE);
   /* Let our engine reuse this query plan for materialization. */
   materialize_join= materialize_engine->join;

sql/item_sum.cc

@@ -1129,7 +1129,7 @@ Item_sum_num::fix_fields(THD *thd, Item **ref)
   maybe_null= sum_func() != COUNT_FUNC;
   for (uint i=0 ; i < arg_count ; i++)
   {
-    if (args[i]->fix_fields(thd, args + i) || args[i]->check_cols(1))
+    if (args[i]->fix_fields_if_needed_for_scalar(thd, &args[i]))
       return TRUE;
     set_if_bigger(decimals, args[i]->decimals);
     m_with_subquery|= args[i]->with_subquery();
@@ -1156,14 +1156,11 @@ Item_sum_hybrid::fix_fields(THD *thd, Item **ref)
   DBUG_ENTER("Item_sum_hybrid::fix_fields");
   DBUG_ASSERT(fixed == 0);
 
-  Item *item= args[0];
-
   if (init_sum_func_check(thd))
     DBUG_RETURN(TRUE);
 
   // 'item' can be changed during fix_fields
-  if ((!item->fixed && item->fix_fields(thd, args)) ||
-      (item= args[0])->check_cols(1))
+  if (args[0]->fix_fields_if_needed_for_scalar(thd, &args[0]))
     DBUG_RETURN(TRUE);
 
   m_with_subquery= args[0]->with_subquery();
@@ -1298,7 +1295,7 @@ Item_sum_sp::fix_fields(THD *thd, Item **ref)
 
   for (uint i= 0 ; i < arg_count ; i++)
   {
-    if (args[i]->fix_fields(thd, args + i) || args[i]->check_cols(1))
+    if (args[i]->fix_fields_if_needed_for_scalar(thd, &args[i]))
       return TRUE;
     set_if_bigger(decimals, args[i]->decimals);
     m_with_subquery|= args[i]->with_subquery();
@@ -3909,9 +3906,7 @@ Item_func_group_concat::fix_fields(THD *thd, Item **ref)
 
   for (i=0 ; i < arg_count ; i++)
   {
-    if ((!args[i]->fixed &&
-         args[i]->fix_fields(thd, args + i)) ||
-        args[i]->check_cols(1))
+    if (args[i]->fix_fields_if_needed_for_scalar(thd, &args[i]))
       return TRUE;
     m_with_subquery|= args[i]->with_subquery();
     with_param|= args[i]->with_param;

sql/item_windowfunc.cc

@@ -327,10 +327,8 @@ bool Item_sum_hybrid_simple::fix_fields(THD *thd, Item **ref)
 
   for (uint i= 0; i < arg_count; i++)
   {
-    Item *item= args[i];
     // 'item' can be changed during fix_fields
-    if ((!item->fixed && item->fix_fields(thd, args)) ||
-        (item= args[i])->check_cols(1))
+    if (args[i]->fix_fields_if_needed_for_scalar(thd, &args[i]))
       return TRUE;
   }
   Type_std_attributes::set(args[0]);