maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

See comments below.

TL;DR - dunno. Changes in behavior. Lots of code for overflow checks
that is unused or duplicated. And only to avoid side effects with
invalid arguments for SEC_TO_TIME() ?

Perhaps, just check for val_str() returing NULL here and have the side
effects fixed in 10.4, when Items will return a Value object?

On Oct 05, Alexander Barkov wrote:
> Hello Sergei,
> 
> Please review a patch for MDEV-13972.
> 
> The patch is for 5.5, as we agreed in slack.
> 
> Thanks!

> commit 003c19c7677e1ce5b18b4081e21b7864700f8250
> Author: Alexander Barkov <bar@xxxxxxxxxxx>
> Date:   Thu Oct 5 21:07:19 2017 +0400
> 
>     MDEV-13972 crash in Item_func_sec_to_time::get_date
>     
>     Item_func_sec_to_time::get_date() called a value method two times:
>     - once val_int() or val_decimal() inside Item::get_seconds()
>     - then val_str() to generate a warning, in case an out-of-range value
>     
>     This approach was wrong for two reasons:
>     1. val_str() in some cases can return NULL even if val_int()/val_decimal()
>        previously returned a not-NULL value.
>        This caused the crash reported in the bug:
>        the warning generation code in Item_func_sec_to_time::get_date()
>        did not expect val_str() to return NULL.
>     2. Calling val_str() to generate warnings fires the function side effect again.
>        For example, consider SEC_TO_TIME(f1()), where f1() is a stored function
>        doing some INSERTs as a side effect. If f1() returns an out-of-range value,
>        the call for val_str() to generate the warning forces the INSERTs to happen
>        again, which is wrong. The side effect must happen only once per call.
>     
>     Fixing the code not to do a dedicated call for val_str() for warnings.
>     To preserve the old warnings recorded in *.result files, four methods
>     where added:
>     - get_seconds_from_string()
>     - get_seconds_from_int()
>     - get_seconds_from_decimal()
>     - get_seconds_from_real()
>     
>     A new structure Secondf_st was added:
>     - to avoid having too many arguments in get_seconds_from_xxx()
>     - to share common code, e.g. between double_to_datetime_with_warn()
>       and get_seconds_from_real()
>     - to share the code easier in the future
>     - to have the code in item.cc and item_timefunc.cc smaller
>     
>     As a good side effect, a redundant warning in func_time.result disappeared

Good comment!

> diff --git a/mysql-test/r/func_time.result b/mysql-test/r/func_time.result
> index 68b1e0f..b46e7f3 100644
> --- a/mysql-test/r/func_time.result
> +++ b/mysql-test/r/func_time.result
> @@ -2626,3 +2625,126 @@ DROP TABLE t1;
>  SELECT 1 MOD ADDTIME( '13:58:57', '00:00:01' ) + 2;
>  1 MOD ADDTIME( '13:58:57', '00:00:01' ) + 2
>  3
> +#
> +# MDEV-13972 crash in Item_func_sec_to_time::get_date
> +#
> +DO TO_DAYS(SEC_TO_TIME(TIME(CEILING(UUID())))) /*sporadic warnings*/;

sporadic warnings in the test file?

> +DO TO_DAYS(SEC_TO_TIME(MAKEDATE('',RAND(~('')))));
...
> diff --git a/sql/item.cc b/sql/item.cc
> index fdfbba3..73a1acd 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -1407,22 +1407,82 @@ bool Item::get_date(MYSQL_TIME *ltime,ulonglong fuzzydate)
>    return null_value|= !(fuzzydate & TIME_FUZZY_DATES);
>  }
>  
> -bool Item::get_seconds(ulonglong *sec, ulong *sec_part)
> +
> +bool Item::get_seconds(Secondf_st *sec, const char *type, ulonglong maxsec)
>  {
> -  if (decimals == 0)
> -  { // optimize for an important special case
> -    longlong val= val_int();
> -    bool neg= val < 0 && !unsigned_flag;
> -    *sec= neg ? -val : val;
> -    *sec_part= 0;
> -    return neg;
> +  switch (cmp_type()) {
> +  case STRING_RESULT:
> +    return get_seconds_from_string(sec, type, maxsec);
> +  case TIME_RESULT:
> +  case DECIMAL_RESULT:
> +    if (decimals == 0) // optimize for an important special case
> +      return get_seconds_from_int(sec, type, maxsec);
> +    return get_seconds_from_decimal(sec, type, maxsec);
> +  case INT_RESULT:
> +    return get_seconds_from_int(sec, type, maxsec);
> +  case REAL_RESULT:
> +    return get_seconds_from_real(sec, type, maxsec);
> +  case ROW_RESULT:
> +  case IMPOSSIBLE_RESULT:
> +    break;
>    }

I'm not sure that's the same as before.
First, you completely ignore decimals when converting from double.
Second, old code was doing item->val_decimal(), while you're doing, for
example, item->val_str() + str2my_decimal(). Which is not always the
same. Some items, e.g. BIT fields, need to know in what context they're
evaluated.

> +  DBUG_ASSERT(0);
> +  sec->init();
> +  return 0;
> +}
> +
> +
> +bool Item::get_seconds_from_int(Secondf_st *sec,
> +                                const char *type, ulonglong maxsec)
> +{
> +  longlong val= val_int();
> +  sec->set(val, unsigned_flag);
> +  sec->check_range_with_warn(type, maxsec, ErrConvInteger(val, unsigned_flag));
> +  return null_value;
> +}
> +
> +
> +bool Item::get_seconds_from_real(Secondf_st *sec,
> +                                 const char *type, ulonglong maxsec)
> +{
> +  double val= val_real();
> +  sec->set(val);
> +  sec->check_range_with_warn(type, maxsec, ErrConvDouble(val));
> +  return null_value;
> +}
> +
> +
> +bool Item::get_seconds_from_decimal(Secondf_st *sec,
> +                                    const char *type, ulonglong maxsec)
> +{
>    my_decimal tmp, *dec= val_decimal(&tmp);
>    if (!dec)
> -    return 0;
> -  return my_decimal2seconds(dec, sec, sec_part);
> +  {
> +    sec->init();
> +    return true;
> +  }
> +  sec->set(dec);
> +  sec->check_range_with_warn(type, maxsec, ErrConvDecimal(dec));
> +  return false;
>  }
>  
> +
> +bool Item::get_seconds_from_string(Secondf_st *sec,
> +                                   const char *type, ulonglong maxsec)
> +{
> +  char buff[64];
> +  String tmp(buff, sizeof(buff), &my_charset_bin), *res= val_str(&tmp);
> +  if (!res)
> +  {
> +    sec->init();
> +    return true;
> +  }
> +  sec->set(res);
> +  sec->check_range_with_warn(type, maxsec, ErrConvString(res));
> +  return false;
> +}
> +
> +
>  CHARSET_INFO *Item::default_charset()
>  {
>    return current_thd->variables.collation_connection;
> diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc
> index 0ed1506..72c3e95 100644
> --- a/sql/item_timefunc.cc
> +++ b/sql/item_timefunc.cc
> @@ -1700,42 +1700,33 @@ bool Item_func_sysdate_local::get_date(MYSQL_TIME *res,
>  bool Item_func_sec_to_time::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date)
>  {
>    DBUG_ASSERT(fixed == 1);
> -  bool sign;
> -  ulonglong sec;
> -  ulong sec_part;
> +  Secondf_st sec;
>  
>    bzero((char *)ltime, sizeof(*ltime));
>    ltime->time_type= MYSQL_TIMESTAMP_TIME;
>  
> -  sign= args[0]->get_seconds(&sec, &sec_part);
> -
> -  if ((null_value= args[0]->null_value))
> -    return 1;
> +  if ((null_value= (args[0]->get_seconds(&sec,
> +                                         "time", TIME_MAX_VALUE_SECONDS))))

strange parentheses around get_seconds() call.

> +    return true;
>  
> -  ltime->neg= sign;
> -  if (sec > TIME_MAX_VALUE_SECONDS)
> +  ltime->neg= sec.m_neg;
> +  if (sec.m_second > TIME_MAX_VALUE_SECONDS)
>      goto overflow;

kinda silly. get_seconds() checks for the overflow and you need to check
it here again.

> -  DBUG_ASSERT(sec_part <= TIME_MAX_SECOND_PART);
> +  DBUG_ASSERT(sec.m_usecond <= TIME_MAX_SECOND_PART);
>    
> -  ltime->hour=   (uint) (sec/3600);
> -  ltime->minute= (uint) (sec % 3600) /60;
> -  ltime->second= (uint) sec % 60;
> -  ltime->second_part= sec_part;
> +  ltime->hour=   (uint) (sec.m_second / 3600);
> +  ltime->minute= (uint) (sec.m_second % 3600) /60;
> +  ltime->second= (uint) sec.m_second % 60;
> +  ltime->second_part= sec.m_usecond;
>  
>    return 0;
>  
>  overflow:
>    /* use check_time_range() to set ltime to the max value depending on dec */
>    int unused;
> -  char buf[100];
> -  String tmp(buf, sizeof(buf), &my_charset_bin), *err= args[0]->val_str(&tmp);
> -
>    ltime->hour= TIME_MAX_HOUR+1;
>    check_time_range(ltime, decimals, &unused);
> -  make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> -                               err->ptr(), err->length(),
> -                               MYSQL_TIMESTAMP_TIME, NullS);
>    return 0;
>  }
>  
> @@ -1929,21 +1920,18 @@ void Item_func_from_unixtime::fix_length_and_dec()
>  bool Item_func_from_unixtime::get_date(MYSQL_TIME *ltime,
>  				       ulonglong fuzzy_date __attribute__((unused)))
>  {
> -  bool sign;
> -  ulonglong sec;
> -  ulong sec_part;
> +  Secondf_st sec;
>  
>    bzero((char *)ltime, sizeof(*ltime));
>    ltime->time_type= MYSQL_TIMESTAMP_TIME;
>  
> -  sign= args[0]->get_seconds(&sec, &sec_part);
> -
> -  if (args[0]->null_value || sign || sec > TIMESTAMP_MAX_VALUE)
> +  if (args[0]->get_seconds(&sec, "", ULONGLONG_MAX) ||
> +                           sec.m_neg || sec.m_second > TIMESTAMP_MAX_VALUE)
>      return (null_value= 1);

and again you need to check for the overflow outside of get_seconds().
because this one doesn't need a warning.

> -  tz->gmt_sec_to_TIME(ltime, (my_time_t)sec);
> +  tz->gmt_sec_to_TIME(ltime, (my_time_t) sec.m_second);
>  
> -  ltime->second_part= sec_part;
> +  ltime->second_part= sec.m_usecond;
>  
>    return (null_value= 0);
>  }
> @@ -2735,12 +2723,11 @@ bool Item_func_maketime::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date)
>    bool overflow= 0;
>    longlong hour=   args[0]->val_int();
>    longlong minute= args[1]->val_int();
> -  ulonglong second;
> -  ulong microsecond;
> -  bool neg= args[2]->get_seconds(&second, &microsecond);
> +  Secondf_st sec;
>
> -  if (args[0]->null_value || args[1]->null_value || args[2]->null_value ||
> -       minute < 0 || minute > 59 || neg || second > 59)
> +  if (args[0]->null_value || args[1]->null_value ||
> +      args[2]->get_seconds(&sec, "", ULONGLONG_MAX) ||
> +      minute < 0 || minute > 59 || sec.m_neg || sec.m_second > 59)

and again.

>      return (null_value= 1);
>
>    bzero(ltime, sizeof(*ltime));

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx