From: Jorgen Loland
Date: February 11 2011 10:00am
Subject: Re: bzr commit into mysql-5.5 branch (tor.didriksen:3323) Bug#60085
List-Archive: http://lists.mysql.com/commits/131108
Message-Id: <4D5508A4.10208@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Tor,

Thank you for the patch. I have a suspicion that there is another problem with 
the Item_singlerow_subselect::val_* functions. See comments inline.

 >  3323 Tor Didriksen	2011-02-11
 >       Bug #60085 crash in Item::save_in_field() with time data type
 >
 >      Item_singlerow_subselect::val_str() incorrectly assumed that
 >      value->val_str() could set value->null_value= true;

JL: "...could *not* set"?

 >=== modified file 'sql/item.cc'
 >--- a/sql/item.cc	2011-01-12 12:58:47 +0000
 >+++ b/ sql/item.cc	2011-02-11 08:48:59 +0000
 >@@ -1051,6 +1051,7 @@ CHARSET_INFO *Item::default_charset()
 >
 > int Item::save_in_field_no_warnings(Field *field, bool no_conversions)
 > {
 >+  DBUG_ENTER("Item::save_in_field_no_warnings");
 >   int res;
 >   TABLE *table= field->table;
 >   THD *thd= table->in_use;
 >@@ -1059,10 +1060,12 @@ int Item::save_in_field_no_warnings(Fiel
 >   ulonglong sql_mode= thd->variables.sql_mode;
 >   thd->variables.sql_mode&= ~(MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE);
 >   thd->count_cuted_fields= CHECK_FIELD_IGNORE;
 >+
 >   res= save_in_field(field, no_conversions);
 >+
 >   thd->count_cuted_fields= tmp;
 >   dbug_tmp_restore_column_map(table->write_set, old_map);
 >   thd->variables.sql_mode= sql_mode;
 >-  return res;
 >+  DBUG_RETURN(res);
 > }

JL: IMHO, this DBUG tracing is unnecessary since the function calls 
save_in_field() which is traced, but it's ok to keep it if you think it's helpful.


 >@@ -7516,10 +7532,12 @@ String *Item_cache_datetime::val_str(Str
 > {
 >   DBUG_ASSERT(fixed == 1);
 >
 >+  DBUG_ENTER("Item_cache_datetime::val_str");
 >+
 >   if ((value_cached || str_value_cached) && null_value)
 >-    return NULL;
 >+    DBUG_RETURN(NULL);
 >
 >   if (!str_value_cached)
 >   {

JL: I don't think tracing one "random" function out of the countless *::val_* 
functions is worthwile. var_* functions are also typically called for every 
evaluated record, so it will significantly increase the size of the trace as 
well. Suggest you remove tracing of this function.

 >=== modified file 'sql/item_subselect.cc'
 >--- a/sql/item_subselect.cc	2011-01-12 12:15:22 +0000
 >+++ b/sql/item_subselect.cc	2011-02-11 08:48:59 +0000
 >@@ -258,3 +258,4 @@ bool Item_subselect::exec()
 > {
 >   int res;
 >
 >+  DBUG_ENTER("Item_subselect::exec");

JL: Thank you!

 >+
 >   /*
 >     Do not execute subselect in case of a fatal error
 >     or if the query has been killed.
 >   */
 >   if (thd->is_error() || thd->killed)
 >-    return 1;
 >+    DBUG_RETURN(1);

JL: This function should return bool :)

 >   DBUG_ASSERT(!thd->lex->context_analysis_only);
 >   /*
 >     Simulate a failure in sub-query execution. Used to test e.g.
 >     out of memory or query being killed conditions.
 >   */
 >-  DBUG_EXECUTE_IF("subselect_exec_fail", return 1;);
 >+  DBUG_EXECUTE_IF("subselect_exec_fail", DBUG_RETURN(1););

JL: here as well

 >   res= engine->exec();
 >
 >   if (engine_changed)
 >   {
 >     engine_changed= 0;
 >-    return exec();
 >+    bool retval= exec();
 >+    DBUG_RETURN(retval);

JL: actually returns bool!

 >   }
 >-  return (res);
 >+  DBUG_RETURN(res);

JL: here as well

 >@@ -599,15 +604,17 @@ longlong Item_singlerow_subselect::val_i
 >
 > String *Item_singlerow_subselect::val_str(String *str)
 > {
 >+  DBUG_ENTER("Item_singlerow_subselect::val_str");
 >   if (!exec() && !value->null_value)
 >   {

JL: The bug was that this code assumes that value->null_value will not
change when value->val_* is called. As this bug shows, this is an
incorrect assumption. I think that it's unsafe to assume anything about
val->null_value before calling val_ in all Item_singlerow_subselect::val_*
functions. This is only a theory and remains to be tested. I send this
concern without investigating thoroughly because we are in a hurry with
this patch:)


 >-    null_value= FALSE;
 >-    return value->val_str(str);
 >+    String *retval= value->val_str(str);
 >+    null_value= value->null_value;
 >+    DBUG_RETURN(retval);
 >   }
 >   else
 >   {
 >     reset();
 >-    return 0;
 >+    DBUG_RETURN(0);
 >   }
 > }

JL: Again, I'm not sure tracing random val_ functions is worthwile but I'll 
leave it to you to decide.


-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway