Bug#14466617 - INVALID WRITES AND/OR CRASH WITH USER

               VARIABLES 

Analysis:
-------------
After executing the query, new value of the user defined
variables are set in the function "select_dumpvar::send_data".
"select_dumpvar::send_data" first calls function 
"Item_func_set_user_var::save_item_result()". This function
checks the nullness of the Item_field passed as parameter 
to it and saves it. The nullness of item is stored with 
arg[0]'s null_value flag. Then "select_dumpvar::send_data" calls
"Item_func_set_user_var::update()" which notices null 
result that was saved and calls "Item_func_set_user_var::
update_hash". But here null_value is not set and args[0]
is different from that given to function "Item_func_set_user_var::
set_item_result()". This causes "Item_func_set_user_var::
update_hash" function to believe that its getting non-null value.
"user_var_entry::length" set to 0 and hence "user_var_entry::value"
is made to point to extra_area allocated in "user_var_entry".
And "Item_func_set_user_var::update_hash" tries to write
at memory beyond extra_area for result type DECIMAL. Because of 
this invalid write issue is reported by Valgrind.

Before this bug was introduced, we avoided this problem by 
creating "Item_func_set_user_var" object with the same 
Item_field as arg[0] and as parameter to 
Item_func_set_user_var::save_item_result(). But now 
they are refering to different args[0]. Because of this
null_value flag set in parameter Item_field in function
"Item_func_set_user_var::save_item_result()" is not
reflected in "Item_func_set_user_var" object.

Fix:
------------
This issue is reported on versions 5.5.24. Issue does not exists
in 5.5.23, 5.1, 5.6 and trunk.

This issue was introduced by
revid:georgi.kodinov@oracle.com-20120309130449-82e3bs5v3et1x0ef (fix for
bug #12408412), which was pushed into 5.5 and later releases. This patch
has later been reversed in 5.6 and trunk by
revid:norvald.ryeng@oracle.com-20121010135242-xj34gg73h04hrmyh (fix for
bug #14664077). Backported this patch in 5.5 also to fix this issue.


sql/item_func.cc:
  here unsigned value is converted to signed value.
sql/item_func.h:
  last_insert_id() gives an auto_incremented value which can be
  positive only,so defined it as a unsigned longlong sets the
  unsigned_flag to 1.

7 files changed, 61 insertions, 54 deletions

diff --git a/sql/item_func.cc b/sql/item_func.cc
index be8ee84712a..39d19fab421 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -4222,13 +4222,18 @@ bool Item_func_set_user_var::set_entry(THD *thd, bool create_if_not_exists)
     return TRUE;
   }
   entry_thread_id= thd->thread_id;
-  /* 
-     Remember the last query which updated it, this way a query can later know
-     if this variable is a constant item in the query (it is if update_query_id
-     is different from query_id).
-  */
+
 end:
-  entry->update_query_id= thd->query_id;
+  /*
+    Remember the last query which updated it, this way a query can later know
+    if this variable is a constant item in the query (it is if update_query_id
+    is different from query_id).
+
+    If this object has delayed setting of non-constness, we delay this
+    until Item_func_set-user_var::save_item_result()
+  */
+  if (!delayed_non_constness)
+    entry->update_query_id= thd->query_id;
   return FALSE;
 }
 
@@ -4616,6 +4621,13 @@ void Item_func_set_user_var::save_item_result(Item *item)
     DBUG_ASSERT(0);
     break;
   }
+  /*
+    Set the ID of the query that last updated this variable. This is
+    usually set by Item_func_set_user_var::set_entry(), but if this
+    item has delayed setting of non-constness, we must do it now.
+   */
+  if (delayed_non_constness)
+    entry->update_query_id= current_thd->query_id;
   DBUG_VOID_RETURN;
 }
 
@@ -5022,7 +5034,8 @@ get_var_with_binlog(THD *thd, enum_sql_command sql_command,
     thd->lex= &lex_tmp;
     lex_start(thd);
     tmp_var_list.push_back(new set_var_user(new Item_func_set_user_var(name,
-                                                                       new Item_null())));
+                                                                       new Item_null(),
+                                                                       false)));
     /* Create the variable */
     if (sql_set_variables(thd, &tmp_var_list))
     {
@@ -5185,7 +5198,7 @@ bool Item_func_get_user_var::eq(const Item *item, bool binary_cmp) const
 bool Item_func_get_user_var::set_value(THD *thd,
                                        sp_rcontext * /*ctx*/, Item **it)
 {
-  Item_func_set_user_var *suv= new Item_func_set_user_var(get_name(), *it);
+  Item_func_set_user_var *suv= new Item_func_set_user_var(get_name(), *it, false);
   /*
     Item_func_set_user_var is not fixed after construction, call
     fix_fields().
diff --git a/sql/item_func.h b/sql/item_func.h
index 6ac0b1b82bd..b897dcfaaaa 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -1457,6 +1457,20 @@ class Item_func_set_user_var :public Item_func
        user variable it the first connection context).
   */
   my_thread_id entry_thread_id;
+  /**
+    Delayed setting of non-constness.
+
+    Normally, Item_func_get_user_var objects are tagged as not const
+    when Item_func_set_user_var::fix_fields() is called for the same
+    variable in the same query. If delayed_non_constness is set, the
+    tagging is delayed until the variable is actually set. This means
+    that Item_func_get_user_var objects will still be treated as a
+    constant by the optimizer and executor until the variable is
+    actually changed.
+
+    @see select_dumpvar::send_data().
+   */
+  bool delayed_non_constness;
   char buffer[MAX_FIELD_WIDTH];
   String value;
   my_decimal decimal_buff;
@@ -1471,9 +1485,9 @@ class Item_func_set_user_var :public Item_func
 
 public:
   LEX_STRING name; // keep it public
-  Item_func_set_user_var(LEX_STRING a,Item *b)
+  Item_func_set_user_var(LEX_STRING a,Item *b, bool delayed)
     :Item_func(b), cached_result_type(INT_RESULT),
-     entry(NULL), entry_thread_id(0), name(a)
+     entry(NULL), entry_thread_id(0), delayed_non_constness(delayed), name(a)
   {}
   enum Functype functype() const { return SUSERVAR_FUNC; }
   double val_real();
diff --git a/sql/log_event.cc b/sql/log_event.cc
index eb848927c12..766c079d3a6 100644
--- a/sql/log_event.cc
+++ b/sql/log_event.cc
@@ -6164,7 +6164,8 @@ int User_var_log_event::do_apply_event(Relay_log_info const *rli)
     }
   }
 
-  Item_func_set_user_var *e= new Item_func_set_user_var(user_var_name, it);
+  Item_func_set_user_var *e= 
+    new Item_func_set_user_var(user_var_name, it, false);
   /*
     Item_func_set_user_var can't substitute something else on its place =>
     0 can be passed as last argument (reference on item)
diff --git a/sql/protocol.cc b/sql/protocol.cc
index 5c1533ad10d..5736a74638d 100644
--- a/sql/protocol.cc
+++ b/sql/protocol.cc
@@ -1213,7 +1213,7 @@ bool Protocol_text::send_out_parameters(List<Item_param> *sp_params)
       continue; // It's an IN-parameter.
 
     Item_func_set_user_var *suv=
-      new Item_func_set_user_var(*user_var_name, item_param);
+      new Item_func_set_user_var(*user_var_name, item_param, false);
     /*
       Item_func_set_user_var is not fixed after construction, call
       fix_fields().
diff --git a/sql/sql_class.cc b/sql/sql_class.cc
index b0d7cac1864..e32844d06ea 100644
--- a/sql/sql_class.cc
+++ b/sql/sql_class.cc
@@ -2884,12 +2884,7 @@ bool select_exists_subselect::send_data(List<Item> &items)
 int select_dumpvar::prepare(List<Item> &list, SELECT_LEX_UNIT *u)
 {
   unit= u;
-  List_iterator_fast<my_var> var_li(var_list);
-  List_iterator_fast<Item> it(list);
-  Item *item;
-  my_var *mv;
-  Item_func_set_user_var **suv;
-  
+
   if (var_list.elements != list.elements)
   {
     my_message(ER_WRONG_NUMBER_OF_COLUMNS_IN_SELECT,
@@ -2897,29 +2892,6 @@ int select_dumpvar::prepare(List<Item> &list, SELECT_LEX_UNIT *u)
     return 1;
   }
 
-  /*
-    Iterate over the destination variables and mark them as being
-    updated in this query.
-    We need to do this at JOIN::prepare time to ensure proper
-    const detection of Item_func_get_user_var that is determined
-    by the presence of Item_func_set_user_vars
-  */
-
-  suv= set_var_items= (Item_func_set_user_var **) 
-    sql_alloc(sizeof(Item_func_set_user_var *) * list.elements);
-
-  while ((mv= var_li++) && (item= it++))
-  {
-    if (!mv->local)
-    {
-      *suv= new Item_func_set_user_var(mv->s, item);
-      (*suv)->fix_fields(thd, 0);
-    }
-    else
-      *suv= NULL;
-    suv++;
-  }
-
   return 0;
 }
 
@@ -3236,33 +3208,41 @@ bool select_dumpvar::send_data(List<Item> &items)
   List_iterator<Item> it(items);
   Item *item;
   my_var *mv;
-  Item_func_set_user_var **suv;
   DBUG_ENTER("select_dumpvar::send_data");
 
   if (unit->offset_limit_cnt)
   {						// using limit offset,count
     unit->offset_limit_cnt--;
-    DBUG_RETURN(0);
+    DBUG_RETURN(false);
   }
   if (row_count++) 
   {
     my_message(ER_TOO_MANY_ROWS, ER(ER_TOO_MANY_ROWS), MYF(0));
-    DBUG_RETURN(1);
+    DBUG_RETURN(true);
   }
-  for (suv= set_var_items; ((mv= var_li++) && (item= it++)); suv++)
+  while ((mv= var_li++) && (item= it++))
   {
     if (mv->local)
     {
-      DBUG_ASSERT(!*suv);
       if (thd->spcont->set_variable(thd, mv->offset, &item))
-	    DBUG_RETURN(1);
+	    DBUG_RETURN(true);
     }
     else
     {
-      DBUG_ASSERT(*suv);
-      (*suv)->save_item_result(item);
-      if ((*suv)->update())
-        DBUG_RETURN (1);
+      /*
+        Create Item_func_set_user_vars with delayed non-constness. We
+        do this so that Item_get_user_var::const_item() will return
+        the same result during
+        Item_func_set_user_var::save_item_result() as they did during
+        optimization and execution.
+       */
+      Item_func_set_user_var *suv=
+        new Item_func_set_user_var(mv->s, item, true);
+      if (suv->fix_fields(thd, 0))
+        DBUG_RETURN(true);
+      suv->save_item_result(item);
+      if (suv->update())
+        DBUG_RETURN(true);
     }
   }
   DBUG_RETURN(thd->is_error());
diff --git a/sql/sql_class.h b/sql/sql_class.h
index d799980f8e1..4f65aa25796 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -3552,7 +3552,6 @@ public:
 
 class select_dumpvar :public select_result_interceptor {
   ha_rows row_count;
-  Item_func_set_user_var **set_var_items;
 public:
   List<my_var> var_list;
   select_dumpvar()  { var_list.empty(); row_count= 0;}
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index e93ed4e8853..7f1b5a57ac4 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -8980,7 +8980,7 @@ variable_aux:
           ident_or_text SET_VAR expr
           {
             Item_func_set_user_var *item;
-            $$= item= new (YYTHD->mem_root) Item_func_set_user_var($1, $3);
+            $$= item= new (YYTHD->mem_root) Item_func_set_user_var($1, $3, false);
             if ($$ == NULL)
               MYSQL_YYABORT;
             LEX *lex= Lex;
@@ -12970,7 +12970,7 @@ option_value:
           '@' ident_or_text equal expr
           {
             Item_func_set_user_var *item;
-            item= new (YYTHD->mem_root) Item_func_set_user_var($2, $4);
+            item= new (YYTHD->mem_root) Item_func_set_user_var($2, $4, false);
             if (item == NULL)
               MYSQL_YYABORT;
             set_var_user *var= new set_var_user(item);