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 <b...@mariadb.org> > 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, µsecond); > + 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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp