Re: bzr commit into mysql-5.5 branch (tor.didriksen:3326) Bug#11766860

[Øystein Grøvlen]

Tor,

I observe that the following patch is an alternative way of fixing the 
problem.  It causes no failures in test suite, and it also fixes Bug#59685:

=== modified file 'sql/item.cc'
--- sql/item.cc	2011-01-12 12:58:47 +0000
+++ sql/item.cc	2011-02-17 09:57:37 +0000
@@ -3562,6 +3562,10 @@ Item_copy *Item_copy::create (Item *item
    switch (item->result_type())
    {
      case STRING_RESULT:
+      if ((item->is_datetime() ||
+           item->field_type() == MYSQL_TYPE_TIME) &&
+          item->result_as_longlong())
+        return new Item_copy_int(item);
        return new Item_copy_string (item);
      case REAL_RESULT:
        return new Item_copy_float (item);

The idea is that if the result is to be handled as longlong, use an 
Item_copy_int object instead of Item_copy_string. (Fix is inspired by 
how DATE/TIME is handled in Item_cache::get_cache()).

However, I can live with your original fix, especially since we will 
hopefully clean up this int/string mess for 5.6.  I have cc-ed a few 
people that might want to chime in here.

Anyhow, I will approve the patch.

--
Øystein


On 02/15/11 11:27 AM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/5.5-foo/ based on
> revid:joerg@stripped
>
>   3326 Tor Didriksen	2011-02-15
>        Bug #11766860 - 60085: CRASH IN ITEM::SAVE_IN_FIELD() WITH TIME DATA TYPE
>
>        This assumption in Item_cache_datetime::cache_value_int
>        was wrong:
>        -  /* Assume here that the underlying item will do correct conversion.*/
>        -  int_value= example->val_int_result();
>       @ mysql-test/r/subselect_innodb.result
>          New test case.
>       @ mysql-test/t/subselect_innodb.test
>          New test case.
>       @ sql/item.cc
>          In Item_cache_datetime::cache_value_int()
>           - call get_time() or get_date() depending on desired type
>           - convert the returned MYSQL_TIME value to longlong depending on desired
> type
>       @ sql/item.h
>          The cached int_value in Item_cache_datetime should not be unsigned:
>           - it is used mostly in signed context
>           - it can actually have negative value (for TIME data type)
>       @ sql/item_subselect.cc
>          Add some DBUG_TRACE for easier bug-hunting.
>
>      modified:
>        mysql-test/r/subselect_innodb.result
>        mysql-test/t/subselect_innodb.test
>        sql/item.cc
>        sql/item.h
>        sql/item_subselect.cc
> === modified file 'mysql-test/r/subselect_innodb.result'
> --- a/mysql-test/r/subselect_innodb.result	2006-01-26 16:54:34 +0000
> +++ b/mysql-test/r/subselect_innodb.result	2011-02-15 10:27:30 +0000
> @@ -245,3 +245,12 @@ x
>   NULL
>   drop procedure p1;
>   drop tables t1,t2,t3;
> +#
> +# Bug#60085 crash in Item::save_in_field() with time data type
> +#
> +CREATE TABLE t1(a date, b int, unique(b), unique(a), key(b)) engine=innodb;
> +INSERT INTO t1 VALUES ('2011-05-13', 0);
> +SELECT 1 FROM t1 WHERE b<  (SELECT CAST(a as date) FROM t1 GROUP BY a);
> +1
> +1
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/subselect_innodb.test'
> --- a/mysql-test/t/subselect_innodb.test	2006-01-26 16:54:34 +0000
> +++ b/mysql-test/t/subselect_innodb.test	2011-02-15 10:27:30 +0000
> @@ -238,3 +238,12 @@ call p1();
>   call p1();
>   drop procedure p1;
>   drop tables t1,t2,t3;
> +
> +--echo #
> +--echo # Bug#60085 crash in Item::save_in_field() with time data type
> +--echo #
> +
> +CREATE TABLE t1(a date, b int, unique(b), unique(a), key(b)) engine=innodb;
> +INSERT INTO t1 VALUES ('2011-05-13', 0);
> +SELECT 1 FROM t1 WHERE b<  (SELECT CAST(a as date) FROM t1 GROUP BY a);
> +DROP TABLE t1;
>
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2011-01-12 12:58:47 +0000
> +++ b/sql/item.cc	2011-02-15 10:27:30 +0000
> @@ -1059,7 +1059,9 @@ 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;
> @@ -7462,16 +7464,43 @@ longlong Item_cache_int::val_int()
>   bool  Item_cache_datetime::cache_value_int()
>   {
>     if (!example)
> -    return FALSE;
> +    return false;
>
> -  value_cached= TRUE;
> +  value_cached= true;
>     // Mark cached string value obsolete
> -  str_value_cached= FALSE;
> -  /* Assume here that the underlying item will do correct conversion.*/
> -  int_value= example->val_int_result();
> +  str_value_cached= false;
> +
> +  MYSQL_TIME ltime;
> +  const bool eval_error=
> +    (field_type() == MYSQL_TYPE_TIME) ?
> +    example->get_time(&ltime) :
> +    example->get_date(&ltime, TIME_FUZZY_DATE);
> +
> +  if (eval_error)
> +    int_value= 0;
> +  else
> +  {
> +    switch(field_type())
> +    {
> +    case MYSQL_TYPE_DATETIME:
> +    case MYSQL_TYPE_TIMESTAMP:
> +      int_value= TIME_to_ulonglong_datetime(&ltime);
> +      break;
> +    case MYSQL_TYPE_TIME:
> +      int_value= TIME_to_ulonglong_time(&ltime);
> +      break;
> +    default:
> +      int_value= TIME_to_ulonglong_date(&ltime);
> +      break;
> +    }
> +    if (ltime.neg)
> +      int_value= -int_value;
> +  }
> +
>     null_value= example->null_value;
>     unsigned_flag= example->unsigned_flag;
> -  return TRUE;
> +
> +  return true;
>   }
>
>
>
> === modified file 'sql/item.h'
> --- a/sql/item.h	2011-02-08 15:47:33 +0000
> +++ b/sql/item.h	2011-02-15 10:27:30 +0000
> @@ -3449,7 +3449,7 @@ class Item_cache_datetime: public Item_c
>   {
>   protected:
>     String str_value;
> -  ulonglong int_value;
> +  longlong int_value;
>     bool str_value_cached;
>   public:
>     Item_cache_datetime(enum_field_types field_type_arg):
>
> === 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-15 10:27:30 +0000
> @@ -256,30 +256,31 @@ bool Item_subselect::walk(Item_processor
>
>   bool Item_subselect::exec()
>   {
> -  int res;
> +  DBUG_ENTER("Item_subselect::exec");
>
>     /*
>       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(true);
>
>     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(true););
>
> -  res= engine->exec();
> +  bool res= engine->exec();
>
>     if (engine_changed)
>     {
>       engine_changed= 0;
> -    return exec();
> +    res= exec();
> +    DBUG_RETURN(res);
>     }
> -  return (res);
> +  DBUG_RETURN(res);
>   }
>
>   Item::Type Item_subselect::type() const
>
>
>
>
>


-- 
Øystein Grøvlen, Principal Software Engineer
MySQL Group, Oracle
Trondheim, Norway