maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 10/07/2017 04:52 PM, Sergei Golubchik wrote:
> 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?

Thanks for the review!

I agree, the true fix removing the side effect appeared to be too big
for 5.5. Let's do it in 10.4 (or maybe 10.3).

I'm attaching a smaller version, which only handles the NULL result from
val_str(). Also, I found a new small problem with tricky charsets such
as ucs2 and fixed it by adding ErrConvString.

Please see attached.

Still, I'd like to response your comments, please see below:

> 
> 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?

No, the warnings are suppressed in *.test file.
By this comment I tried to say *why* they are suppressed.

I agree that the *.result file looked confusing.
I removed the /*sporadic warnings*/ part from the query and added a
comment before the query instead, like this:

+# The below query can return warning sporadically
+--disable_warnings
+DO TO_DAYS(SEC_TO_TIME(TIME(CEILING(UUID()))));
+--enable_warnings


> 
>> +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.

I think I am not. get_seconds_from_real() uses this method:

void Secondf_st::set(double value)
{
  if ((m_neg= value < 0))
    value= -value;
  if (value > LONGLONG_MAX)
    value= static_cast<double>(LONGLONG_MAX);
  m_second= static_cast<ulonglong>(floor(value));
  m_usecond= static_cast<ulong>((value
-floor(value))*TIME_SECOND_PART_FACTOR);
}


> 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. 

Item::val_decimal_from_string() uses exactly the same approach.
All Items with STRING_RESUL should go through this chain: val_str +
str2my_decimal.

> Some items, e.g. BIT fields, need to know in what context they're
> evaluated.

As for the BIT data type, it reports result_type() and cmp_type() as
INT_RESULT, and decimals for an BIT type Item should be 0. So both
before the patch and after the patch, BIT went through the val_int()
branch. There should not be behavior change for BIT.

I could not invent a bad example, but some unexpected anomalies are
probably still possible, e.g. for inconsistently coded Items.
Anyway, to be on the safe side, let's keep 5.5 with as small
change as possible.

> 
>> +  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.

Notice, the maxsec value passed to gets_seconds() is not always the same
with what the caller later checks again. Only
Item_func_sec_to_time::get_date() passes maxsec=TIME_MAX_VALUE_SECONDS,
while the other two calls pass LONGLONG_MAX.

This way I wanted to preserve the behavior in 5.5 a much as possible.

In 10.3 we can fix it in a better way:

- Item_func_from_unixtime::get_date() should issue a warning when
  "seconds > TIMESTAMP_MAX_VALUE". Currently it issues warnings
  only when "seconds >= LONGLONG_MAX".

  For the seconds range  TIMESTAMP_MAX_VALUE..LONGLONG_MAX
  it returns NULL silently, which is bad.

- Item_func_maketime::get_date() should issue a warning when
   "seconds > 59". Currently it issues warnings
  only when "seconds >= LONGLONG_MAX",

  For the seconds range  59..LONGLONG_MAX
  it returns NULL silently, which is bad 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
> 

commit c9f47cdd83e546674a400f2ea64a4495b8185a1c
Author: Alexander Barkov <bar@xxxxxxxxxxx>
Date:   Mon Oct 9 15:00:35 2017 +0400

    MDEV-13972 crash in Item_func_sec_to_time::get_date
    
    Item_func_sec_to_time::get_date() calls an args[0]'s 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 is possible when args[0] is not deterministic
       (for instance, uses RAND() directly or indirectly).
       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.
    
       This patch fixes this problem by checking the result of val_str().
       In case of a non-NULL, the result of val_str() is used as before.
       In case of NULL, the "ulonglong seconds" value is used for the warning.
       Note, "ulong sec_part" value is ignored in the warning for simplicity.
    
    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.
       This problem is NOT fixed by this patch to avoid big changes in 5.5.
       This will be fixed separately in 10.3 or 10.4.
    
    Also, passing err->ptr() and err->length() directly to
    make_truncated_value_warning() was wrong, because it did not take
    into account err->charset(). This caused unreadable warnings in case
    of tricky character sets, such as ucs2. This patch fixes this additional
    problem by using "ErrConvStr err2(err)".

diff --git a/mysql-test/r/ctype_ucs.result b/mysql-test/r/ctype_ucs.result
index 7a93c55..d55d308 100644
--- a/mysql-test/r/ctype_ucs.result
+++ b/mysql-test/r/ctype_ucs.result
@@ -4367,5 +4367,13 @@ NO_ENGINE_SUBSTITUTION
 SET sql_mode=DEFAULT;
 SET NAMES utf8;
 #
+# MDEV-13972 crash in Item_func_sec_to_time::get_date
+#
+SELECT SEC_TO_TIME(CONVERT(900*24*60*60 USING ucs2));
+SEC_TO_TIME(CONVERT(900*24*60*60 USING ucs2))
+838:59:59.999999
+Warnings:
+Warning	1292	Truncated incorrect time value: '77760000'
+#
 # End of 5.5 tests
 #
diff --git a/mysql-test/t/ctype_ucs.test b/mysql-test/t/ctype_ucs.test
index d94c9ae..62890d0 100644
--- a/mysql-test/t/ctype_ucs.test
+++ b/mysql-test/t/ctype_ucs.test
@@ -863,5 +863,12 @@ SET sql_mode=DEFAULT;
 SET NAMES utf8;
 
 --echo #
+--echo # MDEV-13972 crash in Item_func_sec_to_time::get_date
+--echo #
+
+SELECT SEC_TO_TIME(CONVERT(900*24*60*60 USING ucs2));
+
+
+--echo #
 --echo # End of 5.5 tests
 --echo #
diff --git a/mysql-test/t/func_time.test b/mysql-test/t/func_time.test
index 7544f9e..8323bd3 100644
--- a/mysql-test/t/func_time.test
+++ b/mysql-test/t/func_time.test
@@ -1615,6 +1615,21 @@ SELECT * FROM t1;
 DROP TABLE t1;
 SET sql_mode=DEFAULT;
 
+
+--echo #
+--echo # MDEV-13972 crash in Item_func_sec_to_time::get_date
+--echo #
+
+# The below query can return warning sporadically
+--disable_warnings
+DO TO_DAYS(SEC_TO_TIME(TIME(CEILING(UUID()))));
+--enable_warnings
+
+DO TO_DAYS(SEC_TO_TIME(MAKEDATE('',RAND(~('')))));
+SELECT TO_DAYS(SEC_TO_TIME(MAKEDATE(0,RAND(~0))));
+SELECT SEC_TO_TIME(MAKEDATE(0,RAND(~0)));
+
+
 --echo #
 --echo # End of 5.5 tests
 --echo #
diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc
index 0ed1506..eeb373a 100644
--- a/sql/item_timefunc.cc
+++ b/sql/item_timefunc.cc
@@ -1733,9 +1733,18 @@ bool Item_func_sec_to_time::get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date)
 
   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);
+  if (!err)
+  {
+    ErrConvInteger err2(sec, unsigned_flag);
+    make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
+                                 &err2, MYSQL_TIMESTAMP_TIME, NullS);
+  }
+  else
+  {
+    ErrConvString err2(err);
+    make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
+                                 &err2, MYSQL_TIMESTAMP_TIME, NullS);
+  }
   return 0;
 }